[PATCH] D30738: Don't internalize llvm GV's with InternalizeLinkedSymbols

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: clang/include/clang/CodeGen/CodeGenAction.h:40
+    /// If true, we use LLVM module internalizer.
+    bool Internalize;
+
----------------
tejohnson wrote:
> Something I completely missed while reviewing D30792 - I don't see how this is ever getting set. It looks like this line would need to be modified:
> 
> (From http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CodeGenAction.cpp#819)
> 
>       LinkModules.push_back(
>           {std::move(ModuleOrErr.get()), F.PropagateAttrs, F.LinkFlags});
> 
> That's where the values from BitcodeFileToLink, which has the new Internalize flag being set CompilerInvocation.cpp, should be used to initialize the new LinkModule object. It looks like there are tests in tools/clang/CodeGenCUDA/ that are checking to see if internalization happened properly and should fail.
> 
> Ah - because of the field ordering, I think the LinkFlags value is essentially being applied to the Internalize flag, so it may be getting lucky on that front. But then I would think the LinkFlags field would be uninitialized, and the desired flag of llvm::Linker::Flags::LinkOnlyNeeded not being set in the CUDA case. Can you look at why test/CodeGenCUDA/link-device-bitcode.cu, which appears to test for both internalization and the only needed linking, isn't failing? If the test is not sufficient, please augment  it so that it is failing due to this problem.
Thanks! I remember going over this line and telling myself "don't forget to add the new field here" but apparently I still forgot. The test wasn't triggered because I didn't have the NVPTX target enabled.  With all target enabled it failed, as expected.


Repository:
  rL LLVM

https://reviews.llvm.org/D30738