Revert r221096 bringing back r221014 with a fix.
authorRafael Espindola <rafael.espindola@gmail.com>
Sun, 2 Nov 2014 13:28:57 +0000 (13:28 +0000)
committerRafael Espindola <rafael.espindola@gmail.com>
Sun, 2 Nov 2014 13:28:57 +0000 (13:28 +0000)
The issue was that linkAppendingVarProto does the full linking job, including
deleting the old dst variable. The fix is just to call it and return early
if we have a GV with appending linkage.

original message:

    Refactor duplicated code in liking GlobalValues.

    There is quiet a bit of logic that is common to any GlobalValue but was
    duplicated for Functions, GlobalVariables and GlobalAliases.

    While at it, merge visibility even when comdats are used, fixing pr21415.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@221098 91177308-0d34-0410-b5e6-96231b3b80d8

lib/Linker/LinkModules.cpp
test/Linker/Inputs/visibility.ll
test/Linker/visibility.ll

index 4320e88..bac187b 100644 (file)
@@ -453,13 +453,6 @@ namespace {
     bool getComdatResult(const Comdat *SrcC, Comdat::SelectionKind &SK,
                          bool &LinkFromSrc);
 
-    /// This analyzes the two global values and determines what the result will
-    /// look like in the destination module.
-    bool getLinkageResult(GlobalValue *Dest, const GlobalValue *Src,
-                          GlobalValue::LinkageTypes &LT,
-                          GlobalValue::VisibilityTypes &Vis,
-                          bool &LinkFromSrc);
-
     /// Given a global in the source module, return the global in the
     /// destination module that is being linked to, if any.
     GlobalValue *getLinkedToGlobal(const GlobalValue *SrcGV) {
@@ -488,9 +481,15 @@ namespace {
 
     bool linkAppendingVarProto(GlobalVariable *DstGV,
                                const GlobalVariable *SrcGV);
-    bool linkGlobalProto(const GlobalVariable *SrcGV);
-    bool linkFunctionProto(Function *SrcF);
-    bool linkAliasProto(GlobalAlias *SrcA);
+
+    bool linkGlobalValueProto(GlobalValue *GV);
+    GlobalValue *linkGlobalVariableProto(const GlobalVariable *SGVar,
+                                         GlobalValue *DGV, bool LinkFromSrc);
+    GlobalValue *linkFunctionProto(const Function *SF, GlobalValue *DGV,
+                                   bool LinkFromSrc);
+    GlobalValue *linkGlobalAliasProto(const GlobalAlias *SGA, GlobalValue *DGV,
+                                      bool LinkFromSrc);
+
     bool linkModuleFlagsMetadata();
 
     void linkAppendingVarInit(const AppendingVarInfo &AVI);
@@ -688,6 +687,12 @@ bool ModuleLinker::getComdatResult(const Comdat *SrcC,
 bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
                                         const GlobalValue &Dest,
                                         const GlobalValue &Src) {
+  // We always have to add Src if it has appending linkage.
+  if (Src.hasAppendingLinkage()) {
+    LinkFromSrc = true;
+    return false;
+  }
+
   bool SrcIsDeclaration = Src.isDeclarationForLinker();
   bool DestIsDeclaration = Dest.isDeclarationForLinker();
 
@@ -757,36 +762,6 @@ bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
                    "': symbol multiply defined!");
 }
 
-/// This analyzes the two global values and determines what the result will look
-/// like in the destination module. In particular, it computes the resultant
-/// linkage type and visibility, computes whether the global in the source
-/// should be copied over to the destination (replacing the existing one), and
-/// computes whether this linkage is an error or not.
-bool ModuleLinker::getLinkageResult(GlobalValue *Dest, const GlobalValue *Src,
-                                    GlobalValue::LinkageTypes &LT,
-                                    GlobalValue::VisibilityTypes &Vis,
-                                    bool &LinkFromSrc) {
-  assert(Dest && "Must have two globals being queried");
-  assert(!Src->hasLocalLinkage() &&
-         "If Src has internal linkage, Dest shouldn't be set!");
-
-  if (shouldLinkFromSource(LinkFromSrc, *Dest, *Src))
-    return true;
-
-  if (LinkFromSrc)
-    LT = Src->getLinkage();
-  else
-    LT = Dest->getLinkage();
-
-  // Compute the visibility. We follow the rules in the System V Application
-  // Binary Interface.
-  assert(!GlobalValue::isLocalLinkage(LT) &&
-         "Symbols with local linkage should not be merged");
-  Vis = isLessConstraining(Src->getVisibility(), Dest->getVisibility()) ?
-    Dest->getVisibility() : Src->getVisibility();
-  return false;
-}
-
 /// Loop over all of the linked values to compute type mappings.  For example,
 /// if we link "extern Foo *x" and "Foo *x = NULL", then we have two struct
 /// types 'Foo' but one got renamed when the module was loaded into the same
@@ -1010,256 +985,163 @@ bool ModuleLinker::linkAppendingVarProto(GlobalVariable *DstGV,
   return false;
 }
 
-/// Loop through the global variables in the src module and merge them into the
-/// dest module.
-bool ModuleLinker::linkGlobalProto(const GlobalVariable *SGV) {
+bool ModuleLinker::linkGlobalValueProto(GlobalValue *SGV) {
   GlobalValue *DGV = getLinkedToGlobal(SGV);
-  llvm::Optional<GlobalValue::VisibilityTypes> NewVisibility;
+
+  // Handle the ultra special appending linkage case first.
+  if (DGV && DGV->hasAppendingLinkage())
+    return linkAppendingVarProto(cast<GlobalVariable>(DGV),
+                                 cast<GlobalVariable>(SGV));
+
+  bool LinkFromSrc = true;
+  Comdat *C = nullptr;
+  GlobalValue::VisibilityTypes Visibility = SGV->getVisibility();
   bool HasUnnamedAddr = SGV->hasUnnamedAddr();
-  unsigned Alignment = SGV->getAlignment();
 
-  bool LinkFromSrc = false;
-  Comdat *DC = nullptr;
   if (const Comdat *SC = SGV->getComdat()) {
     Comdat::SelectionKind SK;
     std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];
-    DC = DstM->getOrInsertComdat(SC->getName());
-    DC->setSelectionKind(SK);
+    C = DstM->getOrInsertComdat(SC->getName());
+    C->setSelectionKind(SK);
+  } else if (DGV) {
+    if (shouldLinkFromSource(LinkFromSrc, *DGV, *SGV))
+      return true;
   }
 
-  if (DGV) {
-    if (!DC) {
-      // Concatenation of appending linkage variables is magic and handled later.
-      if (DGV->hasAppendingLinkage() || SGV->hasAppendingLinkage())
-        return linkAppendingVarProto(cast<GlobalVariable>(DGV), SGV);
-
-      // Determine whether linkage of these two globals follows the source
-      // module's definition or the destination module's definition.
-      GlobalValue::LinkageTypes NewLinkage = GlobalValue::InternalLinkage;
-      GlobalValue::VisibilityTypes NV;
-      if (getLinkageResult(DGV, SGV, NewLinkage, NV, LinkFromSrc))
-        return true;
-      NewVisibility = NV;
-      HasUnnamedAddr = HasUnnamedAddr && DGV->hasUnnamedAddr();
-      if (DGV->hasCommonLinkage() && SGV->hasCommonLinkage())
-        Alignment = std::max(Alignment, DGV->getAlignment());
-      else if (!LinkFromSrc)
-        Alignment = DGV->getAlignment();
-
-      // If we're not linking from the source, then keep the definition that we
-      // have.
-      if (!LinkFromSrc) {
-        // Special case for const propagation.
-        if (GlobalVariable *DGVar = dyn_cast<GlobalVariable>(DGV)) {
-          DGVar->setAlignment(Alignment);
-
-          if (DGVar->isDeclaration() && !SGV->isConstant())
-            DGVar->setConstant(false);
-        }
-
-        // Set calculated linkage, visibility and unnamed_addr.
-        DGV->setLinkage(NewLinkage);
-        DGV->setVisibility(*NewVisibility);
-        DGV->setUnnamedAddr(HasUnnamedAddr);
-      }
-    }
+  if (!LinkFromSrc) {
+    // Track the source global so that we don't attempt to copy it over when
+    // processing global initializers.
+    DoNotLinkFromSource.insert(SGV);
 
-    if (!LinkFromSrc) {
+    if (DGV)
       // Make sure to remember this mapping.
-      ValueMap[SGV] = ConstantExpr::getBitCast(DGV,TypeMap.get(SGV->getType()));
-
-      // Track the source global so that we don't attempt to copy it over when
-      // processing global initializers.
-      DoNotLinkFromSource.insert(SGV);
+      ValueMap[SGV] =
+          ConstantExpr::getBitCast(DGV, TypeMap.get(SGV->getType()));
+  }
 
-      return false;
-    }
+  if (DGV) {
+    Visibility = isLessConstraining(Visibility, DGV->getVisibility())
+                     ? DGV->getVisibility()
+                     : Visibility;
+    HasUnnamedAddr = HasUnnamedAddr && DGV->hasUnnamedAddr();
   }
 
-  // If the Comdat this variable was inside of wasn't selected, skip it.
-  if (DC && !DGV && !LinkFromSrc) {
-    DoNotLinkFromSource.insert(SGV);
+  if (!LinkFromSrc && !DGV)
     return false;
+
+  GlobalValue *NewGV;
+  if (auto *SGVar = dyn_cast<GlobalVariable>(SGV)) {
+    NewGV = linkGlobalVariableProto(SGVar, DGV, LinkFromSrc);
+    if (!NewGV)
+      return true;
+  } else if (auto *SF = dyn_cast<Function>(SGV)) {
+    NewGV = linkFunctionProto(SF, DGV, LinkFromSrc);
+  } else {
+    NewGV = linkGlobalAliasProto(cast<GlobalAlias>(SGV), DGV, LinkFromSrc);
   }
 
-  // No linking to be performed or linking from the source: simply create an
-  // identical version of the symbol over in the dest module... the
-  // initializer will be filled in later by LinkGlobalInits.
-  GlobalVariable *NewDGV =
-    new GlobalVariable(*DstM, TypeMap.get(SGV->getType()->getElementType()),
-                       SGV->isConstant(), SGV->getLinkage(), /*init*/nullptr,
-                       SGV->getName(), /*insertbefore*/nullptr,
-                       SGV->getThreadLocalMode(),
-                       SGV->getType()->getAddressSpace());
-  // Propagate alignment, visibility and section info.
-  copyGVAttributes(NewDGV, SGV);
-  NewDGV->setAlignment(Alignment);
-  if (NewVisibility)
-    NewDGV->setVisibility(*NewVisibility);
-  NewDGV->setUnnamedAddr(HasUnnamedAddr);
+  if (NewGV) {
+    if (NewGV != DGV)
+      copyGVAttributes(NewGV, SGV);
 
-  if (DC)
-    NewDGV->setComdat(DC);
+    NewGV->setUnnamedAddr(HasUnnamedAddr);
+    NewGV->setVisibility(Visibility);
 
-  if (DGV) {
-    DGV->replaceAllUsesWith(ConstantExpr::getBitCast(NewDGV, DGV->getType()));
-    DGV->eraseFromParent();
+    if (auto *NewGO = dyn_cast<GlobalObject>(NewGV)) {
+      if (C)
+        NewGO->setComdat(C);
+    }
+
+    // Make sure to remember this mapping.
+    if (NewGV != DGV) {
+      if (DGV) {
+        DGV->replaceAllUsesWith(
+            ConstantExpr::getBitCast(NewGV, DGV->getType()));
+        DGV->eraseFromParent();
+      }
+      ValueMap[SGV] = NewGV;
+    }
   }
 
-  // Make sure to remember this mapping.
-  ValueMap[SGV] = NewDGV;
   return false;
 }
 
-/// Link the function in the source module into the destination module if
-/// needed, setting up mapping information.
-bool ModuleLinker::linkFunctionProto(Function *SF) {
-  GlobalValue *DGV = getLinkedToGlobal(SF);
-  llvm::Optional<GlobalValue::VisibilityTypes> NewVisibility;
-  bool HasUnnamedAddr = SF->hasUnnamedAddr();
-
-  bool LinkFromSrc = false;
-  Comdat *DC = nullptr;
-  if (const Comdat *SC = SF->getComdat()) {
-    Comdat::SelectionKind SK;
-    std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];
-    DC = DstM->getOrInsertComdat(SC->getName());
-    DC->setSelectionKind(SK);
-  }
+/// Loop through the global variables in the src module and merge them into the
+/// dest module.
+GlobalValue *ModuleLinker::linkGlobalVariableProto(const GlobalVariable *SGVar,
+                                                   GlobalValue *DGV,
+                                                   bool LinkFromSrc) {
+  unsigned Alignment = 0;
+  bool ClearConstant = false;
 
   if (DGV) {
-    if (!DC) {
-      GlobalValue::LinkageTypes NewLinkage = GlobalValue::InternalLinkage;
-      GlobalValue::VisibilityTypes NV;
-      if (getLinkageResult(DGV, SF, NewLinkage, NV, LinkFromSrc))
-        return true;
-      NewVisibility = NV;
-      HasUnnamedAddr = HasUnnamedAddr && DGV->hasUnnamedAddr();
-
-      if (!LinkFromSrc) {
-        // Set calculated linkage
-        DGV->setLinkage(NewLinkage);
-        DGV->setVisibility(*NewVisibility);
-        DGV->setUnnamedAddr(HasUnnamedAddr);
-      }
-    }
+    if (DGV->hasCommonLinkage() && SGVar->hasCommonLinkage())
+      Alignment = std::max(SGVar->getAlignment(), DGV->getAlignment());
 
-    if (!LinkFromSrc) {
-      // Make sure to remember this mapping.
-      ValueMap[SF] = ConstantExpr::getBitCast(DGV, TypeMap.get(SF->getType()));
-
-      // Track the function from the source module so we don't attempt to remap
-      // it.
-      DoNotLinkFromSource.insert(SF);
+    auto *DGVar = dyn_cast<GlobalVariable>(DGV);
+    if (!SGVar->isConstant() || (DGVar && !DGVar->isConstant()))
+      ClearConstant = true;
+  }
 
-      return false;
+  if (!LinkFromSrc) {
+    if (auto *NewGVar = dyn_cast<GlobalVariable>(DGV)) {
+      if (Alignment)
+        NewGVar->setAlignment(Alignment);
+      if (NewGVar->isDeclaration() && ClearConstant)
+        NewGVar->setConstant(false);
     }
+    return DGV;
   }
 
+  // No linking to be performed or linking from the source: simply create an
+  // identical version of the symbol over in the dest module... the
+  // initializer will be filled in later by LinkGlobalInits.
+  GlobalVariable *NewDGV = new GlobalVariable(
+      *DstM, TypeMap.get(SGVar->getType()->getElementType()),
+      SGVar->isConstant(), SGVar->getLinkage(), /*init*/ nullptr,
+      SGVar->getName(), /*insertbefore*/ nullptr, SGVar->getThreadLocalMode(),
+      SGVar->getType()->getAddressSpace());
+
+  if (Alignment)
+    NewDGV->setAlignment(Alignment);
+
+  return NewDGV;
+}
+
+/// Link the function in the source module into the destination module if
+/// needed, setting up mapping information.
+GlobalValue *ModuleLinker::linkFunctionProto(const Function *SF,
+                                             GlobalValue *DGV,
+                                             bool LinkFromSrc) {
+  if (!LinkFromSrc)
+    return DGV;
+
   // If the function is to be lazily linked, don't create it just yet.
   // The ValueMaterializerTy will deal with creating it if it's used.
   if (!DGV && (SF->hasLocalLinkage() || SF->hasLinkOnceLinkage() ||
                SF->hasAvailableExternallyLinkage())) {
     DoNotLinkFromSource.insert(SF);
-    return false;
-  }
-
-  // If the Comdat this function was inside of wasn't selected, skip it.
-  if (DC && !DGV && !LinkFromSrc) {
-    DoNotLinkFromSource.insert(SF);
-    return false;
+    return nullptr;
   }
 
   // If there is no linkage to be performed or we are linking from the source,
   // bring SF over.
-  Function *NewDF = Function::Create(TypeMap.get(SF->getFunctionType()),
-                                     SF->getLinkage(), SF->getName(), DstM);
-  copyGVAttributes(NewDF, SF);
-  if (NewVisibility)
-    NewDF->setVisibility(*NewVisibility);
-  NewDF->setUnnamedAddr(HasUnnamedAddr);
-
-  if (DC)
-    NewDF->setComdat(DC);
-
-  if (DGV) {
-    // Any uses of DF need to change to NewDF, with cast.
-    DGV->replaceAllUsesWith(ConstantExpr::getBitCast(NewDF, DGV->getType()));
-    DGV->eraseFromParent();
-  }
-
-  ValueMap[SF] = NewDF;
-  return false;
+  return Function::Create(TypeMap.get(SF->getFunctionType()), SF->getLinkage(),
+                          SF->getName(), DstM);
 }
 
 /// Set up prototypes for any aliases that come over from the source module.
-bool ModuleLinker::linkAliasProto(GlobalAlias *SGA) {
-  GlobalValue *DGV = getLinkedToGlobal(SGA);
-  llvm::Optional<GlobalValue::VisibilityTypes> NewVisibility;
-  bool HasUnnamedAddr = SGA->hasUnnamedAddr();
-
-  bool LinkFromSrc = false;
-  Comdat *DC = nullptr;
-  if (const Comdat *SC = SGA->getComdat()) {
-    Comdat::SelectionKind SK;
-    std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];
-    DC = DstM->getOrInsertComdat(SC->getName());
-    DC->setSelectionKind(SK);
-  }
-
-  if (DGV) {
-    if (!DC) {
-      GlobalValue::LinkageTypes NewLinkage = GlobalValue::InternalLinkage;
-      GlobalValue::VisibilityTypes NV;
-      if (getLinkageResult(DGV, SGA, NewLinkage, NV, LinkFromSrc))
-        return true;
-      NewVisibility = NV;
-      HasUnnamedAddr = HasUnnamedAddr && DGV->hasUnnamedAddr();
-
-      if (!LinkFromSrc) {
-        // Set calculated linkage.
-        DGV->setLinkage(NewLinkage);
-        DGV->setVisibility(*NewVisibility);
-        DGV->setUnnamedAddr(HasUnnamedAddr);
-      }
-    }
-
-    if (!LinkFromSrc) {
-      // Make sure to remember this mapping.
-      ValueMap[SGA] = ConstantExpr::getBitCast(DGV,TypeMap.get(SGA->getType()));
-
-      // Track the alias from the source module so we don't attempt to remap it.
-      DoNotLinkFromSource.insert(SGA);
-
-      return false;
-    }
-  }
-
-  // If the Comdat this alias was inside of wasn't selected, skip it.
-  if (DC && !DGV && !LinkFromSrc) {
-    DoNotLinkFromSource.insert(SGA);
-    return false;
-  }
+GlobalValue *ModuleLinker::linkGlobalAliasProto(const GlobalAlias *SGA,
+                                                GlobalValue *DGV,
+                                                bool LinkFromSrc) {
+  if (!LinkFromSrc)
+    return DGV;
 
   // If there is no linkage to be performed or we're linking from the source,
   // bring over SGA.
   auto *PTy = cast<PointerType>(TypeMap.get(SGA->getType()));
-  auto *NewDA =
-      GlobalAlias::create(PTy->getElementType(), PTy->getAddressSpace(),
-                          SGA->getLinkage(), SGA->getName(), DstM);
-  copyGVAttributes(NewDA, SGA);
-  if (NewVisibility)
-    NewDA->setVisibility(*NewVisibility);
-  NewDA->setUnnamedAddr(HasUnnamedAddr);
-
-  if (DGV) {
-    // Any uses of DGV need to change to NewDA, with cast.
-    DGV->replaceAllUsesWith(ConstantExpr::getBitCast(NewDA, DGV->getType()));
-    DGV->eraseFromParent();
-  }
-
-  ValueMap[SGA] = NewDA;
-  return false;
+  return GlobalAlias::create(PTy->getElementType(), PTy->getAddressSpace(),
+                             SGA->getLinkage(), SGA->getName(), DstM);
 }
 
 static void getArrayElements(const Constant *C,
@@ -1599,7 +1481,7 @@ bool ModuleLinker::run() {
   // initializers (which could refer to functions not yet mapped over).
   for (Module::global_iterator I = SrcM->global_begin(),
        E = SrcM->global_end(); I != E; ++I)
-    if (linkGlobalProto(I))
+    if (linkGlobalValueProto(I))
       return true;
 
   // Link the functions together between the two modules, without doing function
@@ -1608,13 +1490,13 @@ bool ModuleLinker::run() {
   // all of the global values that may be referenced are available in our
   // ValueMap.
   for (Module::iterator I = SrcM->begin(), E = SrcM->end(); I != E; ++I)
-    if (linkFunctionProto(I))
+    if (linkGlobalValueProto(I))
       return true;
 
   // If there were any aliases, link them now.
   for (Module::alias_iterator I = SrcM->alias_begin(),
        E = SrcM->alias_end(); I != E; ++I)
-    if (linkAliasProto(I))
+    if (linkGlobalValueProto(I))
       return true;
 
   for (unsigned i = 0, e = AppendingVars.size(); i != e; ++i)
index f5b2065..2ab58fd 100644 (file)
@@ -1,7 +1,10 @@
+$c1 = comdat any
+
 ; Variables
 @v1 = weak hidden global i32 0
 @v2 = weak protected global i32 0
 @v3 = weak hidden global i32 0
+@v4 = hidden global i32 1, comdat $c1
 
 ; Aliases
 @a1 = weak hidden alias i32* @v1
index fc950fa..6436197 100644 (file)
@@ -4,16 +4,21 @@
 ; The values in this file are strong, the ones in Inputs/visibility.ll are weak,
 ; but we should still get the visibility from them.
 
+
+$c1 = comdat any
+
 ; Variables
-; CHECK: @v1 = hidden global i32 0
+; CHECK-DAG: @v1 = hidden global i32 0
 @v1 = global i32 0
 
-; CHECK: @v2 = protected  global i32 0
+; CHECK-DAG: @v2 = protected  global i32 0
 @v2 = global i32 0
 
-; CHECK: @v3 = hidden global i32 0
+; CHECK-DAG: @v3 = hidden global i32 0
 @v3 = protected global i32 0
 
+; CHECK-DAG: @v4 = hidden global i32 1, comdat $c1
+@v4 = global i32 1, comdat $c1
 
 ; Aliases
 ; CHECK: @a1 = hidden alias i32* @v1