[ELF] Linkerscript: define symbols outside SECTIONS

[phosek]

Summary

Symbol assignments outside of SECTIONS command need to be created even when SECTIONS command is not present.

[ruiu]

ELF/LinkerScript.cpp

264–268

Now this can be removed, no?

[phosek]

ELF/LinkerScript.cpp

264–268

Currently, there are two vectors that may contain assignments, Assignments and Command so we need both loops. I was thinking about using just one, but that doesn't seem to work since we need to iterate over assignments done within SECTIONS in assignAddresses in the correct order because of the ..

[ruiu]

LGTM

ELF/LinkerScript.cpp

261

Please add a comment saying that this function handles symbol definitions outside SECTIONS command.

[phosek]

The previous revision was still not correct, it'd break in the following case: foo = 0x100; SECTIONS { bar = foo; } baz = bar; when baz would trigger an error for bar being undefined. The new revision seems to be working fine in all cases. The only problem I see with the this revision is the naming which is a little confusing now. One way to improve it would be to rename HasContents to HasSections and possibly also split createSections() into createSymbols() and createSections() so they can be called separately. Would that make sense?

[ruiu]

There is at most one SECTIONS command, so an alternative approach would be to split SymbolAssignment list into two lists, say, pre-assignments and post-asignments. I feel like it may be slightly better approach. What do you think?

[grimar]

In D23751#523869, @ruiu wrote:

There is at most one SECTIONS command...

btw that is not true :( linkerscript-sections.s contains case for multiple SECTIONS command. I also was wonder why do we support this wierd case.

[phosek]

I tested the support for multiple SECTIONS commands in Binutils ld and gold using the following script:

foo = 0x1000;
SECTIONS {
  bar = foo + 0x10;
}
baz = bar + 0x20;
SECTIONS {
  qux = baz + 0x30;
}
quux = qux + 0x40

ld handles this correctly producing:

0000000000001000     0 NOTYPE  GLOBAL DEFAULT  ABS foo
0000000000001010     0 NOTYPE  GLOBAL DEFAULT  ABS bar
0000000000001030     0 NOTYPE  GLOBAL DEFAULT  ABS baz
0000000000001060     0 NOTYPE  GLOBAL DEFAULT  ABS qux
00000000000010a0     0 NOTYPE  GLOBAL DEFAULT  ABS quux

gold produces a different output which doesn't seem correct:

0000000000001000     0 NOTYPE  GLOBAL DEFAULT  ABS foo
0000000000001010     0 NOTYPE  GLOBAL DEFAULT  ABS bar
0000000000001030     0 NOTYPE  GLOBAL DEFAULT  ABS baz
0000000000000050     0 NOTYPE  GLOBAL DEFAULT  ABS qux
0000000000000090     0 NOTYPE  GLOBAL DEFAULT  ABS quux

So given that, maybe support for multiple SECTIONS commands is not necessary. Old ld manual even explicitly says that "You may use at most one SECTIONS command in a script file", although I couldn't find the same statement in the current one.

[phosek]

Do you have any opinion on what to do about this? I'd say that since we already support having multiple SECTIONS commands and it works reasonable (as in the same as in BFD ld), there's no reason to break that functionality in which case the current patch is probably a better albeit a slightly uglier solution? I still feel feel that renaming HasContents to HasSections would improve the readability.

[grimar]

In D23751#525490, @phosek wrote:

Do you have any opinion on what to do about this? I'd say that since we already support having multiple SECTIONS commands and it works reasonable (as in the same as in BFD ld), there's no reason to break that functionality in which case the current patch is probably a better albeit a slightly uglier solution? I still feel feel that renaming HasContents to HasSections would improve the readability.

Question is addressed to Rui, but
FWIW I think multiple 'SECTIONS' is a bit confusing and uncommon. I think I never saw that in real projects though reviewed many scripts in the wild. Anybody really needs it ?
So if breaking support can help to improve the code I would probably do that.

I agree that 'HasSections' is sounds more readable because currently HasContent actually used only to check if we have SECTIONS command or not.

[ruiu]

I think I agree with George. Have you ever seen a linker script containing multiple SECTIONS?

[phosek]

Personally no, I haven't so I suppose removing this support shouldn't break anyone. Shall I split the SymbolAssignment list into the pre-assignments and post-asignments lists as you suggested then? I can also report an error when we find more than one SECTIONS command.

[ruiu]

Yup, I think splitting it into pre- and post-assignment makes things easier to understand. What do you think?

[phosek]

Turned out we don't even need post-assignment as a separate list, those can be added directly to Commands and they'll be handled correctly by the existing logic, it's just the pre-assignment ones that need to be handled separately (to cover the case where there is no SECTIONS command).

[ruiu]

LGTM

ELF/LinkerScript.cpp

if (Cmd->SortInner)
       std::stable_sort(V.begin(), V.end(), getComparator<ELFT>(Cmd->SortInner));
     if (Cmd->SortOuter)
       std::stable_sort(V.begin(), V.end(), getComparator<ELFT>(Cmd->SortOuter));
     Ret.insert(Ret.end(), V.begin(), V.end());
   }
   return Ret;
 }
 
 template <class ELFT>
 void LinkerScript<ELFT>::createSections(OutputSectionFactory<ELFT> &Factory) {
   for (const std::unique_ptr<BaseCommand> &Base1 : Opt.Commands) {
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Base1.get())) {
       if (shouldDefine<ELFT>(Cmd))
         addRegular<ELFT>(Cmd);
       continue;
     }
 
     if (auto *Cmd = dyn_cast<OutputSectionCommand>(Base1.get())) {
       if (Cmd->Name == "/DISCARD/") {
         discard(*Cmd);
         continue;
       }
 
       std::vector<InputSectionBase<ELFT> *> V = createInputSectionList(*Cmd);
if (auto *Cmd = dyn_cast<OutputSectionCommand>(Base1.get())) {
         if (Subalign)
           Sec->Alignment = Subalign;
         if (!Sec->OutSec)
           OutSec->addSection(Sec);
       }
     }
   }
 
+  if (Opt.HasContents) {
     // Add orphan sections.
     for (const std::unique_ptr<ObjectFile<ELFT>> &F :
          Symtab<ELFT>::X->getObjectFiles()) {
       for (InputSectionBase<ELFT> *S : F->getSections()) {
         if (isDiscarded(S) || S->OutSec)
           continue;
         OutputSectionBase<ELFT> *OutSec;
         bool IsNew;
         std::tie(OutSec, IsNew) = Factory.create(S, getOutputSectionName(S));
         if (IsNew)
           OutputSections->push_back(OutSec);
         OutSec->addSection(S);
       }
     }
   }
+}
 
 template <class ELFT> void assignOffsets(OutputSectionBase<ELFT> *Sec) {
   auto *OutSec = dyn_cast<OutputSection<ELFT>>(Sec);
   if (!OutSec) {
     Sec->assignOffsets();
     return;
   }
 

 }
 
 template <class ELFT> void LinkerScript<ELFT>::assignAddresses() {
   // Orphan sections are sections present in the input files which
   // are not explicitly placed into the output file by the linker script.
   // We place orphan sections at end of file.
   // Other linkers places them using some heuristics as described in
   // https://sourceware.org/binutils/docs/ld/Orphan-Sections.html#Orphan-Sections.
+  if (Opt.HasContents) {
     for (OutputSectionBase<ELFT> *Sec : *OutputSections) {
       StringRef Name = Sec->getName();
       if (getSectionIndex(Name) == INT_MAX)
         Opt.Commands.push_back(llvm::make_unique<OutputSectionCommand>(Name));
     }
+  }
 
   // Assign addresses as instructed by linker script SECTIONS sub-commands.
   Dot = getHeaderSize();
   uintX_t MinVA = std::numeric_limits<uintX_t>::max();
   uintX_t ThreadBssOffset = 0;
 
   for (const std::unique_ptr<BaseCommand> &Base : Opt.Commands) {
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Base.get())) {
for (const std::unique_ptr<BaseCommand> &Base : Opt.Commands) {
 
     Dot = alignTo(Dot, Sec->getAlignment());
     Sec->setVA(Dot);
     assignOffsets(Sec);
     MinVA = std::min(MinVA, Dot);
     Dot += Sec->getSize();
   }
 
+  if (Opt.HasContents) {
     // ELF and Program headers need to be right before the first section in
     // memory. Set their addresses accordingly.
     MinVA = alignDown(MinVA - Out<ELFT>::ElfHeader->getSize() -
                           Out<ELFT>::ProgramHeaders->getSize(),
                       Target->PageSize);
     Out<ELFT>::ElfHeader->setVA(MinVA);
     Out<ELFT>::ProgramHeaders->setVA(Out<ELFT>::ElfHeader->getSize() + MinVA);
   }
+}
 
 // Creates program headers as instructed by PHDRS linker script command.
 template <class ELFT>
 std::vector<PhdrEntry<ELFT>> LinkerScript<ELFT>::createPhdrs() {
   std::vector<PhdrEntry<ELFT>> Ret;
 
   // Process PHDRS and FILEHDR keywords because they are not
   // real output sections and cannot be added in the following loop.

ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::run() {
 
   if (Target->NeedsThunks)
     forEachRelSec(createThunks<ELFT>);
 
   CommonInputSection<ELFT> Common(getCommonSymbols<ELFT>());
   CommonInputSection<ELFT>::X = &Common;
 
   Script<ELFT>::X->OutputSections = &OutputSections;
-  if (ScriptConfig->HasContents)
-    Script<ELFT>::X->createSections(Factory);
-  else
+  if (!ScriptConfig->HasContents)
     createSections();
 
   finalizeSections();
   if (HasError)
     return;
 
   if (Config->Relocatable) {
     assignFileOffsets();
   } else {
     Phdrs = Script<ELFT>::X->hasPhdrsCommands() ? Script<ELFT>::X->createPhdrs()
                                                 : createPhdrs();
     fixHeaders();
-    if (ScriptConfig->HasContents) {
-      Script<ELFT>::X->assignAddresses();
-    } else {
+    if (!ScriptConfig->HasContents) {
       fixSectionAlignments();
       assignAddresses();
     }
     assignFileOffsets();
     setPhdrs();
     fixAbsoluteSymbols();
   }
 

test/ELF/linkerscript/linkerscript-symbols.s

 
 # Provide existing symbol. The value should be 0, even though we
 # have value of 1 in PROVIDE_HIDDEN(). Visibility should not change
 # RUN: echo "PROVIDE_HIDDEN(somesym = 1);" > %t.script
 # RUN: ld.lld -o %t1 --script %t.script %t
 # RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=HIDDEN5 %s
 # HIDDEN5: 0000000000000000         *ABS*    00000000 somesym
 
+# Simple symbol assignment. All three symbols should have the
+# same value.
+# RUN: echo "foo = 0x100; SECTIONS { bar = foo; } baz = bar;" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=SIMPLE2 %s
+# SIMPLE2: 0000000000000100         *ABS*    00000000 foo
+# SIMPLE2: 0000000000000100         *ABS*    00000000 bar
+# SIMPLE2: 0000000000000100         *ABS*    00000000 baz
+
 .global _start
 _start:
  nop
 
 .global somesym
 somesym = 0