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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: ELF/LTO.cpp:170
       undefine(Sym);
     R.LinkerRedefined = Config->RenamedSymbols.count(Sym);
   }
----------------
grimar wrote:
> grimar wrote:
> > pcc wrote:
> > > I think we should be setting `LinkerRedefined` to true for these symbols instead of `VisibleToRegularObj`, otherwise the optimizer may IPO past them.
> > 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 ?
> > 
> When I mentioned `bar` and `foo`, I meant those from my testcase. `foo` is assigned in script, `bar` is not.
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`.


https://reviews.llvm.org/D37059