Add undefined symbols from linker script to output file

[ihalip]

Summary

According to GNU ld documentation, using the EXTERN(symbol) in a linker script has the same effect as the -u/--undefined command-line parameter.

When using such a linker script with lld, the linker doesn't add these symbols to the output file. A simple example:

$ touch a.c
$ clang -c a.c -o a.o
$ echo "EXTERN(test)" > a.lds
$ ld.lld -T a.lds a.o -o a.out
$ objdump -t a.out | grep test
$

These are not added because these symbols are not added to lld's symbol table. Driver.cpp has this code:

for (StringRef Arg : Config->Undefined)
  if (Symbol *Sym = Symtab->find(Arg))
    handleUndefined(Sym);

Here, Symtab is empty, which is why lld silently discards the symbols declared in the script.

I can see two possible fixes for this issue:

1. The one I'm proposing in this review: add the symbols to Symtab as they are read by the script parser
2. Add them to Symtab close to that piece of code in Driver.cpp.

[nickdesaulniers]

https://github.com/ClangBuiltLinux/linux/issues/515

[MaskRay]

If I read correctly, the Linux kernel use case boils down to:

ld.bfd -r a.o -u test -o b.o adds an undefined symbol test to b.o but ld.lld doesn't.

The incompatibility can be observed with the following commands (the default linker script of ld.bfd defines some other symbols but they are irrelevant to your issue):

% ld.bfd a.o -u test -o a && nm a   # or a.lds a.o
ld.bfd: warning: cannot find entry symbol _start; defaulting to 0000000000400078
0000000000601000 A __bss_start
0000000000601000 A _edata
0000000000601000 A _end
0000000000601000 n _GLOBAL_OFFSET_TABLE_
                 U _start
                 U test
% ld.lld a.o -u test -o a && nm a   # or a.lds a.o
ld.lld: warning: cannot find entry symbol _start; defaulting to 0x201000

The documentation of ld.bfd says:

'-u SYMBOL'
'--undefined=SYMBOL'
     Force SYMBOL to be entered in the output file as an undefined
     symbol.  Doing this may, for example, trigger linking of additional
     modules from standard libraries.  '-u' may be repeated with
     different option arguments to enter additional undefined symbols.
     This option is equivalent to the 'EXTERN' linker script command.

'EXTERN(SYMBOL SYMBOL ...)'
     Force SYMBOL to be entered in the output file as an undefined
     symbol.  Doing this may, for example, trigger linking of additional
     modules from standard libraries.  You may list several SYMBOLs for
     each 'EXTERN', and you may use 'EXTERN' multiple times.  This
     command has the same effect as the '-u' command-line option.

It and the behavior I observed from ld.bfd do appear to suggest -u and EXTERN() should be handled the same way. However, this patch adds an undefined symbol for EXTERN() but does not do the same for -u. I think if we want to fix the issue, we should make the two consistent.

Another thing is that:

ld.bfd -shared a.o -u test -o a.so adds test to .symtab but not .dynsym.
However, if we let Driver.cpp Config->Undefined add an undefined symbol, test will be added to both .symtab and .dynsym (the rule if a symbol should be added to .dynsym is different in lld. We can reuse ExportDynamic for Undefined if really want to fix -u). The symbol in .dynsym is definitely not desired (it can cause "undefined references" errors in a subsequent executable link). I need to think more how this issue should be tacked.

Meanwhile, can you check, in the Linux kernel, if the EXTERN(__memcat_p) can be moved from the -r link (lib/lib-ksyms.o) to a subsequent link? The combined -r -u looks a bit unusual and I am not sure if that is desired.
(I am happy with current lld semantics of -u/EXTERN() though it is different from ld.bfd.
Given that we now have --undefined-glob, changing the semantics of -u will make the behavior inconsistent with --undefined-glob)

[ihalip]

In D63564#1551417, @MaskRay wrote:

It and the behavior I observed from ld.bfd do appear to suggest -u and EXTERN() should be handled the same way. However, this patch adds an undefined symbol for EXTERN() but does not do the same for -u. I think if we want to fix the issue, we should make the two consistent.

You're right, this change only handles the EXTERN() case. It should be reworked to also handle -u/--undefined.

Meanwhile, can you check, in the Linux kernel, if the EXTERN(__memcat_p) can be moved from the -r link (lib/lib-ksyms.o) to a subsequent link? The combined -r -u looks a bit unusual and I am not sure if that is desired.

I don't think so, I didn't find any combination of command-line params to ld.lld that would make it add the symbol into the linked binary.

if we let Driver.cpp Config->Undefined add an undefined symbol, test will be added to both .symtab and .dynsym

Yes, I also see that. Looking into it...

[ruiu]

Is there any possibility that this is a GNU ld's bug? Their behavior is different from their man page, and the thing that defines what is the right behavior is usually a manual than an actual behavior.

[MaskRay]

In D63564#1551811, @ruiu wrote:

Is there any possibility that this is a GNU ld's bug? Their behavior is different from their man page, and the thing that defines what is the right behavior is usually a manual than an actual behavior.

GNU ld's behavior seems consistent: both EXTERN(foo) and -u foo force the undefined symbol foo. In lld, I think this option is only useful when the symbol exists and is a LazyArchive/LazyObject.

@ihalip To make foo appears in .symtab but not in .dynsym for ld.bfd -shared a.o -u test -o a.so will be difficult. I think another Symbol bit, like ExportDynamic, will be required to represent this.

Even if we can do that, the semantics of -u will be different from --undefined-glob. In any case, -r -u looks weird to me: if you use the partially linked object to link a shared object or an executable, why can't you delay the use of EXTERN(foo)? So I asked if this issue can be worked around in the Linux kernel.

[ihalip]

Abandoning this, it's obvious it wasn't the right fix anyway.

lld/ELF/ScriptParser.cpp

 
 void ScriptParser::readExtern() {
   expect("(");
-  while (!errorCount() && !consume(")"))
-    Config->Undefined.push_back(unquote(next()));
+  while (!errorCount() && !consume(")")) {
+    StringRef Name = unquote(next());
+    Symtab->addSymbol(
+      Undefined{nullptr, Name, STB_GLOBAL, STV_DEFAULT, 0});
+    Config->Undefined.push_back(Name);
+  }
 }
 
 void ScriptParser::readGroup() {

lld/test/ELF/linkerscript/extern.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: echo "EXTERN( test )" > %t.script
+# RUN: echo "EXTERN( "test" )" >> %t.script
+# RUN: echo "EXTERN( "test2" )" >> %t.script
+# RUN: ld.lld %t -o %t2 -T %t.script
+# RUN: llvm-readobj -t %t2 | FileCheck %s
+
+.globl _start
+_start:
+  ret
+
+## Test that the script adds the specified symbols, and that they
+## are marked as undefined.
+
+# CHECK:      Symbol {
+# CHECK:        Name: test (1)
+# CHECK-NEXT:    Value: 0x0
+# CHECK-NEXT:    Size: 0
+# CHECK-NEXT:    Binding: Global (0x1)
+# CHECK-NEXT:    Type: None (0x0)
+# CHECK-NEXT:    Other: 0
+# CHECK-NEXT:    Section: Undefined (0x0)
+# CHECK-NEXT:  }
+# CHECK-NEXT:  Symbol {
+# CHECK-NEXT:    Name: test2 (6)
+# CHECK-NEXT:    Value: 0x0
+# CHECK-NEXT:    Size: 0
+# CHECK-NEXT:    Binding: Global (0x1)
+# CHECK-NEXT:    Type: None (0x0)
+# CHECK-NEXT:    Other: 0
+# CHECK-NEXT:    Section: Undefined (0x0)
+# CHECK-NEXT:  }