[ELF] Allow defined symbols to be assigned from linker script

[meadori]

Summary

Hi All,

I have been working on extending LLD to handle assigning new values to symbols that are already defined via the linker script (e.g. override). This is common in some scripts that have (mainly from bare-metal GCC toolchains). The attached patch mostly works, but it has some problems related to linker-defined symbols that I will describe next. I am *not* submitting this patch as a formal submission, but a request for feedback.

As mentioned, this patch mostly works for overriding a symbol's value from the linker script, but currently falls over with respect to _gp on MIPS. In particular, the test added for D27036 fails. The reason being is that _gp is defined relative to .got. Thus consider a script with something like:

_gp = 0x100;
.got  : { *(.got) }

_gp gets a value of &.got + 0x100. The *new* definition from the linker script gets defined in .got too.

FWIW, in most MIPS toolchains I have work with _gp is an absolute symbol and *not* a .got relative one and the default linker scripts have things like:

HIDDEN (_gp = ALIGN (16) + 0x7ff0);
.got            : { *(.got) }

Comments on the general symbol override functionality and the MIPS related issue are most welcome.

[ruiu]

In the first place, I wonder why you want to overwrite _gp. What is the background of your motivation?

LLD supports ADDR() funciton, so I think you can do this in a straightforward manner like this.

_gp = ADDR(.got) + 0x100;

Does it work for you?

[meadori]

I want to be able to override *any* symbol's value (which is support in GNUnLD, for example). Currently LLD issues multiple definition errors if I say something like:

foo = 1

and foo is defined in an object file or something.

The _gp thing was just a issue I ran into while implementing the general ability to redefine a symbol's value from a linker script because _gp is defined by the linker relative to .got and thus causes problems when completely overriding it from a linker script.

[atanasyan]

_gp gets a value of &.got + 0x100. The *new* definition from the linker script gets defined in .got too.

FWIW, in most MIPS toolchains I have work with _gp is an absolute symbol and *not* a .got relative one

You are right. It's an old bug in LLD and I regularly postpone to fix it. The _gp symbol should be an absolute symbol. If it is not defined in a linker script we need to assign default value .got + 0x7ff0. It looks like it's time to fix this problem.

To Rui: The _gp symbol marks the base of the small data area. Not only GOT relocations depend on _gp. Users of bare-metal toolchains might want to place small data to specific address.

[ruiu]

Got it. So that's not specific to _gp, but you are talking about the fact that LLD complains if a symbol you are trying to define is already defined in an object file.

That's just a negligence or a bug. Symbols in linker scripts should take precedence over pre-existing symbols.

[atanasyan]

FYI

I fixed the problem with _gp symbol in the D27524.

[meadori]

In D27276#615883, @atanasyan wrote:

FYI

I fixed the problem with _gp symbol in the D27524.

Nice! Thank you! I will check it out.

[ruiu]

What is the status of this patch? Is this ready for review?

[meadori]

In D27276#616083, @ruiu wrote:

What is the status of this patch? Is this ready for review?

I waiting for D27524 to go through. Once that happens I will rework the patch with better comments and tests. If you prefer, we can close this review and I will open a new one later.

[meadori]

• Added a few more comments.
• Added test cases.

[ruiu]

ELF/LinkerScript.cpp

97

What is "it" here?

114

I think this function can be simplified. We do not create a new symbol only when

• it is in a PROVIDE(), and
• the existing symbol is not an Undefined symbol.

Otherwise, we always want to create symbol and then assign a value to it. So, we can return early for the PROVIDE() case.

119–120

nit: the ternary operator doesn't look nice if it need too much indentation. Probably this is better.

if (Cmd_>Expression.IsAbsolute())
  Cmd->Sym = addRegular...;
else
  Cmd->Sym = ...;

[meadori]

Address review feedback:

• Clarify code comments.
• Simplify logic in addSymbol.

[ruiu]

ELF/LinkerScript.cpp

104–108

Use early return and assignment-in-if.

  return;
}
if (const OutputSectionBase *Sec = Cmd->Expression.Section())
  cast<....>... = ...;

123–130

I don't think you need to check if B exists. You can add a new symbol unconditionally.

[meadori]

ELF/LinkerScript.cpp

123–130

If you do it unconditionally, then duplicate symbols might be created, e.g. foo is defined in an input object *and* the linker script.

[ruiu]

ELF/LinkerScript.cpp

123–130

That shouldn't happen because of the architecture of LLD. Did you actually try that?

[meadori]

ELF/LinkerScript.cpp

123–130

I did, and several tests in the test suite fail. Consider making unconditional with the following diff against my current patch:

-  if (!B || B->isUndefined()) {
-    if (Cmd->Expression.IsAbsolute())
-      Cmd->Sym = addRegular<ELFT>(Cmd);
-    else
-      Cmd->Sym = addSynthetic<ELFT>(Cmd);
-  } else {
-    Cmd->Sym = B;
-  }
+  if (Cmd->Expression.IsAbsolute())
+    Cmd->Sym = addRegular<ELFT>(Cmd);
+  else
+    Cmd->Sym = addSynthetic<ELFT>(Cmd);

Then:

$ ./bin/llvm-nm test.o 
0000000000000001 A foo
$ cat test.ld
foo = 12;
$ ./bin/ld.lld test.o -T test.ld
./bin/ld.lld: error: duplicate symbol 'foo' in test.o and (internal)

[ruiu]

ELF/LinkerScript.cpp

123–130

Got it. So addRegular and such check for conflicts. But what we want to do here is to replace symbols unconditionally. Otherwise, this code doesn't work for some cases. For example, what if an existing symbol is a common symbol?

I think you want directly call replaceBody<> to replace symbol body unconditionally.

[atanasyan]

I'm concerned a bit about the XFAIL mips test. Do you have any estimations when this patch be ready?

[meadori]

In D27276#625391, @atanasyan wrote:

I'm concerned a bit about the XFAIL mips test. Do you have any estimations when this patch be ready?

I will have a new patch out this week. Sorry for the delay, I hit another bug when implementing Rui's latest feedback.

[atanasyan]

In D27276#626221, @meadori wrote:

In D27276#625391, @atanasyan wrote:

I'm concerned a bit about the XFAIL mips test. Do you have any estimations when this patch be ready?

I will have a new patch out this week. Sorry for the delay, I hit another bug when implementing Rui's latest feedback.

No problem. Thanks for the information.

[meadori]

Apologies for the delay on this one. @ruiu, if I understand correctly you are suggesting for the unconditional replacement of the symbols in the symbol table. That make sense to me. The latest patch adds a "force create" option to the appropriate symbol table methods. Is that okay?

With that, I can get the desired behavior for replacing symbols *however* there is still one test failing: lld :: ELF/mips-gp-ext.s. The reason this fails is that the expression _gp = . + 0x100; gets put in the wrong section. In particular, it gets put in .text instead of *ABS*. This section assignment problem is due to another bug in lld that needs to be invested.

In suggestion on a way forward? Fixing the aforementioned section assignment problem doesn't seem entirely straight forward.

[ruiu]

ELF/SymbolTable.h

63–68 ↗

(On Diff #82948)

This is towards a right direction, but you don't need to modify the symbol table. The symbol table is to map names to symbols, but because you already have a symbol object in this case, symbol table lookups are not necessary. Please use replaceBody() to overwrite existing symbols with new ones.

[meadori]

ELF/SymbolTable.h

63–68 ↗

(On Diff #82948)

Oh, I missed that you can get the Symbol object from the SymbolBody object. I get it now. Thanks.

[meadori]

• Address @ruiu's point about using replaceBody.
• Add XFAIL back to test/ELF/mips-gp-ext.s -- this test fails now due to another lld bug. I will look into the other bug after pushing this through.

[ruiu]

ELF/LinkerScript.cpp

65–66

It is probably easier to add a dummy symbol and then replace that unconditionally like this.

Symbol *Sym = Symtab<ELFT>::X->addUndefined(Cmd->Name);
replaceBody<DefinedRegular<ELFT>>(Sym, Cmd->Name, ...);
return Sym->body();

You can remove Symbol *Sym from the parameter list of this function.

73

Ditto. You don't need to pass Sym.

[meadori]

ELF/LinkerScript.cpp

65–66

Much better. Thanks!

[meadori]

• Address @ruiu's point about using addUndefined. Note that I had to use the long form to control the visibility (for symbols defined in PROVIDE).
• XFAIL of test/ELF/mips-gp-ext.s remains -- this test fails now due to another lld bug. I will look into the other bug after pushing this through.

[ruiu]

LGTM

ELF/LinkerScript.cpp

82

nit: remove this blank line so that it matches with the style in the above function.

[meadori]

Committed. Thanks for all the review @ruiu. Much appreciated.

ELF/LinkerScript.cpp

 using namespace llvm::object;
 using namespace llvm::support::endian;
 using namespace lld;
 using namespace lld::elf;
 
 LinkerScriptBase *elf::ScriptBase;
 ScriptConfiguration *elf::ScriptConfig;
 
-template <class ELFT> static void addRegular(SymbolAssignment *Cmd) {
+template <class ELFT> static SymbolBody *addRegular(SymbolAssignment *Cmd) {
   uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
-  Symbol *Sym = Symtab<ELFT>::X->addRegular(Cmd->Name, Visibility, STT_NOTYPE,
-                                            0, 0, STB_GLOBAL, nullptr, nullptr);
-  Cmd->Sym = Sym->body();
-
-  // If we have no SECTIONS then we don't have '.' and don't call
-  // assignAddresses(). We calculate symbol value immediately in this case.
-  if (!ScriptConfig->HasSections)
-    cast<DefinedRegular<ELFT>>(Cmd->Sym)->Value = Cmd->Expression(0);
+  Symbol *Sym = Symtab<ELFT>::X->addUndefined(
+      Cmd->Name, /*IsLocal=*/false, STB_GLOBAL, Visibility,
+      /*Type*/ 0,
+      /*CanOmitFromDynSym*/ false, /*File*/ nullptr);
+
+  replaceBody<DefinedRegular<ELFT>>(Sym, Cmd->Name, /*IsLocal=*/false,
+                                    Visibility, STT_NOTYPE, 0, 0, nullptr,
+                                    nullptr);
+  return Sym->body();
 }
 
-template <class ELFT> static void addSynthetic(SymbolAssignment *Cmd) {
-  // If we have SECTIONS block then output sections haven't been created yet.
+template <class ELFT> static SymbolBody *addSynthetic(SymbolAssignment *Cmd) {
+  uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
   const OutputSectionBase *Sec =
       ScriptConfig->HasSections ? nullptr : Cmd->Expression.Section();
-  Symbol *Sym = Symtab<ELFT>::X->addSynthetic(
-      Cmd->Name, Sec, 0, Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT);
-  Cmd->Sym = Sym->body();
+  Symbol *Sym = Symtab<ELFT>::X->addUndefined(
+      Cmd->Name, /*IsLocal=*/false, STB_GLOBAL, Visibility,
+      /*Type*/ 0,
+      /*CanOmitFromDynSym*/ false, /*File*/ nullptr);
 
-  // If we already know section then we can calculate symbol value immediately.
-  if (Sec)
-    cast<DefinedSynthetic>(Cmd->Sym)->Value = Cmd->Expression(0) - Sec->Addr;
+  replaceBody<DefinedSynthetic>(Sym, Cmd->Name, 0, Sec);
+
+  return Sym->body();
 }
 
 static bool isUnderSysroot(StringRef Path) {
   if (Config->Sysroot == "")
     return false;
   for (; !Path.empty(); Path = sys::path::parent_path(Path))
     if (sys::fs::equivalent(Config->Sysroot, Path))
       return true;
   return false;
 }
 
-template <class ELFT> static void addSymbol(SymbolAssignment *Cmd) {
-  if (Cmd->Expression.IsAbsolute())
-    addRegular<ELFT>(Cmd);
-  else
-    addSynthetic<ELFT>(Cmd);
+template <class ELFT> static void assignSymbol(SymbolAssignment *Cmd) {
+  // If there are sections, then let the value be assigned later in
+  // `assignAddresses`.
+  if (ScriptConfig->HasSections)
+    return;
+
+  uint64_t Value = Cmd->Expression(0);
+  if (Cmd->Expression.IsAbsolute()) {
+    cast<DefinedRegular<ELFT>>(Cmd->Sym)->Value = Value;
+  } else {
+    const OutputSectionBase *Sec = Cmd->Expression.Section();
+    if (Sec)
+      cast<DefinedSynthetic>(Cmd->Sym)->Value = Value - Sec->Addr;
   }
-// If a symbol was in PROVIDE(), we need to define it only when
-// it is an undefined symbol.
-template <class ELFT> static bool shouldDefine(SymbolAssignment *Cmd) {
+}
+
+template <class ELFT> static void addSymbol(SymbolAssignment *Cmd) {
   if (Cmd->Name == ".")
-    return false;
-  if (!Cmd->Provide)
-    return true;
+    return;
+
+  // If a symbol was in PROVIDE(), we need to define it only when
+  // it is a referenced undefined symbol.
   SymbolBody *B = Symtab<ELFT>::X->find(Cmd->Name);
-  return B && B->isUndefined();
+  if (Cmd->Provide && (!B || B->isDefined()))
+    return;
+
+  // Otherwise, create a new symbol if one does not exist or an
+  // undefined one does exist.
+  if (Cmd->Expression.IsAbsolute())
+    Cmd->Sym = addRegular<ELFT>(Cmd);
+  else
+    Cmd->Sym = addSynthetic<ELFT>(Cmd);
+  assignSymbol<ELFT>(Cmd);
 }
 
 bool SymbolAssignment::classof(const BaseCommand *C) {
   return C->Kind == AssignmentKind;
 }
 
 bool OutputSectionCommand::classof(const BaseCommand *C) {
   return C->Kind == OutputSectionKind;
 }
 
 bool InputSectionDescription::classof(const BaseCommand *C) {

 template <class ELFT>
 void LinkerScript<ELFT>::processCommands(OutputSectionFactory<ELFT> &Factory) {
   for (unsigned I = 0; I < Opt.Commands.size(); ++I) {
     auto Iter = Opt.Commands.begin() + I;
     const std::unique_ptr<BaseCommand> &Base1 = *Iter;
 
     // Handle symbol assignments outside of any output section.
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Base1.get())) {
-      if (shouldDefine<ELFT>(Cmd))
-        addSymbol<ELFT>(Cmd);
       continue;
     }
 
     if (auto *Cmd = dyn_cast<AssertCommand>(Base1.get())) {
       // If we don't have SECTIONS then output sections have already been
       // created by Writer<ELFT>. The LinkerScript<ELFT>::assignAddresses
       // will not be called, so ASSERT should be evaluated now.
       if (!Opt.HasSections)
if (auto *Cmd = dyn_cast<OutputSectionCommand>(Base1.get())) {
         --I;
         continue;
       }
 
       // A directive may contain symbol definitions like this:
       // ".foo : { ...; bar = .; }". Handle them.
       for (const std::unique_ptr<BaseCommand> &Base : Cmd->Commands)
         if (auto *OutCmd = dyn_cast<SymbolAssignment>(Base.get()))
-          if (shouldDefine<ELFT>(OutCmd))
-            addSymbol<ELFT>(OutCmd);
 
       // Handle subalign (e.g. ".foo : SUBALIGN(32) { ... }"). If subalign
       // is given, input sections are aligned to that value, whether the
       // given value is larger or smaller than the original section alignment.
       if (Cmd->SubalignExpr) {
         uint32_t Subalign = Cmd->SubalignExpr(0);
         for (InputSectionBase<ELFT> *S : V)
           S->Alignment = Subalign;

test/ELF/linkerscript/symbol-assignexpr.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 
 # RUN: echo "SECTIONS { \
 # RUN:         symbol = CONSTANT(MAXPAGESIZE); \
 # RUN:         symbol2 = symbol + 0x1234; \
 # RUN:         symbol3 = symbol2; \
 # RUN:         symbol4 = symbol + -4; \
 # RUN:         symbol5 = symbol - ~ 0xfffb; \
 # RUN:         symbol6 = symbol - ~(0xfff0 + 0xb); \
 # RUN:         symbol7 = symbol - ~ 0xfffb + 4; \
 # RUN:         symbol8 = ~ 0xffff + 4; \
 # RUN:         symbol9 = - 4; \
 # RUN:         symbol10 = 0xfedcba9876543210; \
 # RUN:         symbol11 = ((0x28000 + 0x1fff) & ~(0x1000 + -1)); \
+# RUN:         symbol12 = 0x1234; \
+# RUN:         symbol12 += 1; \
+# RUN:         bar = 0x5678; \
+# RUN:         baz = 0x9abc; \
 # RUN:       }" > %t.script
 # RUN: ld.lld -o %t1 --script %t.script %t
 # RUN: llvm-objdump -t %t1 | FileCheck %s
 
 # CHECK:      SYMBOL TABLE:
 # CHECK-NEXT: 0000000000000000 *UND* 00000000
-# CHECK-NEXT:                  .text 00000000 _start
-# CHECK-NEXT:                  .text 00000000 foo
+# CHECK-NEXT: 0000000000000000 .text 00000000 _start
+# CHECK-NEXT: 0000000000005678 *ABS* 00000000 bar
+# CHECK-NEXT: 0000000000009abc *ABS* 00000000 baz
+# CHECK-NEXT: 0000000000000001 .text 00000000 foo
 # CHECK-NEXT: 0000000000001000 *ABS* 00000000 symbol
 # CHECK-NEXT: 0000000000002234 *ABS* 00000000 symbol2
 # CHECK-NEXT: 0000000000002234 *ABS* 00000000 symbol3
 # CHECK-NEXT: 0000000000000ffc *ABS* 00000000 symbol4
 # CHECK-NEXT: 0000000000010ffc *ABS* 00000000 symbol5
 # CHECK-NEXT: 0000000000010ffc *ABS* 00000000 symbol6
 # CHECK-NEXT: 0000000000011000 *ABS* 00000000 symbol7
 # CHECK-NEXT: ffffffffffff0004 *ABS* 00000000 symbol8
 # CHECK-NEXT: fffffffffffffffc *ABS* 00000000 symbol9
 # CHECK-NEXT: fedcba9876543210 *ABS* 00000000 symbol10
 # CHECK-NEXT: 0000000000029000 *ABS* 00000000 symbol11
+# CHECK-NEXT: 0000000000001235 *ABS* 00000000 symbol12
 
 # RUN: echo "SECTIONS { \
 # RUN:         symbol2 = symbol; \
 # RUN:       }" > %t2.script
 # RUN: not ld.lld -o %t2 --script %t2.script %t 2>&1 \
 # RUN:  | FileCheck -check-prefix=ERR %s
 # ERR: {{.*}}.script:1: symbol not found: symbol
 
 .global _start
 _start:
  nop
 
 .global foo
 foo:
+ nop
+
+.global bar
+bar = 0x1234
+
+.comm baz,8,8

test/ELF/mips-gp-ext.s

 # Check that the linker use a value of _gp symbol defined
 # in a linker script to calculate GOT relocations.
 
-# FIXME: This test is xfailed because it depends on D27276 patch
-# that enables absolute symbols redefinition by a linker's script.
+# FIXME: This test is xfailed because there is currently a bug
+# that causes symbols defined by linker scripts to be put in the
+# wrong sections.  In particular, `_gp = . + 0x100` ends up in
+# `.text` when it should be in `*ABS*`.
 # XFAIL: *
 
 # RUN: llvm-mc -filetype=obj -triple=mips-unknown-linux %s -o %t.o
 
 # RUN: echo "SECTIONS { \
 # RUN:          .text : { *(.text) } \
 # RUN:          _gp = . + 0x100; \
 # RUN:          .got  : { *(.got) } }" > %t.rel.script