[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 ↗

(On Diff #112365)

Why false?

705–707 ↗

(On Diff #112365)

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

[grimar]

ELF/ScriptParser.cpp

632 ↗

(On Diff #112365)

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 ↗

(On Diff #112365)

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

164–171

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

164–171

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

164–171

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

[pcc]

ELF/LTO.cpp

164–171

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

164–171

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

I would inline this function into the caller.

[ruiu]

ELF/LTO.cpp

164–171

"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

Done. Thanks for review !

164–171

Done.

[ruiu]

ELF/LTO.cpp

134

Remove llvm::.

164–171

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

IPO -> interprocedural optimization

167

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

"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

can -> would

[grimar]

Committed as r314097

ELF/LTO.cpp

 //===- LTO.cpp ------------------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "LTO.h"
 #include "Config.h"
 #include "Error.h"
+#include "LinkerScript.h"
 #include "InputFiles.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "lld/Core/TargetOptionsCommandFlags.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"

 
 BitcodeCompiler::~BitcodeCompiler() = default;
 
 static void undefine(Symbol *S) {
   replaceBody<Undefined>(S, nullptr, S->body()->getName(), /*IsLocal=*/false,
                          STV_DEFAULT, S->body()->Type);
 }
 
 void BitcodeCompiler::add(BitcodeFile &F) {
   lto::InputFile &Obj = *F.Obj;
   unsigned SymNum = 0;
   std::vector<SymbolBody *> Syms = F.getSymbols();
   std::vector<lto::SymbolResolution> Resols(Syms.size());
 
+  DenseSet<StringRef> ScriptSymbols;
+  for (BaseCommand *Base : Script->Opt.Commands)
+    if (auto *Cmd = dyn_cast<SymbolAssignment>(Base))
+      ScriptSymbols.insert(Cmd->Name);
+
   // Provide a resolution to the LTO API for each symbol.
   for (const lto::InputFile::Symbol &ObjSym : Obj.symbols()) {
     SymbolBody *B = Syms[SymNum];
     Symbol *Sym = B->symbol();
     lto::SymbolResolution &R = Resols[SymNum];
     ++SymNum;
 
     // Ideally we shouldn't check for SF_Undefined but currently IRObjectFile
for (const lto::InputFile::Symbol &ObjSym : Obj.symbols()) {
     // 2) Symbols that are used in regular objects.
     // 3) C named sections if we have corresponding __start_/__stop_ symbol.
     // 4) Symbols that are defined in bitcode files and used for dynamic linking.
     R.VisibleToRegularObj = Config->Relocatable || Sym->IsUsedInRegularObj ||
                             (R.Prevailing && Sym->includeInDynsym()) ||
                             UsedStartStop.count(ObjSym.getSectionName());
     if (R.Prevailing)
       undefine(Sym);
-    R.LinkerRedefined = Config->RenamedSymbols.count(Sym);
+
+    // We tell LTO to not apply interprocedural optimization for following
+    // symbols because otherwise LTO can inline them while their values are
+    // still not final:
+    // 1) Aliased (with --defsym) or wrapped (with --wrap) symbols.
+    // 2) Symbols redefined in linker script.
+    R.LinkerRedefined =
+        Config->RenamedSymbols.count(Sym) || ScriptSymbols.count(B->getName());
   }
   checkError(LTOObj->add(std::move(F.Obj), Resols));
 }
 
 // Merge all the bitcode files we have seen, codegen the result
 // and return the resulting ObjectFile(s).
 std::vector<InputFile *> BitcodeCompiler::compile() {
   std::vector<InputFile *> Ret;

test/ELF/lto/Inputs/linker-script-symbols-ipo.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @bar()
+
+define i32 @_start() {
+  %1 = tail call i32 @bar()
+  ret i32 %1
+}

test/ELF/lto/linker-script-symbols-assign.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
+; FIXME: Binding should be Global here. Symbols redefined in script does not
+;        receive correct binding and visibility, this is a known bug.
+; CHECK-NEXT:    Binding: Weak
+; 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

test/ELF/lto/linker-script-symbols-ipo.ll

+; REQUIRES: x86
+; RUN: llvm-as %s -o %t1.o
+; RUN: llvm-as %S/Inputs/linker-script-symbols-ipo.ll -o %t2.o
+; RUN: echo "bar = foo;" > %t.script
+
+;; Check that without linkerscript bar is inlined.
+; RUN: ld.lld  -m elf_x86_64 %t1.o %t2.o -o %t3 -save-temps
+; RUN: llvm-objdump -d %t3 | FileCheck %s --check-prefix=IPO
+; IPO:      Disassembly of section .text:
+; IPO:      _start:
+; IPO-NEXT:   201000: {{.*}} movl $1, %eax
+; IPO-NEXT:   201005: {{.*}} retq
+
+;; Check that LTO does not do IPO for symbols assigned by script.
+; RUN: ld.lld  -m elf_x86_64 %t1.o %t2.o -o %t4 --script %t.script -save-temps
+; RUN: llvm-objdump -d %t4 | FileCheck %s --check-prefix=NOIPO
+; NOIPO:      Disassembly of section .text:
+; NOIPO:      foo:
+; NOIPO-NEXT:   201010: {{.*}} movl $2, %eax
+; NOIPO:      _start:
+; NOIPO-NEXT:   201020: {{.*}} jmp -21 <foo>
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @bar() {
+  ret i32 1
+}
+
+define i32 @foo() {
+  ret i32 2
+}