[llvm-link] Fix for an assertion when linking global with appending linkage

[sdmitriev]

Summary

This patch fixes llvm-link assertion when linking external variable
declaration with a definition with appending linkage.

[jdoerfert]

Given the langref text I'm unsure what it means to link an append linkage variable to a non-append linkage one. I can see how extern is different but what would happen before & now if you remove the extern. And what happens before/now if you have append on both @var? We should add those tests if possible.

llvm/lib/Linker/IRMover.cpp

941

parenthesis around the condition please. maybe move it to a new line given the complexity.

[sdmitriev]

Given the langref text I'm unsure what it means to link an append linkage variable to a non-append linkage one. I can see how extern is different but what would happen before & now if you remove the extern.

On current implementation llvm-link will die with an assertion no matter if extern is there or not. With this (updated) patch llvm-link will print an error message that both variables should have appending linkage (if extern is removed). I have added such test as well as few other negative tests.

And what happens before/now if you have append on both @var? We should add those tests if possible.

That should be fine, arrays from both vars will be appended in the result as described in the langref. This behavior is already checked by the following tests - Linker/AppendingLinkage.ll, Linker/AppendingLinkage2.ll.

[jdoerfert]

Looks reasonable to me. Test coverage is nice now, thanks.

llvm/lib/Linker/IRMover.cpp

static void getArrayElements(const Constant *C,
   for (unsigned i = 0; i != NumElements; ++i)
     Dest.push_back(C->getAggregateElement(i));
 }
 
 /// If there were any appending global variables, link them together now.
 Expected<Constant *>
 IRLinker::linkAppendingVarProto(GlobalVariable *DstGV,
                                 const GlobalVariable *SrcGV) {
+  // Check that both variables have compatible properties.
+  if (DstGV && !DstGV->isDeclaration() && !SrcGV->isDeclaration()) {
+    if (!SrcGV->hasAppendingLinkage() || !DstGV->hasAppendingLinkage())
+      return stringErr(
+          "Linking globals named '" + SrcGV->getName() +
+          "': can only link appending global with another appending "
+          "global!");
+
+    if (DstGV->isConstant() != SrcGV->isConstant())
+      return stringErr("Appending variables linked with different const'ness!");
+
+    if (DstGV->getAlignment() != SrcGV->getAlignment())
+      return stringErr(
+          "Appending variables with different alignment need to be linked!");
+
+    if (DstGV->getVisibility() != SrcGV->getVisibility())
+      return stringErr(
+          "Appending variables with different visibility need to be linked!");
+
+    if (DstGV->hasGlobalUnnamedAddr() != SrcGV->hasGlobalUnnamedAddr())
+      return stringErr(
+          "Appending variables with different unnamed_addr need to be linked!");
+
+    if (DstGV->getSection() != SrcGV->getSection())
+      return stringErr(
+          "Appending variables with different section name need to be linked!");
+  }
+
+  // Do not need to do anything if source is a declaration.
+  if (SrcGV->isDeclaration())
+    return DstGV;
+
   Type *EltTy = cast<ArrayType>(TypeMap.get(SrcGV->getValueType()))
                     ->getElementType();
 
   // FIXME: This upgrade is done during linking to support the C API.  Once the
   // old form is deprecated, we should move this upgrade to
   // llvm::UpgradeGlobalVariable() and simplify the logic here and in
   // Mapper::mapAppendingVariable() in ValueMapper.cpp.
   StringRef Name = SrcGV->getName();
IRLinker::linkAppendingVarProto(GlobalVariable *DstGV,
   PointerType *VoidPtrTy = Type::getInt8Ty(SrcGV->getContext())->getPointerTo();
   if (IsOldStructor) {
     auto &ST = *cast<StructType>(EltTy);
     Type *Tys[3] = {ST.getElementType(0), ST.getElementType(1), VoidPtrTy};
     EltTy = StructType::get(SrcGV->getContext(), Tys, false);
   }
 
   uint64_t DstNumElements = 0;
-  if (DstGV) {
+  if (DstGV && !DstGV->isDeclaration()) {
     ArrayType *DstTy = cast<ArrayType>(DstGV->getValueType());
     DstNumElements = DstTy->getNumElements();
 
-    if (!SrcGV->hasAppendingLinkage() || !DstGV->hasAppendingLinkage())
-      return stringErr(
-          "Linking globals named '" + SrcGV->getName() +
-          "': can only link appending global with another appending "
-          "global!");
-
     // Check to see that they two arrays agree on type.
     if (EltTy != DstTy->getElementType())
       return stringErr("Appending variables with different element types!");
-    if (DstGV->isConstant() != SrcGV->isConstant())
-      return stringErr("Appending variables linked with different const'ness!");
-
-    if (DstGV->getAlignment() != SrcGV->getAlignment())
-      return stringErr(
-          "Appending variables with different alignment need to be linked!");
-
-    if (DstGV->getVisibility() != SrcGV->getVisibility())
-      return stringErr(
-          "Appending variables with different visibility need to be linked!");
-
-    if (DstGV->hasGlobalUnnamedAddr() != SrcGV->hasGlobalUnnamedAddr())
-      return stringErr(
-          "Appending variables with different unnamed_addr need to be linked!");
-
-    if (DstGV->getSection() != SrcGV->getSection())
-      return stringErr(
-          "Appending variables with different section name need to be linked!");
   }
 
   SmallVector<Constant *, 16> SrcElements;
   getArrayElements(SrcGV->getInitializer(), SrcElements);
 
   if (IsNewStructor) {
     erase_if(SrcElements, [this](Constant *E) {
       auto *Key =
GlobalVariable *NG = new GlobalVariable(
       /*init*/ nullptr, /*name*/ "", DstGV, SrcGV->getThreadLocalMode(),
       SrcGV->getAddressSpace());
 
   NG->copyAttributesFrom(SrcGV);
   forceRenaming(NG, SrcGV->getName());
 
   Constant *Ret = ConstantExpr::getBitCast(NG, TypeMap.get(SrcGV->getType()));
 
-  Mapper.scheduleMapAppendingVariable(*NG,
-                                      DstGV ? DstGV->getInitializer() : nullptr,
-                                      IsOldStructor, SrcElements);
+  Mapper.scheduleMapAppendingVariable(
+      *NG,
+      (DstGV && !DstGV->isDeclaration()) ? DstGV->getInitializer() : nullptr,
 
   // Replace any uses of the two global variables with uses of the new
   // global.
   if (DstGV) {
     RAUWWorklist.push_back(
         std::make_pair(DstGV, ConstantExpr::getBitCast(NG, DstGV->getType())));
   }
 
if (ShouldLink) {
     if (I != IndirectSymbolValueMap.end())
       return cast<Constant>(I->second);
   }
 
   if (!ShouldLink && ForIndirectSymbol)
     DGV = nullptr;
 
   // Handle the ultra special appending linkage case first.
-  assert(!DGV || SGV->hasAppendingLinkage() == DGV->hasAppendingLinkage());
-  if (SGV->hasAppendingLinkage())
+  if (SGV->hasAppendingLinkage() || (DGV && DGV->hasAppendingLinkage()))
     return linkAppendingVarProto(cast_or_null<GlobalVariable>(DGV),
                                  cast<GlobalVariable>(SGV));
 
   GlobalValue *NewGV;
   if (DGV && !ShouldLink) {
     NewGV = DGV;
   } else {
     // If we are done linking global value bodies (i.e. we are performing

llvm/lib/Linker/LinkModules.cpp

bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
 
   // Should we unconditionally use the Src?
   if (shouldOverrideFromSrc()) {
     LinkFromSrc = true;
     return false;
   }
 
   // We always have to add Src if it has appending linkage.
-  if (Src.hasAppendingLinkage()) {
+  if (Src.hasAppendingLinkage() || Dest.hasAppendingLinkage()) {
     LinkFromSrc = true;
     return false;
   }
 
   bool SrcIsDeclaration = Src.isDeclarationForLinker();
   bool DestIsDeclaration = Dest.isDeclarationForLinker();
 
   if (SrcIsDeclaration) {

llvm/test/Linker/Inputs/appending-global.ll

+@var = appending global [1 x i8* ] undef

llvm/test/Linker/appending-global-err1.ll

+; RUN: not llvm-link %s %p/Inputs/appending-global.ll -S -o - 2>&1 | FileCheck %s
+; RUN: not llvm-link %p/Inputs/appending-global.ll %s -S -o - 2>&1 | FileCheck %s
+
+; Negative test to check that global variable with appending linkage can only be
+; linked with another global variable with appending linkage.
+
+; CHECK: can only link appending global with another appending global
+
+@var = global i8* undef

llvm/test/Linker/appending-global-err2.ll

+; RUN: not llvm-link %s %p/Inputs/appending-global.ll -S -o - 2>&1 | FileCheck %s
+; RUN: not llvm-link %p/Inputs/appending-global.ll %s -S -o - 2>&1 | FileCheck %s
+
+; Negative test to check that global variables with appending linkage and
+; different constness cannot be linked.
+
+; CHECK: Appending variables linked with different const'ness
+
+@var = appending constant [1 x i8* ] undef

llvm/test/Linker/appending-global-err3.ll

+; RUN: not llvm-link %s %p/Inputs/appending-global.ll -S -o - 2>&1 | FileCheck %s
+; RUN: not llvm-link %p/Inputs/appending-global.ll %s -S -o - 2>&1 | FileCheck %s
+
+; Negative test to check that global variables with appending linkage and
+; different element types cannot be linked.
+
+; CHECK: Appending variables with different element types
+
+@var = appending global [1 x i32* ] undef

llvm/test/Linker/appending-global-err4.ll

+; RUN: not llvm-link %s %p/Inputs/appending-global.ll -S -o - 2>&1 | FileCheck %s
+; RUN: not llvm-link %p/Inputs/appending-global.ll %s -S -o - 2>&1 | FileCheck %s
+
+; Negative test to check that global variables with appending linkage and
+; different visibility cannot be linked.
+
+; CHECK: Appending variables with different visibility need to be linked
+
+@var = appending hidden global [1 x i32* ] undef

llvm/test/Linker/appending-global-err5.ll

+; RUN: not llvm-link %s %p/Inputs/appending-global.ll -S -o - 2>&1 | FileCheck %s
+; RUN: not llvm-link %p/Inputs/appending-global.ll %s -S -o - 2>&1 | FileCheck %s
+
+; Negative test to check that global variables with appending linkage and
+; different sections cannot be linked.
+
+; CHECK: Appending variables with different section name need to be linked
+
+@var = appending global [1 x i32* ] undef, section "dummy"

llvm/test/Linker/appending-global-proto.ll

+; RUN: llvm-link %s %p/Inputs/appending-global.ll -S -o - | FileCheck %s
+; RUN: llvm-link %p/Inputs/appending-global.ll %s -S -o - | FileCheck %s
+
+; Checks that we can link global variable with appending linkage with the
+; existing external declaration.
+
+; CHECK-DAG: @var = appending global [1 x i8*] undef
+; CHECK-DAG: @use = global [1 x i8*] [i8* bitcast ([1 x i8*]* @var to i8*)]
+
+@var = external global i8*
+@use = global [1 x i8*] [i8* bitcast (i8** @var to i8*)]