[SeaBIOS] [PATCH v2] romlayout32flag.lds: Use `. +=` instead of `. =`

[Fangrui Song]

On 2020-03-17, Kevin O'Connor wrote:
> On Fri, Mar 13, 2020 at 10:17:09PM -0700, Fangrui Song wrote:
> > This improves the portability of the linker script and allows lld to link rom.o
> > We can thus delete an ld check detecting "cannot move location counter backwards".
> >
> > Dot assignment inside an output section has a inconsistent behavior
> > which makes lld difficult to implement. See https://bugs.llvm.org/show_bug.cgi?id=43083
> > Signed-off-by: Fangrui Song
> >
> > scripts/layoutrom.py  |  7 ++++++-
> >  scripts/test-build.sh | 42 +-----------------------------------------
> >  2 files changed, 7 insertions(+), 42 deletions(-)
> >
> > diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py
> > index 6616721..bbf07aa 100755
> > --- a/scripts/layoutrom.py
> > +++ b/scripts/layoutrom.py
> > @@ -335,14 +335,19 @@ def outRelSections(sections, startsym, useseg=0):
> >                  if section.finalloc is not None]
> >      sections.sort(key=operator.itemgetter(0))
> >      out = ""
> > dot = "_reloc_init_end"
> > for addr, section in sections:
> >     loc = section.finalloc
> >     if useseg:
> >         loc = section.finalsegloc
> >    out += ". = ( 0x%x - %s ) ;\n" % (loc, startsym)
> >    if isinstance(dot, str):
> >        out += ". += 0x%x - %s ;\n" % (loc, dot)
> >    elif dot < loc:
> >        out += ". += 0x%x ;\n" % (loc-dot,)
> >    if section.name in ('.rodata.str1.1', '.rodata'):
> >        out += "_rodata%s = . ;\n" % (section.fileid,)
> >    out += "*%s.*(%s)\n" % (section.fileid, section.name)
> >    dot = loc + section.size
>
> I think it's fine to use relative assignments instead of absolute
> assignments, but I'm struggling to understand the python code above.
> I recommend writing the code without isinstance() and use a more
> descriptive variable name than "dot".
> >  return out
> >
> > # Build linker script output for a list of relocations.
> > diff --git a/scripts/test-build.sh b/scripts/test-build.sh
> > index 25cc2f2..8b35d6f 100755
> > --- a/scripts/test-build.sh
> > +++ b/scripts/test-build.sh
> > @@ -4,50 +4,10 @@
> >  mkdir -p ${OUT}
> >  TMPFILE1=${OUT}/tmp_testcompile1.c
> >  TMPFILE1o=${OUT}/tmp_testcompile1.o
> > -TMPFILE1_ld=${OUT}/tmp_testcompile1.lds
> >  TMPFILE2=${OUT}/tmp_testcompile2.c
> >  TMPFILE2o=${OUT}/tmp_testcompile2.o
> >  TMPFILE3o=${OUT}/tmp_testcompile3.o
> >
> > -# Test if ld's alignment handling is correct.  This is a known problem
> > -# with the linker that ships with Ubuntu 11.04.
>
> This change seems unrelated to the change above (and it isn't
> mentioned in the commit summary).  I'm not sure why this alignment
> test (which is to catch old broken version of ld) should be removed.
> If it does need to be removed then I recommend separating it out to a
> separate patch and explain why it needs to be removed in the commit
> message.
>
> Separately, as mentioned by Gerd and Paul, it would help if the
> patches were updated, sent as a series, and had commit messages that
> apply properly with "git am".
>
> Thanks,
> -Kevin

Thanks for review. I am sorry that I will not adopt the patch series suggestion.
These are independent changes. I think I've started a topic about my
interests to contribute to clang+llvm binary utilities+lld portability
improvement. Adding a cover letter to replicate all the stuff does not seem
very necessary.
Maybe the more important point is that I lack the experience to manage
something like PATCH {0,1,2,3,4,5,6,7,8}/8

Logically they really don't fit as a series.

I will defer the ld check cleanup to a future patch.
> > -cat - > $TMPFILE1 <<EOF
> > -const char v1[] __attribute__((section(".text.v1"))) = "0123456789";
> > -const char v2[] __attribute__((section(".text.v2"))) = "0123456789";
> > -EOF
> > -cat - > $TMPFILE1_ld <<EOF
> > -SECTIONS
> > -{
> > .mysection 0x88f0 : {
> >
> > -. = 0x10 ;
> > -*(.text.v1)
> > -. = 0x20 ;
> > -*(.text.v2)
> > -. = 0x30 ;
> > }
> >
> > -}
> > -EOF
> > -$CC -O -g -c $TMPFILE1 -o $TMPFILE1o > /dev/null 2>&1
> > -if [ $? -ne 0 ]; then
> > echo "Unable to execute the C compiler ($CC)." >&2
> > echo "" >&2
> > echo "Please install a working compiler and retry." >&2
> > echo -1
> > exit 0
> >
> > -fi
> > -$LD -T $TMPFILE1_ld $TMPFILE1o -o $TMPFILE2o > /dev/null 2>&1
> > -if [ $? -ne 0 ]; then
> > echo "The version of LD on this system ($LD) does not properly handle" >&2
> > echo "alignments.  As a result, this project can not be built." >&2
> > echo "" >&2
> > echo "The problem may be the result of this LD bug report:" >&2
> > echo " http://sourceware.org/bugzilla/show_bug.cgi?id=12726" >&2
> > echo "" >&2
> > echo "Please update to a working version of binutils and retry." >&2
> > echo -1
> > exit 0
> >
> > -fi
> >
> > # Test for "-fwhole-program".  Older versions of gcc (pre v4.1) don't
> >  # support the whole-program optimization - detect that.
> >  $CC -fwhole-program -S -o /dev/null -xc /dev/null > /dev/null 2>&1
> > @@ -87,4 +47,4 @@ echo 0
> >  # "ebp" register is clobberred in an "asm" statement.  The code has
> >  # been modified to not clobber "ebp" - no test is available yet.
> >
> > -rm -f $TMPFILE1 $TMPFILE1o $TMPFILE1_ld $TMPFILE2 $TMPFILE2o $TMPFILE3o
> > +rm -f $TMPFILE1 $TMPFILE1o $TMPFILE2 $TMPFILE2o $TMPFILE3o
> >
> > 2.25.1

From 7d3bf908066c0bae6383b03a8c22b7f0c00bd3f5 Mon Sep 17 00:00:00 2001
From: Fangrui Song 
Date: Tue, 17 Mar 2020 23:34:44 -0700
Subject: [PATCH v2] romlayout32flag.lds: Use `. +=` instead of `. =`

This improves the portability of the linker script and allows lld to link rom.o

Dot assignment inside an output section has a inconsistent behavior
which makes lld difficult to implement. See https://bugs.llvm.org/show_bug.cgi?id=43083

Signed-off-by: Fangrui Song 
---
  scripts/layoutrom.py | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py
index 6616721..505e9a1 100755
--- a/scripts/layoutrom.py
+++ b/scripts/layoutrom.py
@@ -335,14 +335,19 @@ def outRelSections(sections, startsym, useseg=0):
                  if section.finalloc is not None]
      sections.sort(key=operator.itemgetter(0))
      out = ""
+    location = "_reloc_init_end"
      for addr, section in sections:
          loc = section.finalloc
          if useseg:
              loc = section.finalsegloc
-        out += ". = ( 0x%x - %s ) ;\n" % (loc, startsym)
+        if location == "_reloc_init_end":
+            out += ". += 0x%x - %s ;\n" % (loc, location)
+        elif location < loc:
+            out += ". += 0x%x ;\n" % (loc-location,)
          if section.name in ('.rodata.str1.1', '.rodata'):
              out += "_rodata%s = . ;\n" % (section.fileid,)
          out += "*%s.*(%s)\n" % (section.fileid, section.name)
+        location = loc + section.size
      return out

# Build linker script output for a list of relocations.