[PATCH] D95943: [ThinLTO] Fix import of a global variable to modules contain its weak def

[Kristina Bessonova via Phabricator via llvm-commits]

krisb added a comment.

Thanks, @tejohnson, for reviewing this!
I'll make the comment more accurate.

> I think we would still want to check linkage type, since it is only interposable that are converted to declarations. The prevailing information would be useful to refine the extra importing further. Hmm, actually I think there might be a correctness issue with this fix as is. I notice in your test below that the imported symbol becomes internalized. I believe this will need to stay interposable. Normally interposable symbols are blocked from inlining and other IPO since they can be overridden at link time. By making the imported copy internalized, it can now be inlined and no longer overridden at link time. If we did ensure that we imported the prevailing copy this might be ok. However, there are going to be cases where that is not possible (e.g. doing ThinLTO on a shared library link, or if the prevailing copy is in an ELF object/shared library and therefore not available for importing). So I think the best thing here is to ensure that when it gets imported it retains the weak linkage type. I'm not sure if is being internalized in this particular test case because the imported copy is read only - can you confirm where/how that is happening?

The imported copy becomes internal because the source def is read-only indeed (the imported copy inherits 'thinlto-internalize' attribute from the source one thus it got internalized in internalizeGVsAfterImport()). This seems okay since access from an ELF object/shared library prevents the source copy to be marked ReadOnly and at that time we already have attributes propagated.
In general, allow importing here we let it follow the normal importing rules as if the destination module doesn't contain a def at all. So that we will not import another copy with interposable linkage (it doesn't pass the check from canImportGlobalVar()). For other cases, the final linkage type will be whatever FunctionImportGlobalProcessing::getLinkage() returns for the source copy. Unless I'm missing something, this seems to be safe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95943/new/

https://reviews.llvm.org/D95943