Page MenuHomePhabricator

Replace temp MD nodes with unique/distinct before cloning
Needs ReviewPublic

Authored by probinson on Aug 22 2017, 4:26 PM.

Details

Reviewers
aprantl
dblaikie
Summary

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

Fixes PR33930.

Diff Detail

Event Timeline

probinson created this revision.Aug 22 2017, 4:26 PM
aprantl added inline comments.Aug 22 2017, 4:34 PM
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?

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 edited edge metadata.Aug 22 2017, 5:10 PM

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 added inline comments.Aug 25 2017, 4:58 PM
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 added inline comments.Aug 28 2017, 11:48 AM
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?

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 added a comment.EditedAug 28 2017, 4:56 PM

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?

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?

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.

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.

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