Don't internalize llvm GV's with InternalizeLinkedSymbols

[JDevlieghere]

Summary

Passing llvm::Linker::Flags::InternalizeLinkedSymbols to the IR linker causes the linked symbols to be internalized, including the global variables llvm.global_ctors, llvm.global_dtors, llvm.used and llvm.compiler.used. This results in the module not being valid anymore. In particular it triggers the assertion "invalid linkage for intrinsic global variable" which checks that these GVs either don't have an initializer or have appending linkage.

[tejohnson]

Test case?

I'm not familiar with when the InternalizeLinkedSymbols is passed to the module linker. A quick search shows that it is typically passed for GPU code. What is the use case?

[tejohnson]

Can the users of this instead run llvm::internalizeModule on the linked module? Note that this will invoke InternalizePass::internalizeModule, which in turn already has handling for these special values (see the AlwaysPreserved inserts. This routine also adds things in the llvm.used set to the AlwaysPreserved set, which is the correct thing to do, and is missing from the code in the ModuleLinker that is force internalizing everything currently.

[JDevlieghere]

In D30738#695504, @tejohnson wrote:

Can the users of this instead run llvm::internalizeModule on the linked module? Note that this will invoke InternalizePass::internalizeModule, which in turn already has handling for these special values (see the AlwaysPreserved inserts. This routine also adds things in the llvm.used set to the AlwaysPreserved set, which is the correct thing to do, and is missing from the code in the ModuleLinker that is force internalizing everything currently.

Won't the pass internalize the whole module rather than only the linked values? Would it make sense to extend the InternalizePass::internalizeModule API to take a list of used global variables and call it from the module linker to take care of the internalization?

I'll add a test if we move forward with the change. My use case is not GPU related, rather something LTO like where we have bitcode modules with clashing function/symbol names but different implementations. The problem is that d we don't know which those will be before linking, so we want to internalize them.

[tejohnson]

In D30738#695541, @JDevlieghere wrote:

In D30738#695504, @tejohnson wrote:

Can the users of this instead run llvm::internalizeModule on the linked module? Note that this will invoke InternalizePass::internalizeModule, which in turn already has handling for these special values (see the AlwaysPreserved inserts. This routine also adds things in the llvm.used set to the AlwaysPreserved set, which is the correct thing to do, and is missing from the code in the ModuleLinker that is force internalizing everything currently.

Won't the pass internalize the whole module rather than only the linked values? Would it make sense to extend the InternalizePass::internalizeModule API to take a list of used global variables and call it from the module linker to take care of the internalization?

llvm::internalizeModule takes a callback that can be used to indicate that a GV should not be internalized (MustPreserveGV), so presumably you could use that. I think the right way to do this is from the ModuleLinker clients, not from in the ModuleLinker itself (i.e. remove the InternalizeLinkedSymbols flag and related handling). In fact, you would end up with a circular dependence calling this from the ModuleLinker, since internalizeModule is in Transforms/IPO which has a dependence on Linker.

I'll add a test if we move forward with the change. My use case is not GPU related, rather something LTO like where we have bitcode modules with clashing function/symbol names but different implementations. The problem is that d we don't know which those will be before linking, so we want to internalize them.

So once they are internalized, the clashing symbols are renamed automatically as they are linked in, I assume? If they are external to start with, how would the linking work in non-LTO mode?

[mehdi_amini]

I can see how this feature can be useful, but as Teresa mentioned it seems easier from a client point of view to:

1. Collect the list of symbol in the current module in a Set
2. Call the IRLinker
3. Invoke internalizeModule using the Set from 1) as "preservedGlobals".

That said the IRLinker could do this itself, so that llvm::Linker::Flags::InternalizeLinkedSymbols would still be exposed, if is wasn't for the circular dependency mentioned above. We could solve the dependency "easily" by injection: changing the IRLinker to take a callback to perform internalization for stage3. That would allow the client to pass in a pointer to internalizeModule that the IRlinker could use during 3) above.

[JDevlieghere]

@tejohnson The symbols are local, so it doesn't cause an issue with regular linking. It only happens when we throw all the modules together.

@mehdi_amini: Is this what you had in mind?

I've run the tests and this implementation doesn't break any existing tests regarding linking with the internalize flag.

[tejohnson]

In D30738#696733, @JDevlieghere wrote:

@tejohnson The symbols are local, so it doesn't cause an issue with regular linking. It only happens when we throw all the modules together.

I'm missing something - if the symbols are already local, why is internalization needed? A test case would help.

But this approach looks much better, thanks!

@mehdi_amini: Is this what you had in mind?

I've run the tests and this implementation doesn't break any existing tests regarding linking with the internalize flag.

There's a user in clang (under OPT_mlink_cuda_bitcode), I would think it would lose the internalization without providing a callback. Maybe there is no test for this case?

lib/Linker/LinkModules.cpp

110 ↗

(On Diff #91192)

Think the default param should be "{}" for consistency with linkInModules and with check below for a non-null InternalizeCallback.

554 ↗

(On Diff #91192)

Presumably the flag can go away, it is subsumed by the presence of the callback. There are a couple of uses in clang itself that will need to change to pass in a callback.

tools/llvm-link/llvm-link.cpp

44 ↗

(On Diff #91192)

Don't clang format the whole file with your patch. You can commit a separate patch with just the clang formating, or just clang format your changes.

[JDevlieghere]

Thanks for the feedback!

In D30738#696758, @tejohnson wrote:

I'm missing something - if the symbols are already local, why is internalization needed? A test case would help.

I have a test case on my work computer, I'll add it tomorrow.

But this approach looks much better, thanks!

@mehdi_amini: Is this what you had in mind?

I've run the tests and this implementation doesn't break any existing tests regarding linking with the internalize flag.

There's a user in clang (under OPT_mlink_cuda_bitcode), I would think it would lose the internalization without providing a callback. Maybe there is no test for this case?

That's my bad, I only ran the LLVM tests. I've removed the flag and updated the code in clang (D30792), which is now also passing all tests. I made sure there was an actual test for this by having the callback always return false, which caused some to fail.

[tejohnson]

Thanks, a couple more questions.

tools/llvm-link/llvm-link.cpp

326 ↗

(On Diff #91218)

Why is this needed since linkInModule has a default for the InternalizeCallback parameter?

329 ↗

(On Diff #91218)

Previously the internalize flag was set before invoking this function from main(). With this being set after the first linkInModule it seems like a behavior change, or am I missing something?

[JDevlieghere]

tools/llvm-link/llvm-link.cpp

326 ↗

(On Diff #91218)

I'm sorry, I don't understand the question. The function is called with only two parameters so the default argument is used for the callback. No callback means no internalization, so thats what we want in the else branch? Am I overlooking something?

329 ↗

(On Diff #91218)

Indeed, however on line 275 it is cleared on the first iteration by AND'ing it with overrideFromSrc flag.

[mehdi_amini]

tools/llvm-link/llvm-link.cpp

99 ↗

(On Diff #91192)

Please don't mix these formatting change.

Did you run clang-format on the full file to have these? Usually git clang-format will format only the part of the code you change.

319 ↗

(On Diff #91192)

Usually we avoid using a Pass outside of the PassManager, can you just call internalizeModule with your callback?

Also any reason you're taking the StringSet by value instead of const ref?

[tejohnson]

tools/llvm-link/llvm-link.cpp

326 ↗

(On Diff #91218)

You are right, nevermind this comment! Looked at it too fast.

[tejohnson]

tools/llvm-link/llvm-link.cpp

329 ↗

(On Diff #91218)

Ah, that's what I was missing. Please add a comment about why this isn't being applied to the first iteration.

[pcc]

Is the callback really necessary? I don't see anything fundamentally wrong with the first version of the patch.

[tejohnson]

In D30738#697024, @pcc wrote:

Is the callback really necessary? I don't see anything fundamentally wrong with the first version of the patch.

I don't think we should be duplicating logic that is already available elsewhere.

[pcc]

The fix can be simplified to

if (P.first().startswith("llvm."))
  continue;

That is two lines of code. Even if we simplified the internalize pass in the same way, we should not add this much complexity just to avoid duplicating two lines.

[mehdi_amini]

In D30738#697051, @pcc wrote:

The fix can be simplified to

if (P.first().startswith("llvm."))
  continue;

I wouldn't want to see this, this is still duplicating logic. I'd be OK with a helper function`legalToInternalize(GlobalValue &)` that would factor out the logic to decide this.

[tejohnson]

In D30738#697051, @pcc wrote:

The fix can be simplified to

if (P.first().startswith("llvm."))
  continue;

That is two lines of code. Even if we simplified the internalize pass in the same way, we should not add this much complexity just to avoid duplicating two lines.

Sure that is simple from a # lines standpoint, but the internalizer already had all the right logic, so we wouldn't have had this issue if we used internalizeModule to start with. My original suggestion was to move it out of here completely and have the clients invoke internalizeModule directly if they want to internalize, since I'm not sure the module linker should be in the business of doing internalization.

[pcc]

In D30738#697081, @mehdi_amini wrote:

In D30738#697051, @pcc wrote:

The fix can be simplified to

if (P.first().startswith("llvm."))
  continue;

I wouldn't want to see this, this is still duplicating logic. I'd be OK with a helper function`legalToInternalize(GlobalValue &)` that would factor out the logic to decide this.

Works for me, I guess.

In D30738#697082, @tejohnson wrote:

In D30738#697051, @pcc wrote:

The fix can be simplified to

if (P.first().startswith("llvm."))
  continue;

That is two lines of code. Even if we simplified the internalize pass in the same way, we should not add this much complexity just to avoid duplicating two lines.

Sure that is simple from a # lines standpoint, but the internalizer already had all the right logic, so we wouldn't have had this issue if we used internalizeModule to start with. My original suggestion was to move it out of here completely and have the clients invoke internalizeModule directly if they want to internalize, since I'm not sure the module linker should be in the business of doing internalization.

I'm not sure about that either. We may want to do something like what you propose, but it seems orthogonal to fixing the bug.

[JDevlieghere]

• Feedback from Teresa and Mehdi

Regarding Peter's comment: While I like the idea of the legalToInternalize, I'm not convinced that it's really better than the callback. Most of the code in InternalizePass is concerned with deciding which GVs are legal to be internalized. Performing the actual internalization is only a small part, so why not keep a unified interface like it is today? Either way you'll need info from the linker about what symbols to internalize, and the callback nicely combines (1) indicating whether it is necessary and (2) performing it.

PS: I'm working on a test case.

[tejohnson]

In D30738#697456, @JDevlieghere wrote:

• Feedback from Teresa and Mehdi

Regarding Peter's comment: While I like the idea of the legalToInternalize, I'm not convinced that it's really better than the callback. Most of the code in InternalizePass is concerned with deciding which GVs are legal to be internalized. Performing the actual internalization is only a small part, so why not keep a unified interface like it is today? Either way you'll need info from the linker about what symbols to internalize, and the callback nicely combines (1) indicating whether it is necessary and (2) performing it.

I remembered the other reason I wanted the internalizer to do this - if we have a llvm.used, then likely there is a GV in its aggregate that shouldn't be internalized. The internalizer will handle this appropriately. It also can't be (efficiently) encapsulated in a per-GV "legalToInternalize" helper.

I was thinking more last night about just having the clients call internalizeModule() after module linking (directly, not from a callback) , but the issue becomes that the symbols that were previously in the combined module shouldn't be internalized. So you either need a mechanism for extracting the final value of ValuesToLink from the module linker, and only internalize those, or you need to build a set of defined values in the combined module before calling linkInModule, and use it to disallow internalization of those symbols. I think the callback approach is better, since the module linker has the best knowledge of *which* GVs should be candidates for internalization (aside from the legality of internalization, which is what is being fixed by using internalizeModule).

PS: I'm working on a test case.

Please include one that has a GV listed in a llvm.used or llvm.compiler.used, and make sure it isn't internalized.

Will take a look at the new version of the patch in a little bit. Thanks!

[tejohnson]

Code looks good, just a few nits about comments left.

include/llvm/Linker/Linker.h

45 ↗

(On Diff #91297)

Document new parameter

lib/Linker/LinkModules.cpp

40 ↗

(On Diff #91297)

Document these members (doxygen-style "///")

tools/llvm-link/llvm-link.cpp

276 ↗

(On Diff #91297)

s/falgs/flags/

[JDevlieghere]

• Added test
• Feedback from Teresa
• Unified diff as suggested by Mehdi in D30792

[tejohnson]

clang/include/clang/CodeGen/CodeGenAction.h

40 ↗

(On Diff #91525)

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.

llvm/test/Linker/link-flags.ll

25 ↗

(On Diff #91525)

Suggest making the type of the new @foo different, so it is clear which one is internalized vs not.

[JDevlieghere]

Addressed code review comments from @tejohnson. Thanks again for reviewing!

clang/include/clang/CodeGen/CodeGenAction.h

40 ↗

(On Diff #91525)

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.

[tejohnson]

LGTM. Thanks!

cfe/trunk/include/clang/CodeGen/CodeGenAction.h

struct LinkModule {
     /// The module to link in.
     std::unique_ptr<llvm::Module> Module;
 
     /// If true, we set attributes on Module's functions according to our
     /// CodeGenOptions and LangOptions, as though we were generating the
     /// function ourselves.
     bool PropagateAttrs;
 
+    /// If true, we use LLVM module internalizer.
+    bool Internalize;
+
     /// Bitwise combination of llvm::LinkerFlags used when we link the module.
     unsigned LinkFlags;
   };
 
   unsigned Act;
   std::unique_ptr<llvm::Module> TheModule;
 
   /// Bitcode modules to link in to our module.

cfe/trunk/include/clang/Frontend/CodeGenOptions.h

public:
 
   struct BitcodeFileToLink {
     /// The filename of the bitcode file to link in.
     std::string Filename;
     /// If true, we set attributes functions in the bitcode library according to
     /// our CodeGenOptions, much as we set attrs on functions that we generate
     /// ourselves.
     bool PropagateAttrs = false;
+    /// If true, we use LLVM module internalizer.
+    bool Internalize = false;
     /// Bitwise combination of llvm::Linker::Flags, passed to the LLVM linker.
     unsigned LinkFlags = 0;
   };
 
   /// The files specified here are linked in to the module before optimizations.
   std::vector<BitcodeFileToLink> LinkBitcodeFiles;
 
   /// The user provided name for the "main file", if non-empty. This is useful

cfe/trunk/lib/CodeGen/CodeGenAction.cpp

 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
+#include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/Linker/Linker.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Support/YAMLTraits.h"
+#include "llvm/Transforms/IPO/Internalize.h"
+
 #include <memory>
 using namespace clang;
 using namespace llvm;
 
 namespace clang {
   class BackendConsumer : public ASTConsumer {
     using LinkModule = CodeGenAction::LinkModule;
 
public:
     // Links each entry in LinkModules into our module.  Returns true on error.
     bool LinkInModules() {
       for (auto &LM : LinkModules) {
         if (LM.PropagateAttrs)
           for (Function &F : *LM.Module)
             Gen->CGM().AddDefaultFnAttrs(F);
 
         CurLinkModule = LM.Module.get();
-        if (Linker::linkModules(*getModule(), std::move(LM.Module),
-                                LM.LinkFlags))
+
+        bool Err;
+        if (LM.Internalize) {
+          Err = Linker::linkModules(
+              *getModule(), std::move(LM.Module), LM.LinkFlags,
+              [](llvm::Module &M, const llvm::StringSet<> &GVS) {
+                internalizeModule(M, [&M, &GVS](const llvm::GlobalValue &GV) {
+                  return !GV.hasName() || (GVS.count(GV.getName()) == 0);
+                });
+              });
+        } else {
+          Err = Linker::linkModules(*getModule(), std::move(LM.Module),
+                                    LM.LinkFlags);
+        }
+
+        if (Err)
           return true;
       }
       return false; // success
     }
 
     void HandleTranslationUnit(ASTContext &C) override {
       {
         PrettyStackTraceString CrashInfo("Per-file LLVM IR generation");
IRGenFinished = true;
     OptimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationBase &D);
     void OptimizationRemarkHandler(
         const llvm::OptimizationRemarkAnalysisFPCommute &D);
     void OptimizationRemarkHandler(
         const llvm::OptimizationRemarkAnalysisAliasing &D);
     void OptimizationFailureHandler(
         const llvm::DiagnosticInfoOptimizationFailure &D);
   };
-  
   void BackendConsumer::anchor() {}
 }
 
 /// ConvertBackendLocation - Convert a location in a temporary llvm::SourceMgr
 /// buffer to be a valid FullSourceLoc.
 static FullSourceLoc ConvertBackendLocation(const llvm::SMDiagnostic &D,
                                             SourceManager &CSM) {
   // Get both the clang and llvm source managers.  The location is relative to
case llvm::SourceMgr::DK_Note:
     DiagID = diag::note_fe_inline_asm;
     break;
   }
   // If this problem has clang-level source location information, report the
   // issue in the source with a note showing the instantiated
   // code.
   if (LocCookie.isValid()) {
     Diags.Report(LocCookie, DiagID).AddString(Message);
-    
     if (D.getLoc().isValid()) {
       DiagnosticBuilder B = Diags.Report(Loc, diag::note_fe_inline_asm_here);
       // Convert the SMDiagnostic ranges into SourceRange and attach them
       // to the diagnostic.
       for (const std::pair<unsigned, unsigned> &Range : D.getRanges()) {
         unsigned Column = D.getColumnNo();
         B << SourceRange(Loc.getLocWithOffset(Range.first - Column),
                          Loc.getLocWithOffset(Range.second - Column));
       }
     }
     return;
   }
-  
   // Otherwise, report the backend issue as occurring in the generated .s file.
   // If Loc is invalid, we still need to report the issue, it just gets no
   // location info.
   Diags.Report(Loc, DiagID).AddString(Message);
 }
 
 #define ComputeDiagID(Severity, GroupName, DiagID)                             \
   do {                                                                         \
for (const CodeGenOptions::BitcodeFileToLink &F :
       if (!ModuleOrErr) {
         handleAllErrors(ModuleOrErr.takeError(), [&](ErrorInfoBase &EIB) {
           CI.getDiagnostics().Report(diag::err_cannot_open_file)
               << F.Filename << EIB.message();
         });
         LinkModules.clear();
         return nullptr;
       }
-      LinkModules.push_back(
-          {std::move(ModuleOrErr.get()), F.PropagateAttrs, F.LinkFlags});
+      LinkModules.push_back({std::move(ModuleOrErr.get()), F.PropagateAttrs,
+                             F.Internalize, F.LinkFlags});
     }
 
   CoverageSourceInfo *CoverageInfo = nullptr;
   // Add the preprocessor callback only when the coverage mapping is generated.
   if (CI.getCodeGenOpts().CoverageMapping) {
     CoverageInfo = new CoverageSourceInfo;
     CI.getPreprocessor().addPPCallbacks(
                                     std::unique_ptr<PPCallbacks>(CoverageInfo));

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.EmitOpenCLArgMetadata = Args.hasArg(OPT_cl_kernel_arg_info);
   Opts.CompressDebugSections = Args.hasArg(OPT_compress_debug_sections);
   Opts.RelaxELFRelocations = Args.hasArg(OPT_mrelax_relocations);
   Opts.DebugCompilationDir = Args.getLastArgValue(OPT_fdebug_compilation_dir);
   for (auto A : Args.filtered(OPT_mlink_bitcode_file, OPT_mlink_cuda_bitcode)) {
     CodeGenOptions::BitcodeFileToLink F;
     F.Filename = A->getValue();
     if (A->getOption().matches(OPT_mlink_cuda_bitcode)) {
-      F.LinkFlags = llvm::Linker::Flags::LinkOnlyNeeded |
-                    llvm::Linker::Flags::InternalizeLinkedSymbols;
+      F.LinkFlags = llvm::Linker::Flags::LinkOnlyNeeded;
       // When linking CUDA bitcode, propagate function attributes so that
       // e.g. libdevice gets fast-math attrs if we're building with fast-math.
       F.PropagateAttrs = true;
+      F.Internalize = true;
     }
     Opts.LinkBitcodeFiles.push_back(F);
   }
   Opts.SanitizeCoverageType =
       getLastArgIntValue(Args, OPT_fsanitize_coverage_type, 0, Diags);
   Opts.SanitizeCoverageIndirectCalls =
       Args.hasArg(OPT_fsanitize_coverage_indirect_calls);
   Opts.SanitizeCoverageTraceBB = Args.hasArg(OPT_fsanitize_coverage_trace_bb);

llvm/trunk/include/llvm/Linker/Linker.h

 //===- Linker.h - Module Linker Interface -----------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LINKER_LINKER_H
 #define LLVM_LINKER_LINKER_H
 
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Linker/IRMover.h"
 
 namespace llvm {
 class Module;
 class StructType;
 class Type;
 
 /// This class provides the core functionality of linking in LLVM. It keeps a
 /// pointer to the merged module so far. It doesn't take ownership of the
 /// module since it is assumed that the user of this class will want to do
 /// something with it after the linking.
 class Linker {
   IRMover Mover;
 
 public:
   enum Flags {
     None = 0,
     OverrideFromSrc = (1 << 0),
     LinkOnlyNeeded = (1 << 1),
-    InternalizeLinkedSymbols = (1 << 2),
   };
 
   Linker(Module &M);
 
   /// \brief Link \p Src into the composite.
   ///
   /// Passing OverrideSymbols as true will have symbols from Src
   /// shadow those in the Dest.
   ///
+  /// Passing InternalizeCallback will have the linker call the function with
+  /// the new module and a list of global value names to be internalized by the
+  /// callback.
+  ///
   /// Returns true on error.
-  bool linkInModule(std::unique_ptr<Module> Src, unsigned Flags = Flags::None);
+  bool linkInModule(std::unique_ptr<Module> Src, unsigned Flags = Flags::None,
+                    std::function<void(Module &, const StringSet<> &)>
+                        InternalizeCallback = {});
 
   static bool linkModules(Module &Dest, std::unique_ptr<Module> Src,
-                          unsigned Flags = Flags::None);
+                          unsigned Flags = Flags::None,
+                          std::function<void(Module &, const StringSet<> &)>
+                              InternalizeCallback = {});
 };
 
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/Linker/LinkModules.cpp

 //===- lib/Linker/LinkModules.cpp - Module Linker Implementation ----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the LLVM module linker.
 //
 //===----------------------------------------------------------------------===//
 
 #include "LinkDiagnosticInfo.h"
 #include "llvm-c/Linker.h"
 #include "llvm/ADT/SetVector.h"
-#include "llvm/ADT/StringSet.h"
 #include "llvm/IR/Comdat.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Linker/Linker.h"
 #include "llvm/Support/Error.h"
 using namespace llvm;
 
 namespace {
 
 /// This is an implementation class for the LinkModules function, which is the
 /// entrypoint for this file.
 class ModuleLinker {
   IRMover &Mover;
   std::unique_ptr<Module> SrcM;
 
   SetVector<GlobalValue *> ValuesToLink;
-  StringSet<> Internalize;
 
   /// For symbol clashes, prefer those from Src.
   unsigned Flags;
 
+  /// List of global value names that should be internalized.
+  StringSet<> Internalize;
+
+  /// Function that will perform the actual internalization. The reason for a
+  /// callback is that the linker cannot call internalizeModule without
+  /// creating a circular dependency between IPO and the linker.
+  std::function<void(Module &, const StringSet<> &)> InternalizeCallback;
+
   /// Used as the callback for lazy linking.
   /// The mover has just hit GV and we have to decide if it, and other members
   /// of the same comdat, should be linked. Every member to be linked is passed
   /// to Add.
   void addLazyFor(GlobalValue &GV, const IRMover::ValueAdder &Add);
 
   bool shouldOverrideFromSrc() { return Flags & Linker::OverrideFromSrc; }
   bool shouldLinkOnlyNeeded() { return Flags & Linker::LinkOnlyNeeded; }
-  bool shouldInternalizeLinkedSymbols() {
-    return Flags & Linker::InternalizeLinkedSymbols;
-  }
 
   bool shouldLinkFromSource(bool &LinkFromSrc, const GlobalValue &Dest,
                             const GlobalValue &Src);
 
   /// Should we have mover and linker error diag info?
   bool emitError(const Twine &Message) {
     SrcM->getContext().diagnose(LinkDiagnosticInfo(DS_Error, Message));
     return true;
class ModuleLinker {
   /// Drop GV if it is a member of a comdat that we are dropping.
   /// This can happen with COFF's largest selection kind.
   void dropReplacedComdat(GlobalValue &GV,
                           const DenseSet<const Comdat *> &ReplacedDstComdats);
 
   bool linkIfNeeded(GlobalValue &GV);
 
 public:
-  ModuleLinker(IRMover &Mover, std::unique_ptr<Module> SrcM, unsigned Flags)
-      : Mover(Mover), SrcM(std::move(SrcM)), Flags(Flags) {}
+  ModuleLinker(IRMover &Mover, std::unique_ptr<Module> SrcM, unsigned Flags,
+               std::function<void(Module &, const StringSet<> &)>
+                   InternalizeCallback = {})
+      : Mover(Mover), SrcM(std::move(SrcM)), Flags(Flags),
+        InternalizeCallback(std::move(InternalizeCallback)) {}
 
   bool run();
 };
 }
 
 static GlobalValue::VisibilityTypes
 getMinVisibility(GlobalValue::VisibilityTypes A,
                  GlobalValue::VisibilityTypes B) {

 }
 
 void ModuleLinker::addLazyFor(GlobalValue &GV, const IRMover::ValueAdder &Add) {
   // Add these to the internalize list
   if (!GV.hasLinkOnceLinkage() && !GV.hasAvailableExternallyLinkage() &&
       !shouldLinkOnlyNeeded())
     return;
 
-  if (shouldInternalizeLinkedSymbols())
+  if (InternalizeCallback)
     Internalize.insert(GV.getName());
   Add(GV);
 
   const Comdat *SC = GV.getComdat();
   if (!SC)
     return;
   for (GlobalValue *GV2 : LazyComdatMembers[SC]) {
     GlobalValue *DGV = getLinkedToGlobal(GV2);
     bool LinkFromSrc = true;
     if (DGV && shouldLinkFromSource(LinkFromSrc, *DGV, *GV2))
       return;
     if (!LinkFromSrc)
       continue;
-    if (shouldInternalizeLinkedSymbols())
+    if (InternalizeCallback)
       Internalize.insert(GV2->getName());
     Add(*GV2);
   }
 }
 
 void ModuleLinker::dropReplacedComdat(
     GlobalValue &GV, const DenseSet<const Comdat *> &ReplacedDstComdats) {
   Comdat *C = GV.getComdat();
for (GlobalValue *GV2 : LazyComdatMembers[SC]) {
       bool LinkFromSrc = true;
       if (DGV && shouldLinkFromSource(LinkFromSrc, *DGV, *GV2))
         return true;
       if (LinkFromSrc)
         ValuesToLink.insert(GV2);
     }
   }
 
-  if (shouldInternalizeLinkedSymbols()) {
+  if (InternalizeCallback) {
     for (GlobalValue *GV : ValuesToLink)
       Internalize.insert(GV->getName());
   }
 
   // FIXME: Propagate Errors through to the caller instead of emitting
   // diagnostics.
   bool HasErrors = false;
   if (Error E = Mover.move(std::move(SrcM), ValuesToLink.getArrayRef(),
                            [this](GlobalValue &GV, IRMover::ValueAdder Add) {
                              addLazyFor(GV, Add);
                            },
                            /* IsPerformingImport */ false)) {
     handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
       DstM.getContext().diagnose(LinkDiagnosticInfo(DS_Error, EIB.message()));
       HasErrors = true;
     });
   }
   if (HasErrors)
     return true;
 
-  for (auto &P : Internalize) {
-    GlobalValue *GV = DstM.getNamedValue(P.first());
-    GV->setLinkage(GlobalValue::InternalLinkage);
-  }
+  if (InternalizeCallback)
+    InternalizeCallback(DstM, Internalize);
 
   return false;
 }
 
 Linker::Linker(Module &M) : Mover(M) {}
 
-bool Linker::linkInModule(std::unique_ptr<Module> Src, unsigned Flags) {
-  ModuleLinker ModLinker(Mover, std::move(Src), Flags);
+bool Linker::linkInModule(
+    std::unique_ptr<Module> Src, unsigned Flags,
+    std::function<void(Module &, const StringSet<> &)> InternalizeCallback) {
+  ModuleLinker ModLinker(Mover, std::move(Src), Flags,
+                         std::move(InternalizeCallback));
   return ModLinker.run();
 }
 
 //===----------------------------------------------------------------------===//
 // LinkModules entrypoint.
 //===----------------------------------------------------------------------===//
 
 /// This function links two modules together, with the resulting Dest module
 /// modified to be the composite of the two input modules. If an error occurs,
 /// true is returned and ErrorMsg (if not null) is set to indicate the problem.
 /// Upon failure, the Dest module could be in a modified state, and shouldn't be
 /// relied on to be consistent.
-bool Linker::linkModules(Module &Dest, std::unique_ptr<Module> Src,
-                         unsigned Flags) {
+bool Linker::linkModules(
+    Module &Dest, std::unique_ptr<Module> Src, unsigned Flags,
+    std::function<void(Module &, const StringSet<> &)> InternalizeCallback) {
   Linker L(Dest);
-  return L.linkInModule(std::move(Src), Flags);
+  return L.linkInModule(std::move(Src), Flags, std::move(InternalizeCallback));
 }
 
 //===----------------------------------------------------------------------===//
 // C API.
 //===----------------------------------------------------------------------===//
 
 LLVMBool LLVMLinkModules2(LLVMModuleRef Dest, LLVMModuleRef Src) {
   Module *D = unwrap(Dest);
   std::unique_ptr<Module> M(unwrap(Src));
   return Linker::linkModules(*D, std::move(M));
 }

llvm/trunk/test/Linker/Inputs/linkage.d.ll

+@Y = global i8 42
+
+define i64 @foo() { ret i64 7 }
+
[e-mail] = appending global [2 x i8*] [i8* @Y, i8* bitcast (i64 ()* @foo to i8*)], section "llvm.metadata"

llvm/trunk/test/Linker/link-flags.ll

 ; RUN: llvm-link -S %S/Inputs/linkage.b.ll %S/Inputs/linkage.c.ll | FileCheck %s -check-prefix=B -check-prefix=C -check-prefix=CU
 ; RUN: llvm-link -S -only-needed %S/Inputs/linkage.b.ll %S/Inputs/linkage.c.ll | FileCheck %s -check-prefix=B -check-prefix=C -check-prefix=CN
 ; RUN: llvm-link -S -internalize %S/Inputs/linkage.b.ll %S/Inputs/linkage.c.ll | FileCheck %s -check-prefix=B -check-prefix=CI
 ; RUN: llvm-link -S -internalize -only-needed %S/Inputs/linkage.b.ll %S/Inputs/linkage.c.ll | FileCheck %s -check-prefix=B -check-prefix=CN
+; RUN: llvm-link -S -internalize %S/Inputs/linkage.b.ll %S/Inputs/linkage.c.ll %S/Inputs/linkage.d.ll | FileCheck %s -check-prefix=B -check-prefix=DI
 
 C-LABEL: @X = global i32 5
 CI-LABEL: @X = internal global i32 5
 CU-LABEL:@U = global i32 6
 CI-LABEL:@U = internal global i32 6
 CN-NOT:@U
+DI-LABEL: @Y = global i8 42
+DI-LABEL: @llvm.used = appending global [2 x i8*] [i8* @Y, i8* bitcast (i64 ()* @foo to i8*)], section "llvm.metadata"
 
 B-LABEL: define void @bar() {
 
 C-LABEL: define i32 @foo()
 CI-LABEL: define internal i32 @foo()
 
 CU-LABEL:define i32 @unused() {
 CI-LABEL:define internal i32 @unused() {
 CN-NOT:@unused()
+
+DI-LABEL: define internal i32 @foo.6()
+DI-LABEL: define i64 @foo()

llvm/trunk/tools/llvm-link/llvm-link.cpp

 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/SystemUtils.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Transforms/IPO/FunctionImport.h"
+#include "llvm/Transforms/IPO/Internalize.h"
 #include "llvm/Transforms/Utils/FunctionImportUtils.h"
 
 #include <memory>
 #include <utility>
 using namespace llvm;
 
 static cl::list<std::string>
 InputFilenames(cl::Positional, cl::OneOrMore,
static bool importFunctions(const char *argv0, Module &DestModule) {
   return true;
 }
 
 static bool linkFiles(const char *argv0, LLVMContext &Context, Linker &L,
                       const cl::list<std::string> &Files,
                       unsigned Flags) {
   // Filter out flags that don't apply to the first file we load.
   unsigned ApplicableFlags = Flags & Linker::Flags::OverrideFromSrc;
+  // Similar to some flags, internalization doesn't apply to the first file.
+  bool InternalizeLinkedSymbols = false;
   for (const auto &File : Files) {
     std::unique_ptr<Module> M = loadFile(argv0, File, Context);
     if (!M.get()) {
       errs() << argv0 << ": error loading file '" << File << "'\n";
       return false;
     }
 
     // Note that when ODR merging types cannot verify input files in here When
if (!SummaryIndex.empty()) {
       // Promotion
       if (renameModuleForThinLTO(*M, *Index))
         return true;
     }
 
     if (Verbose)
       errs() << "Linking in '" << File << "'\n";
 
-    if (L.linkInModule(std::move(M), ApplicableFlags))
+    bool Err = false;
+    if (InternalizeLinkedSymbols) {
+      Err = L.linkInModule(
+          std::move(M), ApplicableFlags, [](Module &M, const StringSet<> &GVS) {
+            internalizeModule(M, [&M, &GVS](const GlobalValue &GV) {
+              return !GV.hasName() || (GVS.count(GV.getName()) == 0);
+            });
+          });
+    } else {
+      Err = L.linkInModule(std::move(M), ApplicableFlags);
+    }
+
+    if (Err)
       return false;
+
+    // Internalization applies to linking of subsequent files.
+    InternalizeLinkedSymbols = Internalize;
+
     // All linker flags apply to linking of subsequent files.
     ApplicableFlags = Flags;
   }
 
   return true;
 }
 
 int main(int argc, char **argv) {
int main(int argc, char **argv) {
 
   if (!DisableDITypeMap)
     Context.enableDebugTypeODRUniquing();
 
   auto Composite = make_unique<Module>("llvm-link", Context);
   Linker L(*Composite);
 
   unsigned Flags = Linker::Flags::None;
-  if (Internalize)
-    Flags |= Linker::Flags::InternalizeLinkedSymbols;
   if (OnlyNeeded)
     Flags |= Linker::Flags::LinkOnlyNeeded;
 
   // First add all the regular input files
   if (!linkFiles(argv[0], Context, L, InputFilenames, Flags))
     return 1;
 
   // Next the -override ones.