[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
probinson added a comment. I now think the tactic to pursue is making sure the containing DICompositeType(s) for the method have complete descriptions, and RAUW the temporary nodes, prior to calling CloneFunction. Actually wading around in the MDNodes, Types, and Decls to accomplish that result promises to be "interesting" and "educational" (advice/suggestions welcome). https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
probinson added a comment. In https://reviews.llvm.org/D37038#89, @aprantl wrote: > This may have gotten lost earlier: Would it be possible to instruct > CloneFunction to not clone any temporary MDNodes via one of the flags that > are passed to the ValueMapper? I tried that. Basically, the new flag just disabled all the `isUniqued` assertions. What I found for test case 1 is that the DIE for CharlieImpl was duplicated, but there was only one copy of Charlie. This is, hmmm, less bad (I hesitate to say "better") than the original patch, which duplicated both CharlieImpl and Charlie. Obviously we'd rather not duplicate anything. https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
probinson added a comment. In https://reviews.llvm.org/D37038#89, @aprantl wrote: > This may have gotten lost earlier: Would it be possible to instruct > CloneFunction to not clone any temporary MDNodes via one of the flags that > are passed to the ValueMapper? I did look at that, sorry for not reporting back. ValueMapper seems to be quite adamant about having non-temporary nodes. I can try pursuing that further but it would be quite the "learning experience" I think. Alternatively we could try deferring the thunk cloning until after all other codegen, but that also seems like a pretty hacky approach. https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
aprantl added a comment. This may have gotten lost earlier: Would it be possible to instruct CloneFunction to not clone any temporary MDNodes via one of the flags that are passed to the ValueMapper? https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
probinson added a comment. Dumping the whole module, the temporary DICompositeType node for CBdVfsImpl is used both as the scope node for the DISubprogram, and also as the baseType of a DIDerivedType which is a pointer type in the type list for the subprogram. Given that the DICompositeType is a scope for the method, but is still marked as a forward declaration, I'm guessing that the composite type will be expanded into a full type, but that apparently happens after codegen for its methods. So, it would be premature to finalize the metadata node at this point? https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
probinson added a comment. In https://reviews.llvm.org/D37038#854722, @probinson wrote: > finalizeSubprogram() retrieves the variables from the subprogram and handles > them; what is it missing? For temp-md-nodes2.cpp, the assertion in mapTopLevelUniquedNode() trips on a DICompositeType for CBdVfsImpl. This appears to be the "scope" operand of the (distinct) DISubprogram for "ReqCacheHint" which is the function being cloned. For temp-md-nodes1.cpp, the assertion in visitOperands() trips on a DICompositeType for "Charlie" but I haven't worked out where it's used yet. Is there a straightforward way to dump all the metadata for a function? Calling `print()` or 'dump()` just does the one node. https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
probinson added a comment. Trying to understand the broader context here, I looked back through the list of revisions mentioned in PR33930 to see if that helped. When called on a method, CodeGenModule::EmitGlobalDefinition() calls CodeGenModule::EmitGlobalFunctionDefinition(), which in turn calls CodeGenFunction::GenerateCode(), which calls CodeGenFunction::FinishFunction(), which calls CGDebugInfo::EmitFunctionEnd(), which calls DIBuilder::finalizeSubprogram(). This is supposed to finalize the metadata for the subprogram. If the method is virtual, EmitGlobalDefinition() then calls getVTables().EmitThunks() which eventually gets to GenerateVarArgsThunk(). Which crashes when it tries to CloneFunction the original virtual method, because an operand of some piece of metadata is still temporary. So, either something happens such that EmitFunctionEnd() doesn't actually call finalizeSubprogram(), or finalizeSubprogram() doesn't finalize everything that it needs to finalize. finalizeSubprogram() retrieves the variables from the subprogram and handles them; what is it missing? https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
aprantl added inline comments. Comment at: lib/CodeGen/CGVTables.cpp:157 + if (DebugInfo) +DebugInfo->replaceTemporaryNodes(); + probinson wrote: > aprantl wrote: > > Have you looked at what it would take to only finalize the nodes reachable > > via the function? > > Otherwise — have you audited that this is always safe? > I do not know how to find nodes reachable from a particular function, either > before or after they are finalized. > > GenerateVarArgsThunk is called after we do EmitGlobalFunctionDefinition on > the function we are about to clone. The ReplaceMap holds replaceable forward > type declarations, I think? So I can imagine that codegen for a subsequent > function might need to create a complete type, even one that is reachable > from the function we are about to clone. > > Of course I find the whole metadata memory management scheme pretty opaque, > but this is my best guess about the situation. Alternatively, would it be possible to instruct CloneFunction to not clone any temporary MDNodes via one of the flags that are passed to ValueMapper? https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
probinson added inline comments. Comment at: lib/CodeGen/CGVTables.cpp:157 + if (DebugInfo) +DebugInfo->replaceTemporaryNodes(); + aprantl wrote: > Have you looked at what it would take to only finalize the nodes reachable > via the function? > Otherwise — have you audited that this is always safe? I do not know how to find nodes reachable from a particular function, either before or after they are finalized. GenerateVarArgsThunk is called after we do EmitGlobalFunctionDefinition on the function we are about to clone. The ReplaceMap holds replaceable forward type declarations, I think? So I can imagine that codegen for a subsequent function might need to create a complete type, even one that is reachable from the function we are about to clone. Of course I find the whole metadata memory management scheme pretty opaque, but this is my best guess about the situation. https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
aprantl added a comment. Performance-wise the change is fine because it does the same amount of work, but I would prefer someone to audit the code to make sure that we aren't uniquing a node while we still want to make changes to it. Unfortunately testcases for these issues will involve impossibly nested C++ so it may take a while to find a counterexample in the wild. That's why I'd rather read the code instead :-) https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
probinson added a comment. I mentioned this in the PR but I should have restated it here, sorry... Wolfgang authored this patch. He is away for a month so I volunteered to post this, in case it was okay for resolving the PR as the crash is present in the 5.0 branch. I do know Wolfgang was not really happy with it, and your questions might well be part of why that is. The patch has been in our local version of 5.0 for a couple of weeks without obvious problems, but that is not proof that it is always okay. https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
aprantl added inline comments. Comment at: lib/CodeGen/CGVTables.cpp:157 + if (DebugInfo) +DebugInfo->replaceTemporaryNodes(); + Have you looked at what it would take to only finalize the nodes reachable via the function? Otherwise — have you audited that this is always safe? https://reviews.llvm.org/D37038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning
probinson created this revision.
Make sure all temporary MD nodes have been replaced with uniqued or distinct
nodes before we clone a function.
Fixes PR33930.
https://reviews.llvm.org/D37038
Files:
lib/CodeGen/CGDebugInfo.cpp
lib/CodeGen/CGDebugInfo.h
lib/CodeGen/CGVTables.cpp
test/CodeGenCXX/temp-md-nodes1.cpp
test/CodeGenCXX/temp-md-nodes2.cpp
Index: test/CodeGenCXX/temp-md-nodes2.cpp
===
--- test/CodeGenCXX/temp-md-nodes2.cpp
+++ test/CodeGenCXX/temp-md-nodes2.cpp
@@ -0,0 +1,33 @@
+// REQUIRES: asserts
+// RUN: %clang_cc1 -O0 -triple %itanium_abi_triple -debug-info-kind=limited -S -emit-llvm %s -o - | \
+// RUN: FileCheck %s
+
+// This test simply checks that the varargs thunk is created. The failing test
+// case asserts.
+
+typedef signed char __int8_t;
+typedef int BOOL;
+class CMsgAgent;
+
+class CFs {
+public:
+ typedef enum {} CACHE_HINT;
+ virtual BOOL ReqCacheHint( CMsgAgent* p_ma, CACHE_HINT hint, ... ) ;
+};
+
+typedef struct {} _Lldiv_t;
+
+class CBdVfs {
+public:
+ virtual ~CBdVfs( ) {}
+};
+
+class CBdVfsImpl : public CBdVfs, public CFs {
+ BOOL ReqCacheHint( CMsgAgent* p_ma, CACHE_HINT hint, ... );
+};
+
+BOOL CBdVfsImpl::ReqCacheHint( CMsgAgent* p_ma, CACHE_HINT hint, ... ) {
+ return true;
+}
+
+// CHECK: define {{.*}} @_ZThn8_N10CBdVfsImpl12ReqCacheHintEP9CMsgAgentN3CFs10CACHE_HINTEz(
Index: test/CodeGenCXX/temp-md-nodes1.cpp
===
--- test/CodeGenCXX/temp-md-nodes1.cpp
+++ test/CodeGenCXX/temp-md-nodes1.cpp
@@ -0,0 +1,18 @@
+// REQUIRES: asserts
+// RUN: %clang_cc1 -O0 -triple %itanium_abi_triple -debug-info-kind=limited -S -emit-llvm %s -o - | \
+// RUN: FileCheck %s
+
+// This test simply checks that the varargs thunk is created. The failing test
+// case asserts.
+
+struct Alpha {
+ virtual void bravo(...);
+};
+struct Charlie {
+ virtual ~Charlie() {}
+};
+struct CharlieImpl : Charlie, Alpha {
+ void bravo(...) {}
+} delta;
+
+// CHECK: define {{.*}} void @_ZThn8_N11CharlieImpl5bravoEz(
Index: lib/CodeGen/CGVTables.cpp
===
--- lib/CodeGen/CGVTables.cpp
+++ lib/CodeGen/CGVTables.cpp
@@ -152,6 +152,10 @@
llvm::Value *Callee = CGM.GetAddrOfFunction(GD, Ty, /*ForVTable=*/true);
llvm::Function *BaseFn = cast(Callee);
+ // Ensure we don't have any temporary MD nodes before we clone the function.
+ if (DebugInfo)
+DebugInfo->replaceTemporaryNodes();
+
// Clone to thunk.
llvm::ValueToValueMapTy VMap;
llvm::Function *NewFn = llvm::CloneFunction(BaseFn, VMap);
Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -440,6 +440,7 @@
void completeTemplateDefinition(const ClassTemplateSpecializationDecl &SD);
void completeUnusedClass(const CXXRecordDecl &D);
+ void replaceTemporaryNodes();
/// Create debug info for a macro defined by a #define directive or a macro
/// undefined by a #undef directive.
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -4091,18 +4091,7 @@
TheCU->setDWOId(Signature);
}
-
-void CGDebugInfo::finalize() {
- // Creating types might create further types - invalidating the current
- // element and the size(), so don't cache/reference them.
- for (size_t i = 0; i != ObjCInterfaceCache.size(); ++i) {
-ObjCInterfaceCacheEntry E = ObjCInterfaceCache[i];
-llvm::DIType *Ty = E.Type->getDecl()->getDefinition()
- ? CreateTypeDefinition(E.Type, E.Unit)
- : E.Decl;
-DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
- }
-
+void CGDebugInfo::replaceTemporaryNodes() {
for (auto p : ReplaceMap) {
assert(p.second);
auto *Ty = cast(p.second);
@@ -4115,6 +4104,21 @@
DBuilder.replaceTemporary(llvm::TempDIType(Ty),
cast(it->second));
}
+ ReplaceMap.clear();
+}
+
+void CGDebugInfo::finalize() {
+ // Creating types might create further types - invalidating the current
+ // element and the size(), so don't cache/reference them.
+ for (size_t i = 0; i != ObjCInterfaceCache.size(); ++i) {
+ObjCInterfaceCacheEntry E = ObjCInterfaceCache[i];
+llvm::DIType *Ty = E.Type->getDecl()->getDefinition()
+ ? CreateTypeDefinition(E.Type, E.Unit)
+ : E.Decl;
+DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
+ }
+
+ replaceTemporaryNodes();
for (const auto &p : FwdDeclReplaceMap) {
assert(p.second);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
