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

[Kevin O'Connor]

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
> -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
> _______________________________________________
> SeaBIOS mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]