From 6dc53ccb0b0900a09d4a49a5af75e307ced5fc17 Mon Sep 17 00:00:00 2001 From: Rafael Auler Date: Wed, 20 Mar 2019 19:20:07 +0000 Subject: [PATCH] [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 llvm-svn: 356597 --- llvm/lib/Linker/IRMover.cpp | 35 ++++++++++++++++--- .../Resolution/X86/Inputs/appending-var-2.ll | 14 ++++++++ llvm/test/LTO/Resolution/X86/appending-var.ll | 16 +++++++++ 3 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 llvm/test/LTO/Resolution/X86/Inputs/appending-var-2.ll create mode 100644 llvm/test/LTO/Resolution/X86/appending-var.ll diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp index 70d5262fd618..b21b8f33ab42 100644 --- a/llvm/lib/Linker/IRMover.cpp +++ b/llvm/lib/Linker/IRMover.cpp @@ -402,6 +402,7 @@ class IRLinker { DenseSet ValuesToLink; std::vector Worklist; + std::vector> 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 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/llvm/test/LTO/Resolution/X86/Inputs/appending-var-2.ll b/llvm/test/LTO/Resolution/X86/Inputs/appending-var-2.ll new file mode 100644 index 000000000000..29bc8433d3e4 --- /dev/null +++ b/llvm/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/llvm/test/LTO/Resolution/X86/appending-var.ll b/llvm/test/LTO/Resolution/X86/appending-var.ll new file mode 100644 index 000000000000..4d259d7f8db7 --- /dev/null +++ b/llvm/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"