[ELF] Keep st_type for symbol assignment

[MaskRay]

Summary

PR46970: for alias = aliasee, the alias can be used in relocation processing
and on ARM st_type does affect Thumb interworking. It is thus desirable for the
alias to get the same st_type.

Note that the st_size field should not be retained because some tools use
st_size=0 as a heuristic to detect aliases. Retaining st_size can thwart such
heuristics and cause aliases to be preferred over the original symbols.

[MaskRay]

Rebase after ac46bc35e98d922f1b05b451341f03dcaccd1527
Add an ARM test. Set st_type in LinkerScript::addSymbol (which runs before relocation processing) to make this affect relocation processing

[MaskRay]

Add a doc

[psmith]

Not had a chance to look into the changes in detail yet, but on first glance it looks good. Given that the compiler uses STT_FUNC for the entry of a symbol I think it is reasonable to retain the type only for an alias.

lld/docs/ELF/linker_script.rst

37 ↗

(On Diff #286717)

May be worth using "The `st_size` field is set to 0."

[grimar]

This LGTM with minor nit. Worth waiting for a final word from Peter too.

lld/ELF/LinkerScript.h

61

Deserves a comment I think. It is probably not that obvious that type is a symbol type.

Also, use STT_NOTYPE instead of 0?

[psmith]

Had a chance to look over lunch. Thanks looks good to me too.

[smeenai]

Thank you!

lld/test/ELF/linkerscript/symbol-assign-type.s

3

Typo: aliase -> alias

[MaskRay]

Fix typo

Improve doc

Test _start in symbol-assign-type.s

[MaskRay]

Add a comment

[MaskRay]

Update comment

[smeenai]

@hans, would you consider this for 11.0? 11.0 LLD will no longer perform ARM-Thumb interworking for symbols that aren't marked STT_FUNC, so this fixes cases where interworking would incorrectly not occur because of a simple alias.

[MaskRay]

In D86263#2230869, @smeenai wrote:

@hans, would you consider this for 11.0? 11.0 LLD will no longer perform ARM-Thumb interworking for symbols that aren't marked STT_FUNC, so this fixes cases where interworking would incorrectly not occur because of a simple alias.

This is not a case we intended to support before, so this is more of a feature request. The risk is tiny and can make some users happy. I am in favor of patching this into 11.0.

[hans]

The risk is tiny and can make some users happy. I am in favor of patching this into 11.0.

Sounds like a good trade-off :-)

I tried applying it, but ELF/arm-thumb-interwork-ifunc.s doesn't pass with it on the branch. I guess the expectations need rebasing. Would you be willing to do that and push it to the branch?

[MaskRay]

In D86263#2234268, @hans wrote:

The risk is tiny and can make some users happy. I am in favor of patching this into 11.0.

Sounds like a good trade-off :-)

I tried applying it, but ELF/arm-thumb-interwork-ifunc.s doesn't pass with it on the branch. I guess the expectations need rebasing. Would you be willing to do that and push it to the branch?

Done in d6d03d09e3f7498f60e2976b8cea235080f55fe7 (arm-thumb-interwork-ifunc.s needs a clean-up/fix before this patch) & c4e216711d001316df2313a85c8af73a85d804c0 (rewritten lld/test/ELF/linkerscript/symbol-assign-type.s to not depend on 'split-file' (which is not in release/11.x)).

[hans]

In D86263#2234411, @MaskRay wrote:

In D86263#2234268, @hans wrote:

The risk is tiny and can make some users happy. I am in favor of patching this into 11.0.

Sounds like a good trade-off :-)

I tried applying it, but ELF/arm-thumb-interwork-ifunc.s doesn't pass with it on the branch. I guess the expectations need rebasing. Would you be willing to do that and push it to the branch?

Done in d6d03d09e3f7498f60e2976b8cea235080f55fe7 (arm-thumb-interwork-ifunc.s needs a clean-up/fix before this patch) & c4e216711d001316df2313a85c8af73a85d804c0 (rewritten lld/test/ELF/linkerscript/symbol-assign-type.s to not depend on 'split-file' (which is not in release/11.x)).

Thanks!

lld/ELF/LinkerScript.h

struct ExprValue {
   SectionBase *sec;
 
   // True if this expression is enclosed in ABSOLUTE().
   // This flag affects the return value of getValue().
   bool forceAbsolute;
 
   uint64_t val;
   uint64_t alignment = 1;
+  uint8_t type = 0;
 
   // Original source location. Used for error messages.
   std::string loc;
 };
 
 // This represents an expression in the linker script.
 // ScriptParser::readExpr reads an expression and returns an Expr.
 // Later, we evaluate the expression by calling the function.

lld/ELF/LinkerScript.cpp

void LinkerScript::addSymbol(SymbolAssignment *cmd) {
   // However, if an expression is `x = .`, there's no way to know its
   // value at the moment.
   //
   // We want to set symbol values early if we can. This allows us to
   // use symbols as variables in linker scripts. Doing so allows us to
   // write expressions like this: `alignment = 16; . = ALIGN(., alignment)`.
   uint64_t symValue = value.sec ? 0 : value.getValue();
 
-  Defined newSym(nullptr, cmd->name, STB_GLOBAL, visibility, STT_NOTYPE,
+  Defined newSym(nullptr, cmd->name, STB_GLOBAL, visibility, value.type,
                  symValue, 0, sec);
 
   Symbol *sym = symtab->insert(cmd->name);
   sym->mergeProperties(newSym);
   sym->replace(newSym);
   cmd->sym = cast<Defined>(sym);
 }
 
void LinkerScript::assignSymbol(SymbolAssignment *cmd, bool inSec) {
   ExprValue v = cmd->expression();
   if (v.isAbsolute()) {
     cmd->sym->section = nullptr;
     cmd->sym->value = v.getValue();
   } else {
     cmd->sym->section = v.sec;
     cmd->sym->value = v.getSectionOffset();
   }
+  cmd->sym->type = v.type;
 }
 
 static std::string getFilename(InputFile *file) {
   if (!file)
     return "";
   if (file->archiveName.empty())
     return std::string(file->getName());
   return (file->archiveName + ':' + file->getName()).str();
ExprValue LinkerScript::getSymbolValue(StringRef name, const Twine &loc) {
   if (name == ".") {
     if (ctx)
       return {ctx->outSec, false, dot - ctx->outSec->addr, loc};
     error(loc + ": unable to get location counter value");
     return 0;
   }
 
   if (Symbol *sym = symtab->find(name)) {
-    if (auto *ds = dyn_cast<Defined>(sym))
-      return {ds->section, false, ds->value, loc};
+    if (auto *ds = dyn_cast<Defined>(sym)) {
+      ExprValue v{ds->section, false, ds->value, loc};
+      // Retain the original st_type, so that the alias will get the same
+      // behavior in relocation processing. Any operation will reset st_type to
+      // STT_NOTYPE.
+      v.type = ds->type;
+      return v;
+    }
     if (isa<SharedSymbol>(sym))
       if (!errorOnMissingSection)
         return {nullptr, false, 0, loc};
   }
 
   error(loc + ": symbol not found: " + name);
   return 0;
 }

lld/test/ELF/arm-thumb-interwork-ifunc.s

 // REQUIRES: arm
 // RUN: llvm-mc --triple=armv7a-linux-gnueabihf -arm-add-build-attributes -filetype=obj -o %t.o %s
 // RUN: ld.lld %t.o -o %t
 // RUN: llvm-objdump --triple=armv7a-none-linux-gnueabi -d --no-show-raw-insn %t | FileCheck %s
 
+/// A symbol assignment defined alias inherits st_type and gets the same treatment.
+// RUN: llvm-mc --triple=armv7a-linux-gnueabihf -arm-add-build-attributes -filetype=obj --defsym ALIAS=1 -o %t1.o %s
+// RUN: ld.lld --defsym foo=foo1 %t1.o -o %t1
+// RUN: llvm-objdump --triple=armv7a-none-linux-gnueabi -d --no-show-raw-insn %t | FileCheck %s
+
 /// Non-preemptible ifuncs are called via a PLT entry which is always Arm
 /// state, expect the ARM callers to go direct to the PLT entry, Thumb
 /// branches are indirected via state change thunks, the bl is changed to blx.
 
  .syntax unified
  .text
  .balign 0x1000
+.ifdef ALIAS
+ .type foo1 STT_GNU_IFUNC
+ .globl foo1
+foo1:
+.else
  .type foo STT_GNU_IFUNC
  .globl foo
 foo:
+.endif
  bx lr
 
  .section .text.1, "ax", %progbits
  .arm
  .global _start
 _start:
  b foo
  bl foo

lld/test/ELF/linkerscript/common-assign.s

 # CHECK-NEXT:     Other: 0
 # CHECK-NEXT:     Section: .bss
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: pfoo
 # CHECK-NEXT:     Value: [[FOO]]
 # CHECK-NEXT:     Size: 0
 # CHECK-NEXT:     Binding: Global
-# CHECK-NEXT:     Type: None
+# CHECK-NEXT:     Type: Object
 # CHECK-NEXT:     Other: 0
 # CHECK-NEXT:     Section: .bss
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: pbar
 # CHECK-NEXT:     Value: [[BAR]]
 # CHECK-NEXT:     Size: 0
 # CHECK-NEXT:     Binding: Global
-# CHECK-NEXT:     Type: None
+# CHECK-NEXT:     Type: Object
 # CHECK-NEXT:     Other: 0
 # CHECK-NEXT:     Section: .bss
 # CHECK-NEXT:   }
 # CHECK-NEXT: ]
 
 .comm foo,4,4
 .comm bar,4,4
 movl  $1, foo(%rip)
 movl  $2, bar(%rip)

lld/test/ELF/linkerscript/symbol-assign-type.s

+# REQUIRES: x86
+## Keep st_type for simple assignment (`aliase = aliasee`). This property is
+## desired on some targets, where symbol types can affect relocation processing
+## (e.g. Thumb interworking). However, the st_size field should not be retained
+## because some tools use st_size=0 as a heuristic to detect aliases. With any
+## operation, it can be argued that the new symbol may not be of the same type,
+## so reset st_type to STT_NOTYPE.
+
+## NOTE: GNU ld retains st_type for many operations.
+
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/main.s -o %t.o
+# RUN: ld.lld -T %t/a.lds %t.o -o %t1
+# RUN: llvm-readelf -s %t1 | FileCheck %s
+
+# CHECK:      Size Type   Bind   Vis     Ndx Name
+# CHECK:         0 FUNC   GLOBAL DEFAULT   1 retain1
+# CHECK-NEXT:    0 FUNC   GLOBAL DEFAULT   1 retain2
+# CHECK-NEXT:    0 NOTYPE GLOBAL DEFAULT   1 drop1
+# CHECK-NEXT:    0 NOTYPE GLOBAL DEFAULT ABS drop2
+# CHECK-NEXT:    0 NOTYPE GLOBAL DEFAULT ABS drop3
+
+# RUN: ld.lld --defsym 'retain=_start' --defsym 'drop=_start+0' %t.o -o %t2
+# RUN: llvm-readelf -s %t2 | FileCheck %s --check-prefix=DEFSYM
+
+# DEFSYM:        0 FUNC   GLOBAL DEFAULT   1 retain
+# DEFSYM-NEXT:   0 NOTYPE GLOBAL DEFAULT   1 drop
+
+#--- a.lds
+retain1 = _start;
+retain2 = 1 ? _start : 0;
+
+## Reset to STT_NOTYPE if any operation is performed,
+## even if the operation is an identity function.
+drop1 = _start + 0;
+drop2 = 0 ? _start : 1;
+drop3 = -_start;
+
+#--- main.s
+.globl _start
+.type _start, @function
+_start:
+  ret
+.size _start, 1