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 <mask...@google.com>
---
 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 <mask...@google.com>
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 <mask...@google.com>
---
 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.
--
2.25.1.481.gfbce0eb801-goog
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

Reply via email to