[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

274–278

Now this can be removed, no?

[phosek]

ELF/LinkerScript.cpp

274–278

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–271

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.h

struct PhdrsCommand {
   unsigned Type;
   bool HasFilehdr;
   bool HasPhdrs;
   unsigned Flags;
 };
 
 // ScriptConfiguration holds linker script parse results.
 struct ScriptConfiguration {
+  // Used to create symbol assignments outside SECTIONS command.
+  std::vector<std::unique_ptr<SymbolAssignment>> Assignments;
   // Used to assign addresses to sections.
   std::vector<std::unique_ptr<BaseCommand>> Commands;
 
   // Used to assign sections to headers.
   std::vector<PhdrsCommand> PhdrsCommands;
 
   bool HasContents = false;
 
   llvm::BumpPtrAllocator Alloc;
 
   // List of section patterns specified with KEEP commands. They will
   // be kept even if they are unused and --gc-sections is specified.
   std::vector<StringRef> KeptSections;
 };
 
 extern ScriptConfiguration *ScriptConfig;
 
 // This is a runner of the linker script.
 template <class ELFT> class LinkerScript {
   typedef typename ELFT::uint uintX_t;
 
 public:
   LinkerScript();
   ~LinkerScript();
+  void createAssignments();
   void createSections(OutputSectionFactory<ELFT> &Factory);
 
   std::vector<PhdrEntry<ELFT>> createPhdrs();
   bool ignoreInterpSection();
 
   ArrayRef<uint8_t> getFiller(StringRef Name);
   Expr getLma(StringRef Name);
   bool shouldKeep(InputSectionBase<ELFT> *S);

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>::createAssignments() {
+  for (const std::unique_ptr<SymbolAssignment> &Cmd : Opt.Assignments) {
+    if (shouldDefine<ELFT>(Cmd.get()))
+      addRegular<ELFT>(Cmd.get());
+    if (Cmd->Sym)
+      cast<DefinedRegular<ELFT>>(Cmd->Sym)->Value = Cmd->Expression(0);
+  }
+}
+
+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);
const StringMap<elf::ScriptParser::Handler> elf::ScriptParser::Cmd = {
     {"PHDRS", &ScriptParser::readPhdrs},
     {"SEARCH_DIR", &ScriptParser::readSearchDir},
     {"SECTIONS", &ScriptParser::readSections},
     {";", &ScriptParser::readNothing}};
 
 void ScriptParser::run() {
   while (!atEOF()) {
     StringRef Tok = next();
-    if (Handler Fn = Cmd.lookup(Tok))
+    if (Handler Fn = Cmd.lookup(Tok)) {
       (this->*Fn)();
-    else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok))
+    } else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok)) {
+      if (Opt.HasContents)
         Opt.Commands.emplace_back(Cmd);
       else
+        Opt.Assignments.emplace_back(Cmd);
+    } else {
       setError("unknown directive: " + Tok);
     }
   }
+}
 
 void ScriptParser::addFile(StringRef S) {
   if (IsUnderSysroot && S.startswith("/")) {
     SmallString<128> Path;
     (Config->Sysroot + S).toStringRef(Path);
     if (sys::fs::exists(Path)) {
       Driver->addFile(Saver.save(Path.str()));
       return;

ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::run() {
   addReservedSymbols();
 
   if (Target->NeedsThunks)
     forEachRelSec(createThunks<ELFT>);
 
   CommonInputSection<ELFT> Common(getCommonSymbols<ELFT>());
   CommonInputSection<ELFT>::X = &Common;
 
+  Script<ELFT>::X->createAssignments();
+
   Script<ELFT>::X->OutputSections = &OutputSections;
   if (ScriptConfig->HasContents)
     Script<ELFT>::X->createSections(Factory);
   else
     createSections();
 
   finalizeSections();
   if (HasError)

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