Replace temp MD nodes with unique/distinct before cloning

[probinson]

Summary

Make sure all temporary MD nodes have been replaced with uniqued or distinct nodes before we clone a function.

Fixes PR33930.

[aprantl]

lib/CodeGen/CGVTables.cpp

157

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?

[probinson]

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.

[aprantl]

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 :-)

[probinson]

lib/CodeGen/CGVTables.cpp

157

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.

[aprantl]

lib/CodeGen/CGVTables.cpp

157

Alternatively, would it be possible to instruct CloneFunction to not clone any temporary MDNodes via one of the flags that are passed to ValueMapper?

[probinson]

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?

[probinson]

In 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.

[probinson]

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?

[aprantl]

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?

[probinson]

In D37038#855559, @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.

[probinson]

In D37038#855559, @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.

[probinson]

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).

lib/CodeGen/CGDebugInfo.h

public:
   void completeType(const EnumDecl *ED);
   void completeType(const RecordDecl *RD);
   void completeRequiredType(const RecordDecl *RD);
   void completeClassData(const RecordDecl *RD);
   void completeClass(const RecordDecl *RD);
 
   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.
   llvm::DIMacro *CreateMacro(llvm::DIMacroFile *Parent, unsigned MType,
                              SourceLocation LineLoc, StringRef Name,
                              StringRef Value);
 
   /// Create debug info for a file referenced by an #include directive.

lib/CodeGen/CGDebugInfo.cpp

CGDebugInfo::getOrCreateNamespace(const NamespaceDecl *NSDecl) {
   return NS;
 }
 
 void CGDebugInfo::setDwoId(uint64_t Signature) {
   assert(TheCU && "no main compile unit");
   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<llvm::DIType>(p.second);
     assert(Ty->isForwardDecl());
 
     auto it = TypeCache.find(p.first);
     assert(it != TypeCache.end());
     assert(it->second);
 
     DBuilder.replaceTemporary(llvm::TempDIType(Ty),
                               cast<llvm::DIType>(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);
     llvm::TempMDNode FwdDecl(cast<llvm::MDNode>(p.second));
     llvm::Metadata *Repl;
 
     auto it = DeclCache.find(p.first);
     // If there has been no definition for the declaration, call RAUW

lib/CodeGen/CGVTables.cpp

CodeGenFunction::GenerateVarArgsThunk(llvm::Function *Fn,
   QualType ResultType = FPT->getReturnType();
 
   // Get the original function
   assert(FnInfo.isVariadic());
   llvm::Type *Ty = CGM.getTypes().GetFunctionType(FnInfo);
   llvm::Value *Callee = CGM.GetAddrOfFunction(GD, Ty, /*ForVTable=*/true);
   llvm::Function *BaseFn = cast<llvm::Function>(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);
   Fn->replaceAllUsesWith(NewFn);
   NewFn->takeName(Fn);
   Fn->eraseFromParent();
   Fn = NewFn;
 

test/CodeGenCXX/temp-md-nodes1.cpp

+// 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(

test/CodeGenCXX/temp-md-nodes2.cpp

+// 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(