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

Fri Feb 5 08:48:21 PST 2021

tejohnson added inline comments.

================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:294
+        // is read-only it will be internalized thus become not available for
+        // linking.
+        // FIXME: Instead of checking for linkage type, use prevailing symbol
----------------
Can you add that this only affects interposable symbols since they are converted to declarations.

================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:295
+        // linking.
+        // FIXME: Instead of checking for linkage type, use prevailing symbol
+        // information and ensure the prevailing copy is read-only before allow
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95943