[Linker] Fix crash handling appending linkage

Summary:
When linking two llvm.used arrays, if the resulting merged
array ends up with duplicated elements (with the same name) but with
different types, the IRLinker was crashing. This was supposed to be
legal, as the IRLinker bitcasts elements to match types in these
situations.

This bug was exposed by D56928 in clang to support attribute used
in member functions of class templates. Crash happened when self-hosting
with LTO. Since LLVM depends on attribute used to generate code
for the dump() method, ubiquitous in the code base, many input bc
had a definition of this method referenced in their llvm.used array.
Some of these classes got optimized, changing the type of the first
parameter (this) in the dump method, leading to a scenario with a
pool of valid definitions but some with a different type, triggering
this bug.

This is a memory bug: ValueMapper depends on (calls) the materializer
provided by IRLinker, and this materializer was freely calling RAUW
methods whenever a global definition was updated in the temporary merged
output file. However, replaceAllUsesWith may or may not destroy
constants that use this global. If the linked definition has a type
mismatch regarding the new def and the old def, the materializer would
bitcast the old type to the new type and the elements of the llvm.used
array, which already uses bitcast to i8*, would end up with elements
cascading two bitcasts. RAUW would then indirectly call the
constantfolder to update the constant to the new ref, which would,
instead of updating the constant, destroy it to be able to create
a new constant that folds the two bitcasts into one. The problem is that
ValueMapper works with pointers to the same constants that may be
getting destroyed by RAUW. Obviously, RAUW can update references in the
Module to do not use the old destroyed constant, but it can't update
ValueMapper's internal pointers to these constants, which are now
invalid.

The approach here is to move the task of RAUWing old definitions
outside of the materializer.

Test Plan:
Added LIT test case, tested clang self-hosting with D56928 and
verified it works

Reviewed By: efriedma

Differential Revision: https://reviews.llvm.org/D59552

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

diff --git a/lib/Linker/IRMover.cpp b/lib/Linker/IRMover.cpp
index 70d5262fd618ef4eee7cfe2b17bcb0f2dc037319..b21b8f33ab42fd37495e9574972326a46b588676 100644 (file)
--- a/lib/Linker/IRMover.cpp
+++ b/lib/Linker/IRMover.cpp
@@ -402,6 +402,7 @@ class IRLinker {
 
   DenseSet<GlobalValue *> ValuesToLink;
   std::vector<GlobalValue *> Worklist;
+  std::vector<std::pair<GlobalValue *, Value*>> RAUWWorklist;
 
   void maybeAdd(GlobalValue *GV) {
     if (ValuesToLink.insert(GV).second)
@@ -494,6 +495,14 @@ class IRLinker {
   Function *copyFunctionProto(const Function *SF);
   GlobalValue *copyGlobalAliasProto(const GlobalAlias *SGA);
 
+  /// Perform "replace all uses with" operations. These work items need to be
+  /// performed as part of materialization, but we postpone them to happen after
+  /// materialization is done. The materializer called by ValueMapper is not
+  /// expected to delete constants, as ValueMapper is holding pointers to some
+  /// of them, but constant destruction may be indirectly triggered by RAUW.
+  /// Hence, the need to move this out of the materialization call chain.
+  void flushRAUWWorklist();
+
   /// When importing for ThinLTO, prevent importing of types listed on
   /// the DICompileUnit that we don't need a copy of in the importing
   /// module.
@@ -883,8 +892,8 @@ IRLinker::linkAppendingVarProto(GlobalVariable *DstGV,
   // Replace any uses of the two global variables with uses of the new
   // global.
   if (DstGV) {
-    DstGV->replaceAllUsesWith(ConstantExpr::getBitCast(NG, DstGV->getType()));
-    DstGV->eraseFromParent();
+    RAUWWorklist.push_back(
+        std::make_pair(DstGV, ConstantExpr::getBitCast(NG, DstGV->getType())));
   }
 
   return Ret;
@@ -983,9 +992,12 @@ Expected<Constant *> IRLinker::linkGlobalValueProto(GlobalValue *SGV,
   }
 
   if (DGV && NewGV != DGV) {
-    DGV->replaceAllUsesWith(
-      ConstantExpr::getPointerBitCastOrAddrSpaceCast(NewGV, DGV->getType()));
-    DGV->eraseFromParent();
+    // Schedule "replace all uses with" to happen after materializing is
+    // done. It is not safe to do it now, since ValueMapper may be holding
+    // pointers to constants that will get deleted if RAUW runs.
+    RAUWWorklist.push_back(std::make_pair(
+        DGV,
+        ConstantExpr::getPointerBitCastOrAddrSpaceCast(NewGV, DGV->getType())));
   }
 
   return C;
@@ -1043,6 +1055,18 @@ Error IRLinker::linkGlobalValueBody(GlobalValue &Dst, GlobalValue &Src) {
   return Error::success();
 }
 
+void IRLinker::flushRAUWWorklist() {
+  for (const auto Elem : RAUWWorklist) {
+    GlobalValue *Old;
+    Value *New;
+    std::tie(Old, New) = Elem;
+
+    Old->replaceAllUsesWith(New);
+    Old->eraseFromParent();
+  }
+  RAUWWorklist.clear();
+}
+
 void IRLinker::prepareCompileUnitsForImport() {
   NamedMDNode *SrcCompileUnits = SrcM->getNamedMetadata("llvm.dbg.cu");
   if (!SrcCompileUnits)
@@ -1368,6 +1392,7 @@ Error IRLinker::run() {
     Mapper.mapValue(*GV);
     if (FoundError)
       return std::move(*FoundError);
+    flushRAUWWorklist();
   }
 
   // Note that we are done linking global value bodies. This prevents

diff --git a/test/LTO/Resolution/X86/Inputs/appending-var-2.ll b/test/LTO/Resolution/X86/Inputs/appending-var-2.ll
new file mode 100644 (file)
index 0000000..29bc843
--- /dev/null
+++ b/test/LTO/Resolution/X86/Inputs/appending-var-2.ll
@@ -0,0 +1,14 @@
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%"foo" = type { i8 }
+
+@llvm.used = appending global [1 x i8*] [i8* bitcast (i32 (%"foo"*)* @bar to i8*)], section "llvm.metadata"
+
+; Function Attrs: norecurse nounwind readnone uwtable
+define dso_local i32 @bar(%"foo"* nocapture readnone %this) align 2 !type !0 {
+entry:
+  ret i32 0
+}
+
+!0 = !{i64 0, !"typeid1"}

diff --git a/test/LTO/Resolution/X86/appending-var.ll b/test/LTO/Resolution/X86/appending-var.ll
new file mode 100644 (file)
index 0000000..4d259d7
--- /dev/null
+++ b/test/LTO/Resolution/X86/appending-var.ll
@@ -0,0 +1,16 @@
+; Check we don't crash when linking a global variable with appending linkage
+; if the types in their elements don't have a straightforward mapping, forcing
+; us to use bitcasts.
+
+; RUN: opt %s -o %t1.o
+; RUN: opt %p/Inputs/appending-var-2.ll -o %t2.o
+
+; RUN: llvm-lto2 run -o %t3.o %t1.o %t2.o -r %t1.o,bar, -r %t2.o,bar,px
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%"foo.1" = type { i8, i8 }
+declare dso_local i32 @bar(%"foo.1"* nocapture readnone %this) local_unnamed_addr
+
+@llvm.used = appending global [1 x i8*] [i8* bitcast (i32 (%"foo.1"*)* @bar to i8*)], section "llvm.metadata"