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

[George Rimar via Phabricator via llvm-commits]

grimar planned changes to this revision.
grimar added inline comments.


================
Comment at: ELF/LTO.cpp:170
       undefine(Sym);
     R.LinkerRedefined = Config->RenamedSymbols.count(Sym);
   }
----------------
pcc wrote:
> 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`.
Ah, got it, thanks for explanation ! I'll try to prepare testcase and update the diff with this change.


https://reviews.llvm.org/D37059