This is an archive of the discontinued LLVM Phabricator instance.

[GISel]: Fix more Undefined behavior in GlobalISel::IRTranslator
ClosedPublic

Authored by aditya_nandakumar on May 15 2017, 5:58 PM.

Details

Summary

Turns out the MachineIRBuilder holds on references to TrackingMDRef (and IRTranslator has MachineIRBuilder objects). When the context gets destroyed, first all the MDRefs are deallocated and then the passes are destroyed. When the IRTranslator is about to get destroyed, it's destructor would end up trying to destroy the trackingMDRefs which the builders held on to leading to Undefined behavior.
Fix this by explicitly default constructing the MachineIRBuidlers at the end of runOnMachineFunction.

Diff Detail

Event Timeline

dsanders edited edge metadata.May 16 2017, 4:13 AM

Is it possible to add a test case for this? It seems that a test where the last instruction has a DILocation should trigger it.

lib/CodeGen/GlobalISel/IRTranslator.cpp
1132–1140

Based on our discussion off-list it was my understanding that ~IRTranslator() wasn't the only trigger for this. Wasn't the call to MachineIRBuilder::setMF() in IRTranslator::runOnMachineFunction() also triggering it?

If so, I'd suggest adapting this comment to talk about the MachineIRBuilder/DebugLoc outliving the DILocation it holds rather than going into detail about the LLVMContext case. That way it will cover both.

Updated comment based on Daniel's feedback.

dsanders accepted this revision.May 17 2017, 2:55 AM

LGTM

lib/CodeGen/GlobalISel/IRTranslator.cpp
1132–1134

One tiny nit: Doxygen comments don't do anything here as far as I know so these should use //

This revision is now accepted and ready to land.May 17 2017, 2:55 AM
aditya_nandakumar marked an inline comment as done.

Submitted in r303277