Open Quuxplusone opened 6 years ago
Quuxplusone
commented
6 years ago | Bugzilla Link | PR37685 |
| Status | NEW |
| Importance | P enhancement |
| Reported by | Vlad Tsyrklevich (vlad@tsyrklevich.net) |
| Reported on | 2018-06-04 19:13:42 -0700 |
| Last modified on | 2018-06-08 17:47:53 -0700 |
| Version | trunk |
| Hardware | PC All |
| CC | aprantl@apple.com, kcc@google.com, llvm-bugs@lists.llvm.org, rtereshin@apple.com |
| Fixed by commit(s) | |
| Attachments | |
| Blocks | |
| Blocked by | |
| See also |
Quuxplusone
commented
6 years ago Brief look at this:
- bugpoint calls CloneModule()
- CloneModule() calls CloneFunction()
- CloneFunction() enumerates DIs referenced by the functions and sets them to
map to themselves in the ValueToValueMapTy metadata map (e.g. they won't need
to be duplicated later.)
- CloneModule calls MapMetadata to copy metadata
- mapSimpleMetadata uses the cached values set by CloneFunction() above instead
of recursing into the metadata and creating a copy of the ConstantAsMetadata MD
node.
((At this point the cloned module has a reference to the old module through the
metadata reference))
- BitcodeWriter calls ValueEnumerator. It enumerates all GVs, then all
constants. However, because the metadata points to a function in another
module, the old module's function is inserted at the end of values array with
the constants and hence the failure below.
I'm not sure what the correct way to handle the DI values is in CloneFunction,
for now the following patch fixes it locally and allows me to move forward on
PR37684:
$ git diff
diff --git a/lib/Transforms/Utils/CloneFunction.cpp
b/lib/Transforms/Utils/CloneFunction.cpp
index 956514db42..ad23b631c9 100644
--- a/lib/Transforms/Utils/CloneFunction.cpp
+++ b/lib/Transforms/Utils/CloneFunction.cpp
@@ -126,6 +126,7 @@ void llvm::CloneFunctionInto(Function *NewFunc, const
Function *OldFunc,
bool MustCloneSP =
OldFunc->getParent() && OldFunc->getParent() == NewFunc->getParent();
DISubprogram *SP = OldFunc->getSubprogram();
+#if 0
if (SP) {
assert(!MustCloneSP || ModuleLevelChanges);
// Add mappings for some DebugInfo nodes that we don't want duplicated
@@ -139,6 +140,7 @@ void llvm::CloneFunctionInto(Function *NewFunc, const
Function *OldFunc,
if (!MustCloneSP)
MD[SP].reset(SP);
}
+#endif
SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
OldFunc->getAllMetadata(MDs);
@@ -189,6 +191,7 @@ void llvm::CloneFunctionInto(Function *NewFunc, const
Function *OldFunc,
Returns.push_back(RI);
}
+#if 0
for (DISubprogram *ISP : DIFinder.subprograms())
if (ISP != SP)
VMap.MD()[ISP].reset(ISP);
@@ -198,6 +201,7 @@ void llvm::CloneFunctionInto(Function *NewFunc, const
Function *OldFunc,
for (DIType *Type : DIFinder.types())
VMap.MD()[Type].reset(Type);
+#endif
// Loop over all of the instructions in the function, fixing up operand
// references as we go. This uses VMap to do all the hard work.
Here's another failing input that required a different #if 0 in the patch above
from the first reproducer:
$ cat IOHandler.cpp.o.ll
%"c1" = type {}
%"c2" = type {}
define void @a(%"c1"* , %"c2"* ) !dbg !160 {
unreachable
}
!llvm.module.flags = !{!4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!27 = !DICompositeType(tag: 1, elements: !29)
!29 = !{!104}
!104 = !DITemplateValueParameter(value: void (%"c1"*, %"c2"*)* @a)
!160 = !DISubprogram(scope: !27, isDefinition: false)Looked at the git history and bisected this a bit to find that this regression was introduced in https://reviews.llvm.org/D45593 (r330069)
Quuxplusone
commented
6 years ago Another way to reproduce the problem is `./bin/opt -run-twice -o /dev/null
test.ll`. If dumped as *.ll-file (`-S`) opt finishes fine.A bit simplified / canonicalized test case:
define void @a() !dbg !7 {
unreachable
}
!llvm.module.flags = !{!0}
!llvm.dbg.cu = !{!1}
!0 = !{i32 2, !"Debug Info Version", i32 3}
!1 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !2,
isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug, retainedTypes: !3)
!2 = !DIFile(filename: "ASTContext.cpp", directory: "")
!3 = !{!4}
!4 = !DICompositeType(tag: DW_TAG_class_type, file: !2, templateParams: !5)
!5 = !{!6}
!6 = !DITemplateValueParameter(value: void ()* @a)
!7 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true,
isOptimized: false, unit: !1)I've added an IR Verifier check that follows:
diff --git a/lib/IR/Verifier.cpp b/lib/IR/Verifier.cpp
index dca9d9b64ff..bdee07d00f0 100644
--- a/lib/IR/Verifier.cpp
+++ b/lib/IR/Verifier.cpp
@@ -806,10 +806,15 @@ void Verifier::visitMDNode(const MDNode &MD) {
void Verifier::visitValueAsMetadata(const ValueAsMetadata &MD, Function *F) {
Assert(MD.getValue(), "Expected valid value", &MD);
Assert(!MD.getValue()->getType()->isMetadataTy(),
"Unexpected metadata round-trip through values", &MD, MD.getValue());
+ if (auto *GV = dyn_cast<GlobalValue>(MD.getValue())) {
+ Module *ActualM = GV->getParent();
+ Assert(ActualM == &M, "module-local metadata used in wrong module", &MD);
+ }
+
auto *L = dyn_cast<LocalAsMetadata>(&MD);
if (!L)
return;
Assert(F, "function-local metadata used outside a function", L);(https://reviews.llvm.org/D47969)
and hit another interesting case:
llvm/test/ThinLTO/X86/dicompositetype-unique2.ll fails on the 3rd RUN-line (llvm-lto --thinlto-action=run %t1.bc %t2.bc -thinlto-save-temps=%t3.) with the following (partial) stack trace:
frame #11: 0x0000000100df4c5f llvm-lto`llvm::verifyModule(M=0x00000001077017c0, OS=0x00000001039aba08, BrokenDebugInfo=0x0000700002f56d4b) at Verifier.cpp:4672
frame #12: 0x0000000100b448f3 llvm-lto`llvm::UpgradeDebugInfo(M=0x00000001077017c0) at AutoUpgrade.cpp:3099
frame #13: 0x000000010221037c llvm-lto`llvm::FunctionImporter::importFunctions(this=0x0000700002f57d80, DestModule=0x0000000107700f00, ImportList=0x000000010750b9c8) at FunctionImport.cpp:970
frame #14: 0x0000000100ee52b3 llvm-lto`(anonymous namespace)::crossImportIntoModule(TheModule=0x0000000107700f00, Index=0x000000010750bd50, ModuleMap=0x00007ffeefbfb8f0, ImportList=0x000000010750b9c8) at ThinLTOCodeGenerator.cpp:202
frame #15: 0x0000000100f03d82 llvm-lto`(anonymous namespace)::ProcessThinLTOModule(TheModule=0x0000000107700f00, Index=0x000000010750bd50, ModuleMap=0x00007ffeefbfb8f0, TM=0x0000000107306be0, ImportList=0x000000010750b9c8, ExportList=size=0, GUIDPreservedSymbols=0x00007ffeefbfb8a8, DefinedGlobals=0x000000010750b888, CacheOptions=0x00007ffeefbff608, DisableCodeGen=false, SaveTempsDir=(Data = "/Volumes/Data/llvm/build/obj/test/ThinLTO/X86/Output/dicompositetype-unique2.ll.tmp3", Length = 84), Freestanding=false, OptLevel=3, count=0) at ThinLTOCodeGenerator.cpp:460
frame #16: 0x0000000100f022d6 llvm-lto`llvm::ThinLTOCodeGenerator::run(this=0x000000010750dd88, count=0)::$_2::operator()(int) const at ThinLTOCodeGenerator.cpp:1016That is an independent problem as CloneFunctionInto isn't used by this llvm-lto run at all, though it's quite similar: it would be !DITemplateValueParameter(name: "F", type: !24, value: %struct.S () @_Z3Getv) referenced from a DISubprogram associated with a function belonging to a module M1, while the global value referenced by DITemplateValueParameter itself would belong to a different module M2.