Make sure all temporary MD nodes have been replaced with uniqued or distinct nodes before we clone a function.
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.
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 :-)
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.
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?
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.
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?
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.
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.
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).