[PATCH] D42107: [ThinLTO] - Stop internalizing and drop non-prevailing symbols.

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Jan 27 01:44:39 PST 2018


grimar added inline comments.


================
Comment at: lib/LTO/LTO.cpp:430
+    // if symbol is defined in module level inline asm block.
+    if (GlobalRes.IRName.empty() || Res.Prevailing)
+      GlobalRes.IRName = Sym.getIRName();
----------------
tejohnson wrote:
> This assumes that the non-prevailing asm symbol with the name is seen here before the prevailing symbol without a name (which does always seem to currently be the case). Otherwise, the non-prevailing symbol would come along and fill in the name since it will be empty. Suggest moving this block up above the previous one that sets the Prevailing flag, then add an assert that if !Sym.getIRName.empty() that Res.Prevailing is not already true. I.e. that we don't overwrite the IRName when we already set it from the Prevailing copy.
Yes, currently in a single module asm symbols looks always placed before IR ones seems. 
But unfortunately asserting would also be not enough for case when we have multiple modules. 

If we have 2 modules, where
* Module A has both asm and IR symbol `foo`:
   1) Prevailing, IRName="" (asm)
   2) Non-prevailing, IRName="foo" (IR)
* Module B with non-prevailing, IRName="foo".

it would not work, because handing of resolution from module B would trigger assertion failture.
I extended LLD testcase to catch this case too in rL323584. 

Seems what we can do is never update IRName if symbol is already known to be prevailing (so always prefer IR name of prevailing symbol). 
Will update patch with such change.



https://reviews.llvm.org/D42107





More information about the llvm-commits mailing list