[ELF] Linkerscript: support assignment outside SECTIONS

[phosek]

Summary

We only support assignments inside SECTIONS, but this does not matches the behavior of GNU linker which also allows them outside SECTIONS. The only restriction on assignments outside SECTIONS is that they cannot reference . (they have to be absolute expressions).

[ruiu]

I'd rather ignore errors and evaluate . as 0 instead of adding the second parameter to many functions.

[ruiu]

ELF/LinkerScript.cpp

661–662

} else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok, false)) {

[phosek]

In D23598#518449, @ruiu wrote:

I'd rather ignore errors and evaluate . as 0 instead of adding the second parameter to many functions.

Is the new version what you had in mind? Do you want to avoid completely ignoring errors, even if . is on the LHS? A different variant I've been also experimenting with is to change the Expr type to something like typedef std::function<uint64_t(optional<uint64_t>)> Expr; and when evaluating Dot inside getSymbolValue, throw an error when . is not available.

[ruiu]

I'd even completely ignore the error and not try to make efforts to detect it. It is what we do for . in other contexts such as PHDRs addresses and section's AT command.

ELF/LinkerScript.cpp

1072–1073

Please remove dead code.

[ruiu]

ELF/LinkerScript.h

70–72 ↗

(On Diff #68472)

Let's remove this -- as I wrote in the previous comment, this is not an error we are detecting in other contexts, and it is okay to ignore them. I don't think it's a popular error, and it is "undefined behavior" (if there were the linker script language specification.)

[phosek]

ELF/LinkerScript.h

70–72 ↗

(On Diff #68472)

I believe we still need some way to recognize whether the assignment is inside SECTIONS or not to avoid modifying . in case it's not when assigning addresses inside assignAddress()?

[ruiu]

ELF/LinkerScript.h

70–72 ↗

(On Diff #68472)

Users shouldn't write an assignment outside SECTIONS with . as the RHS, so I don't think we need to worry too much about it.

[phosek]

ELF/LinkerScript.h

70–72 ↗

(On Diff #68472)

The problem is if . is on LHS (in the assignment outside SECTIONS) because this will affect all subsequent assignments (even those within SECTIONS).

[ruiu]

ELF/LinkerScript.h

70–72 ↗

(On Diff #68472)

Users shouldn't write . on LHS too, no?

[phosek]

ELF/LinkerScript.h

70–72 ↗

(On Diff #68472)

No they shouldn't, but if they do, shall we just produce incorrect output? I guess that's probably fine since we're not reporting error anyway?

[ruiu]

ELF/LinkerScript.h

70–72 ↗

(On Diff #68472)

Please do nothing and let it just create incorrect outputs for incorrect inputs. We may want to do more fine-grained error checking in future, but at this moment, this error check would be too much compared to other error checks. There are many interesting ways to produce broken output using linker scripts, and we are not trying to stop them from shooting them in the foot.

[ruiu]

LGTM. Thanks!

ELF/LinkerScript.cpp

const StringMap<elf::ScriptParser::Handler> elf::ScriptParser::Cmd = {
     {"SECTIONS", &ScriptParser::readSections},
     {";", &ScriptParser::readNothing}};
 
 void ScriptParser::run() {
   while (!atEOF()) {
     StringRef Tok = next();
     if (Handler Fn = Cmd.lookup(Tok))
       (this->*Fn)();
+    else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok))
+      Opt.Commands.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()));
static uint64_t getHeaderSize() {
   default:
     llvm_unreachable("unsupported target");
   }
 }
 
 SymbolAssignment *ScriptParser::readAssignment(StringRef Name) {
   StringRef Op = next();
   assert(Op == "=" || Op == "+=");
   Expr E = readExpr();
   if (Op == "+=")
     E = [=](uint64_t Dot) { return getSymbolValue(Name, Dot) + E(Dot); };
   return new SymbolAssignment(Name, E);
 }
 
 // This is an operator-precedence parser to parse a linker
 // script expression.
 Expr ScriptParser::readExpr() { return readExpr1(readPrimary(), 0); }
 

test/ELF/invalid-linkerscript.test

 # RUN: mkdir -p %t.dir
 
 ## Note that we are using "cannot open no-such-file: " as a marker that the
 ## linker keep going when it found an error. That specific error message is not
 ## related to the linker script tests.
 
 # RUN: echo foobar > %t1
 # RUN: not ld.lld %t1 no-such-file 2>&1 | FileCheck -check-prefix=ERR1 %s
-# ERR1: unknown directive: foobar
+# ERR1: unexpected EOF
 # ERR1: cannot open no-such-file:
 
 # RUN: echo "foo \"bar" > %t2
 # RUN: not ld.lld %t2 no-such-file 2>&1 | FileCheck -check-prefix=ERR2 %s
 # ERR2: unclosed quote
 # ERR2: cannot open no-such-file:
 
 # RUN: echo "/*" > %t3

test/ELF/linkerscript/linkerscript-symbols.s

 # HIDDEN2: 0000000000000000         *ABS*    00000000 somesym
 
 # Hidden symbol assignment.
 # RUN: echo "SECTIONS { HIDDEN(newsym = 1);}" > %t.script
 # RUN: ld.lld -o %t1 --script %t.script %t
 # RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=HIDDEN3 %s
 # HIDDEN3: 0000000000000001         *ABS*    00000000 .hidden newsym
 
+# The symbol is not referenced. Don't provide it.
+# RUN: echo "PROVIDE(newsym = 1);" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=PROVIDE4 %s
+# PROVIDE4-NOT: 0000000000000001         *ABS*    00000000 newsym
+
+# The symbol is not referenced. Don't provide it.
+# RUN: echo "PROVIDE_HIDDEN(newsym = 1);" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=HIDDEN4 %s
+# HIDDEN4-NOT: 0000000000000001         *ABS*    00000000 .hidden newsym
+
+# Provide existing symbol. The value should be 0, even though we
+# have value of 1 in PROVIDE()
+# RUN: echo "PROVIDE(somesym = 1);" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=PROVIDE5 %s
+# PROVIDE5: 0000000000000000         *ABS*    00000000 somesym
+
+# 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
+
 .global _start
 _start:
  nop
 
 .global somesym
 somesym = 0