[PATCH] D55423: [LLD][ELF] - A fix for "linker script assignment loses relative nature of section" bug.

[George Rimar via Phabricator via llvm-commits]

grimar created this revision.
grimar added a reviewer: ruiu.
Herald added subscribers: arichardson, emaste.
Herald added a reviewer: espindola.

This is https://bugs.llvm.org//show_bug.cgi?id=39857.
I added the comment with much more details to the bug page,
the short version is below.

The following script and code demonstrates the issue:

  aliasto__text = __text;
   SECTIONS {
    .text 0x1000 : { __text = . ; *(.text) }
   }

  call aliasto__text at PLT

LLD fails with "cannot refer to absolute symbol: aliasto__text" error.
It happens because at the moment of scanning the relocations
we do not yet assign the correct/final/**any** section value for the symbol `aliasto__text`.
I made a change to Writer.cpp to workaround that, but it is not enough to fix the PR.

Another change is used to set the correct symbol section value.
Without that change, `aliasto__text` would be assigned to the first section
(`.dynamic` in our case). Seems a fine way is to do an additional iteration,
and that is what this patch does.

This also fixed one more bug we had (see the change in addr-zero.test, it was broken).


https://reviews.llvm.org/D55423

Files:
  ELF/Relocations.cpp
  ELF/Writer.cpp
  test/ELF/linkerscript/addr-zero.test
  test/ELF/linkerscript/symbol-alias-relocation.s


Index: test/ELF/linkerscript/symbol-alias-relocation.s
===================================================================
--- test/ELF/linkerscript/symbol-alias-relocation.s
+++ test/ELF/linkerscript/symbol-alias-relocation.s
@@ -0,0 +1,36 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "aliasto__text = __text; SECTIONS { .text 0x1000 : { __text = . ; *(.text) } }" > %t.script
+# RUN: ld.lld -pie -o %t --script %t.script %t.o
+# RUN: llvm-readobj -symbols %t | FileCheck %s
+
+## Check that alias 'aliasto__text' has the correct value.
+## (It should belong to the section .text and point to it's start).
+
+# CHECK:      Symbol {
+# CHECK:        Name: __text
+# CHECK-NEXT:   Value: 0x1000
+# CHECK-NEXT:   Size: 0
+# CHECK-NEXT:   Binding: Global
+# CHECK-NEXT:   Type: None
+# CHECK-NEXT:   Other: 0
+# CHECK-NEXT:   Section: .text
+# CHECK-NEXT: }
+
+# CHECK:      Symbol {
+# CHECK:        Name: aliasto__text
+# CHECK-NEXT:   Value: 0x1000
+# CHECK-NEXT:   Size: 0
+# CHECK-NEXT:   Binding: Global
+# CHECK-NEXT:   Type: None
+# CHECK-NEXT:   Other: 0
+# CHECK-NEXT:   Section: .text
+# CHECK-NEXT: }
+
+.text
+.globl _start
+.type _start, %function
+_start:
+.globl aliasto__text
+   call __text at PLT
+   call aliasto__text at PLT
Index: test/ELF/linkerscript/addr-zero.test
===================================================================
--- test/ELF/linkerscript/addr-zero.test
+++ test/ELF/linkerscript/addr-zero.test
@@ -8,7 +8,7 @@
 
 # CHECK:      Symbol {
 # CHECK:        Name: foo
-# CHECK-NEXT:   Value: 0x70
+# CHECK-NEXT:   Value: 0x0
 # CHECK-NEXT:   Size: 0
 # CHECK-NEXT:   Binding: Global
 # CHECK-NEXT:   Type: None
Index: ELF/Writer.cpp
===================================================================
--- ELF/Writer.cpp
+++ ELF/Writer.cpp
@@ -480,6 +480,20 @@
 
   Script->assignAddresses();
 
+  // Linker script may contain aliases, for example:
+  // "alias = foo; SECTIONS { .text 0x1000 : { foo = .; *(.text) } }"
+  // Since we process symbol assignment commands one by one, for that script we
+  // end up with a situation when alias symbol gets the section value of the
+  // symbol `foo` too early, right before 'foo' gets the .text section value by
+  // itself. To resolve this we call `assignAddresses` once again. It is safe
+  // because `assignAddresses` is designed to run multiple times to support the
+  // thunks creation. That would not resolve all the possible circular
+  // dependencies, but we do not need that. GNU linkers disagree with the
+  // behavior here. BFD resolves all dependencies, and gold seems to resolve
+  // only a part, seems it do something the same we do here.
+  if (Script->HasSectionsCommand)
+    Script->assignAddresses();
+
   // If -compressed-debug-sections is specified, we need to compress
   // .debug_* sections. Do it right now because it changes the size of
   // output sections.
Index: ELF/Relocations.cpp
===================================================================
--- ELF/Relocations.cpp
+++ ELF/Relocations.cpp
@@ -382,6 +382,11 @@
   if (E == R_SIZE)
     return true;
 
+  // We set the final symbols values for linker script defined symbols later.
+  // They always can be computed as a link time constant.
+  if (Sym.ScriptDefined)
+    return true;
+
   // For the target and the relocation, we want to know if they are
   // absolute or relative.
   bool AbsVal = isAbsoluteValue(Sym);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55423.177160.patch
Type: text/x-patch
Size: 3454 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181207/3b0aad39/attachment.bin>