[LLD][ELF] - Fix the different behavior of the linker script symbols on different platforms.

[grimar]

Summary

This should help make D55423 be even more simple and also
fixes the broken behavior shown in one of our test cases
for some targets, like x86-64.

The issue occurs when the forward declarations are used in the script.
One of the samples is:

SECTIONS {
  foo = ADDR(.text) - ABSOLUTE(ADDR(.text));
};

In that case, we have a broken output when output target does
not use thunks. That happens because thunks creating code
calls Script->assignAddresses() at least once one more time,
what fixups the values.

In this patch, I generalize and rename maybeAddThunks to be used
by all targets.

[peter.smith]

Overall I'm happy with this change as I think it is simpler than adding another call to Writer<ELFT>::run(). The one remaining thought is whether Writer<ELFT>::run() can be moved into finalizeAddressDependentContent() as there shouldn't be anything after that function that changes address. Could we move the remaining Writer<ELFT>::run() to the end of finalizeAddressDependentContent() ?

[grimar]

In D55550#1326818, @peter.smith wrote:

Overall I'm happy with this change as I think it is simpler than adding another call to Writer<ELFT>::run(). The one remaining thought is whether Writer<ELFT>::run() can be moved into finalizeAddressDependentContent() as there shouldn't be anything after that function that changes address. Could we move the remaining Writer<ELFT>::run() to the end of finalizeAddressDependentContent() ?

I am not sure honestly. I like that we do not spread and have all the finalizeSynthetic in one place (in one method) and I
like that finalizeAddressDependentContent is an isolated piece where the iteration magic happens.
I would probably try to avoid adding the additional code into it. This patch only removes the code from there
and I think it is good.

But If we decide to do something like this, I would suggest doing it in a different patch.

[grimar]

I think this should be landed because at least it simplifies the code. Ping.

[MaskRay]

Ping

[peter.smith]

As mentioned above, I'm in favour of making this change.

[grimar]

Rui, I am a bit tired of uncertainty in some LLD patches like this one. Could you please either confirm you're not going to accept this, so I'll abandon it. Or if we can proceed with that somehow, please say your word.

[peter.smith]

In D55550#1466482, @grimar wrote:

Rui, I am a bit tired of uncertainty in some LLD patches like this one. Could you please either confirm you're not going to accept this, so I'll abandon it. Or if we can proceed with that somehow, please say your word.

Somewhat off-topic; I'm wondering if it is worth revisiting this in a wider context to see if we can find a better way of resolving these compatibility problems. At EuroLLVM there was an interesting discussion in the binutils BOF/Round-table on how important compatibility with GNU tools was. The general consensus was that it was particularly important in projects that had to support both sets of tools and that rewriting inputs to work around limitations in one or other of the tools was a significant headache. The overall philosophy to be taken forward was for GNU compatible option names and output wherever possible, i.e. unless the GNU behaviour was impossible to implement or was crazy anyway. I think that it would be a reasonable to come up with a similar position for LLD, perhaps an RFC to LLVM-DEV might be a way forward.

With the linux kernel attempting to use LLD, first with AArch64 and Arm as part of Android, but there are people trying with x86_64 as well, simultaneously there is an increased interest from embedded systems as LLD is naturally part of the Clang/LLVM binutils. I think we are so close to be able to support these projects, but we'll have a long tail of small inconsistencies, like this one that we should fix. It would be good to come up with a common process/design to fix these that didn't get stuck on reviews.

[ruiu]

LGTM

Changes to the linker script processor oftentimes has subtle implications and in order to review such change I have to stop and think. But for this patch I took too much time. Apologies for the delay.

Somewhat off-topic; I'm wondering if it is worth revisiting this in a wider context to see if we can find a better way of resolving these compatibility problems. At EuroLLVM there was an interesting discussion in the binutils BOF/Round-table on how important compatibility with GNU tools was. The general consensus was that it was particularly important in projects that had to support both sets of tools and that rewriting inputs to work around limitations in one or other of the tools was a significant headache. The overall philosophy to be taken forward was for GNU compatible option names and output wherever possible, i.e. unless the GNU behaviour was impossible to implement or was crazy anyway. I think that it would be a reasonable to come up with a similar position for LLD, perhaps an RFC to LLVM-DEV might be a way forward.

I think that's a fair argument and we should take that stance; as long as it's not impossible to implement nor too crazy, we should make lld compatible with GNU bfd linker whenever possible at least in terms of linker script compatibility. I don't personally like the linker script and perhaps the ELF file format itself (I dislike the fact that segments and sections are orthogonal in the ELF format), but in many cases a linker script just does its job, and without that some kind of task is not doable. In practice, GNU compatibility is very important.

One of the reasons why it is not easy to review linker script code change is because it's complicated. I don't feel I fully understand the existing code of the linker script processor. It grows organically, and in many cases a small issue can be resolved by adding another call of assignOffsets(). It would for sure fixes a problem, but it is sometimes hard to say whether that's correct or not. Each stage doesn't seem clearly separated, and it is sometimes not clear whether doing something in some stage is the right thing or not. Is this due to the nature of the complexity of the linker script? I don't think so -- we didn't understand the linker script well when we started implementing it a few years ago, and the way how we organized code in the few years was still there. But now we understand it well including corner cases. I want to sit down and think hard about how to (re)organize the code. However, that idea seems the cause of slow review of linker script patches (I always started thinking how this should really be organized). I'll defer the idea of refactoring and let me review linker script changes more casually. I'm sorry about the delay, again.

ELF/Writer.cpp

1503 ↗

(On Diff #177672)

Can you mention that for x86 this loop iterates only once?

[grimar]

Thanks!

lld/trunk/ELF/Writer.cpp

public:
   void run();
 
 private:
   void copyLocalSymbols();
   void addSectionSymbols();
   void forEachRelSec(llvm::function_ref<void(InputSectionBase &)> Fn);
   void sortSections();
   void resolveShfLinkOrder();
-  void maybeAddThunks();
+  void finalizeAddressDependentContent();
   void sortInputSections();
   void finalizeSections();
   void checkExecuteOnly();
   void setReservedSymbolSections();
 
   std::vector<PhdrEntry *> createPhdrs();
   void removeEmptyPTLoad();
   void addPhdrForSection(std::vector<PhdrEntry *> &Phdrs, unsigned ShType,
for (OutputSection *Sec : OutputSections) {
 
     std::stable_sort(Sections.begin(), Sections.end(), compareByFilePosition);
 
     for (int I = 0, N = Sections.size(); I < N; ++I)
       *ScriptSections[I] = Sections[I];
   }
 }
 
-// For most RISC ISAs, we need to generate content that depends on the address
-// of InputSections. For example some architectures such as AArch64 use small
-// displacements for jump instructions that is the linker's responsibility for
-// creating range extension thunks for. As the generation of the content may
-// also alter InputSection addresses we must converge to a fixed point.
-template <class ELFT> void Writer<ELFT>::maybeAddThunks() {
-  if (!Target->NeedsThunks && !Config->AndroidPackDynRelocs &&
-      !Config->RelrPackDynRelocs)
-    return;
-
+// We need to generate and finalize the content that depends on the address of
+// InputSections. As the generation of the content may also alter InputSection
+// addresses we must converge to a fixed point. We do that here. See the comment
+// in Writer<ELFT>::finalizeSections().
+template <class ELFT> void Writer<ELFT>::finalizeAddressDependentContent() {
   ThunkCreator TC;
   AArch64Err843419Patcher A64P;
 
+  // For some targets, like x86, this loop iterates only once.
   for (;;) {
     bool Changed = false;
 
     Script->assignAddresses();
 
     if (Target->NeedsThunks)
       Changed |= TC.createThunks(OutputSections);
 
template <class ELFT> void Writer<ELFT>::finalizeSections() {
 
   if (!Script->HasSectionsCommand && !Config->Relocatable)
     fixSectionAlignments();
 
   // SHFLinkOrder processing must be processed after relative section placements are
   // known but before addresses are allocated.
   resolveShfLinkOrder();
 
-  // Jump instructions in many ISAs have small displacements, and therefore they
-  // cannot jump to arbitrary addresses in memory. For example, RISC-V JAL
-  // instruction can target only +-1 MiB from PC. It is a linker's
-  // responsibility to create and insert small pieces of code between sections
-  // to extend the ranges if jump targets are out of range. Such code pieces are
-  // called "thunks".
-  //
-  // We add thunks at this stage. We couldn't do this before this point because
-  // this is the earliest point where we know sizes of sections and their
-  // layouts (that are needed to determine if jump targets are in range).
-  maybeAddThunks();
+  // This is used to:
+  // 1) Create "thunks":
+  //    Jump instructions in many ISAs have small displacements, and therefore
+  //    they cannot jump to arbitrary addresses in memory. For example, RISC-V
+  //    JAL instruction can target only +-1 MiB from PC. It is a linker's
+  //    responsibility to create and insert small pieces of code between
+  //    sections to extend the ranges if jump targets are out of range. Such
+  //    code pieces are called "thunks".
+  //
+  //    We add thunks at this stage. We couldn't do this before this point
+  //    because this is the earliest point where we know sizes of sections and
+  //    their layouts (that are needed to determine if jump targets are in
+  //    range).
+  //
+  // 2) Update the sections. We need to generate content that depends on the
+  //    address of InputSections. For example, MIPS GOT section content or
+  //    android packed relocations sections content.
+  //
+  // 3) Assign the final values for the linker script symbols. Linker scripts
+  //    sometimes using forward symbol declarations. We want to set the correct
+  //    values. They also might change after adding the thunks.
+  finalizeAddressDependentContent();
 
-  // maybeAddThunks may have added local symbols to the static symbol table.
+  // finalizeAddressDependentContent may have added local symbols to the static symbol table.
   finalizeSynthetic(In.SymTab);
   finalizeSynthetic(In.PPC64LongBranchTarget);
 
   // Fill other section headers. The dynamic table is finalized
   // at the end because some tags like RELSZ depend on result
   // of finalizing other sections.
   for (OutputSection *Sec : OutputSections)
     Sec->finalize();

lld/trunk/test/ELF/linkerscript/addr-zero.test

-# REQUIRES: x86
+# REQUIRES: x86, aarch64
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux /dev/null -o %t.o
 # RUN: ld.lld -o %t.so --script %s %t.o -shared
 # RUN: llvm-readobj --symbols %t.so | FileCheck %s
 
-# Test that the script creates a non absolute symbol with value
-# 0 I.E., a symbol that refers to the load address.
+## Test that the script creates a non absolute symbol with value
+## 0 I.E., a symbol that refers to the load address.
 
 # 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
 # CHECK-NEXT:   Other: 0
 # CHECK-NEXT:   Section: .text
 # CHECK-NEXT: }
 
+## Because of a bug we had a different behavior (different symbol 'foo' value)
+## on a platforms that might use thunks, like AArch64. Check that issue is fixed.
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnux /dev/null -o %t.o
+# RUN: ld.lld -o %t.so --script %s %t.o -shared
+# RUN: llvm-readobj --symbols %t.so | FileCheck %s
+
 SECTIONS {
   foo = ADDR(.text) - ABSOLUTE(ADDR(.text));
 };