This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Remove pointer from SmallPtrSet before deletion
ClosedPublic

Authored by NutshellySima on Aug 14 2018, 9:22 AM.

Details

Summary

Previously, eraseFromParent() calls delete which invalidates the value of the pointer. Copying the value of the pointer later is undefined behavior in C++11 and implementation-defined (which may cause a segfault on implementations having strict pointer safety) in C++14.

This patch removes the BasicBlock pointer from related SmallPtrSet before delete invalidates it in the SimplifyCFG pass.

Diff Detail

Event Timeline

NutshellySima created this revision.Aug 14 2018, 9:22 AM
dmgreen accepted this revision.Aug 14 2018, 9:35 AM

LGTM

I guess because it's only erasing it from a SmallPtrSet, it's not actually _using_ it, but this looks like a very sensible change.

This revision is now accepted and ready to land.Aug 14 2018, 9:35 AM

LGTM

I guess because it's only erasing it from a SmallPtrSet, it's not actually _using_ it, but this looks like a very sensible change.

Thanks for pointing it out. I didn't notice that. :)

NutshellySima retitled this revision from [SimplifyCFG] Fix BasicBlock use-after-free to [NFC][SimplifyCFG] Remove pointer from SmallPtrSet before deletion.Aug 14 2018, 9:54 AM
NutshellySima edited the summary of this revision. (Show Details)
kuhar added a comment.EditedAug 14 2018, 9:55 AM

I guess because it's only erasing it from a SmallPtrSet, it's not actually _using_ it, but this looks like a very sensible change.

I thought that erase calls delete on the link list node which invalidates not only the node itself but also the pointer, doesn't it?

I'm no expert, but I thought that SmallPtrSet->erase(BB) would treat BB as an opaque value that it removes from it's set, not accessing it's content in any way. You may be right about the eraseFromParent invalidating things though.

It's certainly much better this new way :)

I'm no expert, but I thought that SmallPtrSet->erase(BB) would treat BB as an opaque value that it removes from it's set, not accessing it's content in any way. You may be right about the eraseFromParent invalidating things though.

It's certainly much better this new way :)

I found this (C++14 [basic.stc.dynamic.deallocation]/4):

Indirection through an invalid pointer value and passing an invalid pointer value to a deallocation function have undefined behavior. Any other use of an invalid pointer value has implementation-defined behavior.

[ Footnote: Some implementations might define that copying an invalid pointer value causes a system-generated runtime fault. — end footnote ]

Seems like erasing from a set can cause both a copy (possibly a segfault) and produce unexpected values when used in comparison with other pointers.

kuhar accepted this revision.Aug 14 2018, 10:21 AM
NutshellySima retitled this revision from [NFC][SimplifyCFG] Remove pointer from SmallPtrSet before deletion to [SimplifyCFG] Remove pointer from SmallPtrSet before deletion.Aug 14 2018, 11:44 PM
NutshellySima edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.