[ELF] - LTO: do not optimize away symbols accessed from linkerscript.

[grimar]

Summary

Previously when BC file had global variable that was accessed from script,
it was optimized away.
In this patch I add left side of assignment expression to list of symbols referenced
by script. That marks them used from regular objects for LTO and fixes issue observed.

[davide]

Out of curiosity, where did you hit this?
Is there any way to test that the assignment updates the value of foo to be 1 ?

[grimar]

In D37059#850053, @davide wrote:

Out of curiosity, where did you hit this?

It is very close to D37009 btw.

[grimar]

Looks phab ate my first reply. Duplicating again for history:

Out of curiosity, where did you hit this?

I did not hit, just basing on my knowledge about how things work, I supposed this case will not work and checked.

Is there any way to test that the assignment updates the value of foo to be 1 ?

I believe yes, I'll update the testcase.

[davide]

What happens if foo is specified in the linker script but actually doesn't appear in any bitcode file?
IIRC we should error in that case.

[grimar]

This comment was removed by grimar.

[grimar]

In D37059#850075, @davide wrote:

What happens if foo is specified in the linker script but actually doesn't appear in any bitcode file?
IIRC we should error in that case.

I think we will create absolute symbol foo for this case.
If it would be foo = bar then we would error out.

[grimar]

I was wrong, if all inputs are BC and we have foo = 1 script then I do not see it in output and link succeeds.
I'll take a look closer on this case.

[davide]

Yes, I was afraid this wasn't enough.

[grimar]

In D37059#850094, @davide wrote:

Yes, I was afraid this wasn't enough.

As far I see without this patch situation is the same. We do not create absolute symbols if have
foo = 1 script and only bitcode inputs that does not have foo symbol.
I think such case should be fixed in a different patch then.

[grimar]

• Improved testcase.

[grimar]

My appologies. Everything is fine with emiting absolute symbols.
Looks I looked into temporarily LTO object instead of final binary.

I updated testcase to show they work fine. Sorry for all noise.

[ruiu]

ELF/ScriptParser.cpp

632

Why false?

705–707

This function doesn't seem like the best place to do this. Is readAssignment better, isn't it?

[grimar]

ELF/ScriptParser.cpp

632

I need to distinguish assignments inside output section declaration and outside (global).
Because for example we have in one of testcases:

ONLY_IF_RO { abc = 1; *(foo) }

And in that case we should not create abc at all.
I decided to handle only global assignments for now, probably it can be enough.
That is why false.

705–707

Unfortunaly not, because PROVIDE/HIDDEN/PROVIDE_HIDDEN are using it too.

We should not provide symbol that was not referenced.
When we add it to Opt.ReferencedSymbols it creates undefined symbol and that works like it was referenced.
(https://github.com/llvm-mirror/lld/blob/master/ELF/LinkerScript.cpp#L148)
What breaks PROVIDE logic.

[davide]

FWIW, this is just an incarnation of a more general problem, i.e. the fact that the interaction between LTO and linker scripts is/wasn't well studied in the past.
I guess we may want to fix this case, but I'm afraid several other problems will arise.
I wonder when/where we should draw the line between supporting arbitrary interactions between the two or just suggesting people to pass -fno-lto to an handful of files in their compile step, as GCC seems to be doing.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65252
(also, cc @pcc for thoughts)

[pcc]

I feel like a reasonable position to start with would be "you can't use a linker script to talk about sections in an LTO input file, only symbols". i.e. a SECTIONS clause would not be able to refer to sections in LTO input files, but you would be able to redefine symbols (that support already exists for --wrap and --defsym of course) and define symbols in terms of other symbols defined in LTO input files.

That basically seems to be the position that GCC has taken, and doesn't seem too onerous to support in LLD.

[grimar]

• Reimplemented, taking considerations from D37208 into account.

[grimar]

• Removed excessive variable.

[grimar]

Ping.

[grimar]

Ping.

[pcc]

ELF/LTO.cpp

170 ↗

(On Diff #113070)

I think we should be setting LinkerRedefined to true for these symbols instead of VisibleToRegularObj, otherwise the optimizer may IPO past them.

[grimar]

ELF/LTO.cpp

170 ↗

(On Diff #113070)

I am not really familar with LTO logic, but if that is true, that sounds like a bug of LTO for me.
May be I am missing something, but if linker says symbols should be visible (and that means used) from regular object,
I would expect LTO should not eliminate them out.

But actually I think it does not do that:
I looked how both VisibleToRegularObj and LinkerRedefined are used. They set symbol resolution to
GlobalResolution::External:
https://github.com/llvm-mirror/llvm/blob/master/lib/LTO/LTO.cpp#L426

And that prevents making them internal here:
https://github.com/llvm-mirror/llvm/blob/master/lib/LTO/LTO.cpp#L812

After all when parsing LTO objects and symbols:
https://github.com/llvm-mirror/lld/blob/master/ELF/SymbolTable.cpp#L125
there is no bar symbol at all (because VisibleToRegularObj was not set for it,
but foo is still there). That probably shows that logic is correct.
What am I missing ?

[grimar]

ELF/LTO.cpp

170 ↗

(On Diff #113070)

When I mentioned bar and foo, I meant those from my testcase. foo is assigned in script, bar is not.

[pcc]

ELF/LTO.cpp

170 ↗

(On Diff #113070)

By "IPO past them" I meant that the optimizer may apply inlining or otherwise use the symbol's definition to optimize code that references the symbol, which is what setting LinkerRedefined prevents. That is orthogonal to whether the symbol is dropped.

For example suppose we have these two TUs:

int f() {
  return g();
}

int g() {
   return 1;
}
int h() {
   return 2;
}

and this linker script:

g = h;

If we mark g as VisibleToRegularObj, that would not prevent g from being inlined into f at LTO time, causing f to incorrectly return 1. Setting LinkerRedefined prevents g from being inlined, so f will return 2 as a result of the linker script replacing g with h.

[grimar]

ELF/LTO.cpp

170 ↗

(On Diff #113070)

Ah, got it, thanks for explanation ! I'll try to prepare testcase and update the diff with this change.

[grimar]

• Addressed review comment, added testcase for IPO.

[pcc]

LGTM

ELF/LTO.cpp

128 ↗

(On Diff #115218)

I would inline this function into the caller.

[ruiu]

ELF/LTO.cpp

168 ↗

(On Diff #115218)

"following symbols" doesn't explain what this code is for. It's basically saying that this code does something as is written. Please update the comment.

[grimar]

• Addressed comments.

ELF/LTO.cpp

128 ↗

(On Diff #115218)

Done. Thanks for review !

168 ↗

(On Diff #115218)

Done.

[ruiu]

ELF/LTO.cpp

134 ↗

(On Diff #115377)

Remove llvm::.

168 ↗

(On Diff #115218)

This commend needs to explain "why" instead of "what", as what this code does is obvious by, well, code.

[grimar]

• Addressed review comments.

[ruiu]

ELF/LTO.cpp

165 ↗

(On Diff #115612)

IPO -> interprocedural optimization

167 ↗

(On Diff #115612)

Linkerscript is not a word. It's a linker script.

2. Symbols redefined in linker scripts.

[grimar]

• Addressed review comments.

[ruiu]

ELF/LTO.cpp

165–170 ↗

(On Diff #115815)

"We have to do this because ..." part explains why we can't do IPO for both (1) and (2) symbols. So, writing it as part of (2) is not correct.

[grimar]

• Updated comment.

[ruiu]

LGTM

ELF/LTO.cpp

166 ↗

(On Diff #115987)

can -> would

[grimar]

Committed as r314097

ELF/ScriptParser.cpp

private:
   std::vector<StringRef> readOutputSectionPhdrs();
   InputSectionDescription *readInputSectionDescription(StringRef Tok);
   StringMatcher readFilePatterns();
   std::vector<SectionPattern> readInputSectionsList();
   InputSectionDescription *readInputSectionRules(StringRef FilePattern);
   unsigned readPhdrType();
   SortSectionPolicy readSortKind();
   SymbolAssignment *readProvideHidden(bool Provide, bool Hidden);
-  SymbolAssignment *readProvideOrAssignment(StringRef Tok);
+  SymbolAssignment *readProvideOrAssignment(StringRef Tok, bool Global);
   void readSort();
   AssertCommand *readAssert();
   Expr readAssertExpr();
   Expr readConstant();
   Expr getPageSize();
 
   uint64_t readMemoryAssignment(StringRef, StringRef, StringRef);
   std::pair<uint32_t, uint32_t> readMemoryAttributes();
while (!atEOF()) {
     } else if (Tok == "PHDRS") {
       readPhdrs();
     } else if (Tok == "SEARCH_DIR") {
       readSearchDir();
     } else if (Tok == "SECTIONS") {
       readSections();
     } else if (Tok == "VERSION") {
       readVersion();
-    } else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok)) {
+    } else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok, true)) {
       Script->Opt.Commands.push_back(Cmd);
     } else {
       setError("unknown directive: " + Tok);
     }
   }
 }
 
 void ScriptParser::addFile(StringRef S) {
void ScriptParser::readSections() {
   // -no-rosegment is used to avoid placing read only non-executable sections in
   // their own segment. We do the same if SECTIONS command is present in linker
   // script. See comment for computeFlags().
   Config->SingleRoRx = true;
 
   expect("{");
   while (!ErrorCount && !consume("}")) {
     StringRef Tok = next();
-    BaseCommand *Cmd = readProvideOrAssignment(Tok);
+    BaseCommand *Cmd = readProvideOrAssignment(Tok, true);
     if (!Cmd) {
       if (Tok == "ASSERT")
         Cmd = readAssert();
       else
         Cmd = readOutputSectionDescription(Tok);
     }
     Script->Opt.Commands.push_back(Cmd);
   }
OutputSection *ScriptParser::readOutputSectionDescription(StringRef OutSec) {
   if (consume("ONLY_IF_RW"))
     Cmd->Constraint = ConstraintKind::ReadWrite;
   expect("{");
 
   while (!ErrorCount && !consume("}")) {
     StringRef Tok = next();
     if (Tok == ";") {
       // Empty commands are allowed. Do nothing here.
-    } else if (SymbolAssignment *Assign = readProvideOrAssignment(Tok)) {
+    } else if (SymbolAssignment *Assign = readProvideOrAssignment(Tok, false)) {
       Cmd->Commands.push_back(Assign);
     } else if (BytesDataCommand *Data = readBytesDataCommand(Tok)) {
       Cmd->Commands.push_back(Data);
     } else if (Tok == "ASSERT") {
       Cmd->Commands.push_back(readAssert());
       expect(";");
     } else if (Tok == "CONSTRUCTORS") {
       // CONSTRUCTORS is a keyword to make the linker recognize C++ ctors/dtors
SymbolAssignment *ScriptParser::readProvideHidden(bool Provide, bool Hidden) {
   SymbolAssignment *Cmd = readAssignment(next());
   Cmd->Provide = Provide;
   Cmd->Hidden = Hidden;
   expect(")");
   expect(";");
   return Cmd;
 }
 
-SymbolAssignment *ScriptParser::readProvideOrAssignment(StringRef Tok) {
+SymbolAssignment *ScriptParser::readProvideOrAssignment(StringRef Tok,
+                                                        bool Global) {
   SymbolAssignment *Cmd = nullptr;
   if (peek() == "=" || peek() == "+=") {
     Cmd = readAssignment(Tok);
     expect(";");
-  } else if (Tok == "PROVIDE") {
+    if (Global && Tok != ".")
+      Script->Opt.ReferencedSymbols.push_back(Tok);
+    return Cmd;
+  }
+
+  if (Tok == "PROVIDE") {
     Cmd = readProvideHidden(true, false);
   } else if (Tok == "HIDDEN") {
     Cmd = readProvideHidden(false, true);
   } else if (Tok == "PROVIDE_HIDDEN") {
     Cmd = readProvideHidden(true, true);
   }
   return Cmd;
 }

test/ELF/linkerscript/align.s

 # RUN: }" > %t3.script
 # RUN: ld.lld -o %t3 --script %t3.script %t
 # RUN: llvm-objdump -section-headers %t3 | FileCheck %s
 # RUN: llvm-objdump -t %t3 | FileCheck -check-prefix SYMBOLS %s
 
 # SYMBOLS-LABEL: SYMBOL TABLE:
 # SYMBOLS-NEXT: 0000000000000000         *UND*           00000000
 # SYMBOLS-NEXT: 0000000000014008         .text           00000000 _start
-# SYMBOLS-NEXT: 0000000000010000         *ABS*           00000000 __code_base__
 # SYMBOLS-NEXT: 0000000000001000         *ABS*           00000000 VAR
+# SYMBOLS-NEXT: 0000000000010000         *ABS*           00000000 __code_base__
 # SYMBOLS-NEXT: 0000000000011000         .bbb            00000000 __start_bbb
 # SYMBOLS-NEXT: 0000000000012000         .bbb            00000000 __end_bbb
 
 ## Check that ALIGN zero do nothing and does not crash #1.
 # RUN: echo "SECTIONS { . = ALIGN(0x123, 0); .aaa : { *(.aaa) } }" > %t.script
 # RUN: ld.lld -o %t4 --script %t.script %t
 # RUN: llvm-objdump -section-headers %t4 | FileCheck %s -check-prefix=ZERO
 

test/ELF/linkerscript/symbols-synthetic.s

 # RUN:        PROVIDE_HIDDEN(_end_sec = ADDR(.text) + SIZEOF(.text)); }" > %t.script
 # RUN: ld.lld -o %t1 --eh-frame-hdr --script %t.script %t
 # RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=IN-SEC %s
 
 # SIMPLE:      0000000000000128         .foo    00000000 .hidden _end_sec
 # SIMPLE-NEXT: 0000000000000120         .foo    00000000 _begin_sec
 # SIMPLE-NEXT: 0000000000000128         *ABS*   00000000 _end_sec_abs
 # SIMPLE-NEXT: 0000000000001048         .text   00000000 _start
+# SIMPLE-NEXT: 0000000000000ee4         *ABS*   00000000 size_foo_3
 # SIMPLE-NEXT: 0000000000000120         .foo    00000000 begin_foo
 # SIMPLE-NEXT: 0000000000000128         .foo    00000000 end_foo
 # SIMPLE-NEXT: 0000000000000008         *ABS*   00000000 size_foo_1
 # SIMPLE-NEXT: 0000000000000008         *ABS*   00000000 size_foo_1_abs
 # SIMPLE-NEXT: 0000000000001000         .foo    00000000 begin_bar
 # SIMPLE-NEXT: 0000000000001004         .foo    00000000 end_bar
 # SIMPLE-NEXT: 0000000000000ee4         *ABS*   00000000 size_foo_2
-# SIMPLE-NEXT: 0000000000000ee4         *ABS*   00000000 size_foo_3
 # SIMPLE-NEXT: 0000000000001004         .eh_frame_hdr     00000000 __eh_frame_hdr_start
 # SIMPLE-NEXT: 0000000000001010         *ABS*             00000000 __eh_frame_hdr_start2
 # SIMPLE-NEXT: 0000000000001018         .eh_frame_hdr     00000000 __eh_frame_hdr_end
 # SIMPLE-NEXT: 0000000000001020         *ABS*             00000000 __eh_frame_hdr_end2
 
 # NO-SEC:       0000000000201000         .text     00000000 .hidden _begin_sec
 # NO-SEC-NEXT:  0000000000201001         .text     00000000 .hidden _end_sec
 

test/ELF/lto/linker-script-symbols2.ll

+; REQUIRES: x86
+; RUN: llvm-as %s -o %t.o
+
+; RUN: echo "foo = 1;" > %t.script
+; RUN: ld.lld -m elf_x86_64 %t.o -o %t2 --script %t.script -save-temps
+; RUN: llvm-readobj -symbols %t2.lto.o | FileCheck %s
+
+; CHECK-NOT: bar
+; CHECK:     Symbol {
+; CHECK:         Name: foo
+; CHECK-NEXT:    Value: 0x0
+; CHECK-NEXT:    Size: 4
+; CHECK-NEXT:    Binding: Global
+; CHECK-NEXT:    Type: Object
+; CHECK-NEXT:    Other: 0
+; CHECK-NEXT:    Section: .bss.foo
+; CHECK-NEXT:  }
+; CHECK-NEXT:]
+
+; RUN: llvm-readobj -symbols %t2 | FileCheck %s --check-prefix=VAL
+; VAL:       Symbol {
+; VAL:        Name: foo
+; VAL-NEXT:   Value: 0x1
+; VAL-NEXT:   Size:
+; VAL-NEXT:   Binding: Global
+; VAL-NEXT:   Type: None
+; VAL-NEXT:   Other:
+; VAL-NEXT:   Section: Absolute
+; VAL-NEXT: }
+
+; RUN: echo "zed = 1;" > %t2.script
+; RUN: ld.lld -m elf_x86_64 %t.o -o %t3 --script %t2.script
+; RUN: llvm-readobj -symbols %t3 | FileCheck %s --check-prefix=ABS
+; ABS:      Symbol {
+; ABS:        Name: zed 
+; ABS-NEXT:   Value: 0x1
+; ABS-NEXT:   Size: 0
+; ABS-NEXT:   Binding: Global
+; ABS-NEXT:   Type: None
+; ABS-NEXT:   Other: 0
+; ABS-NEXT:   Section: Absolute
+; ABS-NEXT: }
+
+target triple = "x86_64-unknown-linux-gnu"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+@foo = global i32 0
+@bar = global i32 0