[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

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;
 
+  // The original st_type if the expression references a symbol. Any operation
+  // resets type to STT_NOTYPE.
+  uint8_t type = llvm::ELF::STT_NOTYPE;
+
   // 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.
 using Expr = std::function<ExprValue()>;

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/docs/ELF/linker_script.rst

 implementation and have changed over time.
 
 The lld implementation policy for properties of linker scripts that are not
 defined by the documentation is to follow the GNU ld implementation wherever
 possible. We reserve the right to make different implementation choices where
 it is appropriate for LLD. Intentional deviations will be documented in this
 file.
 
+Symbol assignment
+~~~~~~~~~~~~~~~~~
+
+A symbol assignment looks like:
+
+::
+
+  symbol = expression;
+  symbol += expression;
+
+The first form defines ``symbol``. If ``symbol`` is already defined, it will be
+overridden. The other form requires ``symbol`` to be already defined.
+
+For a simple assignment like ``alias = aliasee;``, the ``st_type`` field is
+copied from the original symbol. Any arithmetic operation (e.g. ``+ 0`` will
+reset ``st_type`` to ``STT_NOTYPE``.
+
+The ``st_size`` field is set to 0.
+
 Output section description
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 The description of an output section looks like:
 
 ::
 
   section [address] [(type)] : [AT(lma)] [ALIGN(section_align)] [SUBALIGN](subsection_align)] {

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 (`alias = 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:         1 FUNC   GLOBAL DEFAULT   1 _start
+# 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