Page MenuHomePhabricator

[SelectionDAG] Assert that operands to SelectionDAG::getNode are not DELETED_NODE to catch issues like PR49393 earlier.
ClosedPublic

Authored by craig.topper on Mar 4 2021, 11:37 AM.

Details

Summary

I'm not sure this would catch all such issues, but it would catch some.

The problem for PR49393 was that we were holding a reference to a node that
wasn't connect edto the DAG across a function that could delete unused nodes. In
this particular case we managed to try to use the deleted node while it was in
the deleted state before its memory got recycled.

It could also happen that we delete the node, something allocates a new node
which recycles the memory. Then we try to use the reference we were holding and
it is now a completely different node with different valid opcode. This patch
would not catch that.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 4 2021, 11:37 AM
craig.topper requested review of this revision.Mar 4 2021, 11:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 11:37 AM
Herald added a subscriber: wdng. · View Herald Transcript

Won't be SDValue::Node deallocated by then, or worse, potentially reallocated?
I guess that is still fine because it would trip ASAN?

craig.topper edited the summary of this revision. (Show Details)Mar 4 2021, 11:47 AM

Won't be SDValue::Node deallocated by then, or worse, potentially reallocated?
I guess that is still fine because it would trip ASAN?

When an SDNode is deallocated it gets remembered in a free list in the RecylingAllocator. The opcode field had __asan_unpoison_memory_region called on it. See SelectionDAG::DeallocateNode

spatel accepted this revision.Mar 4 2021, 1:26 PM

LGTM

This revision is now accepted and ready to land.Mar 4 2021, 1:26 PM

Won't be SDValue::Node deallocated by then, or worse, potentially reallocated?
I guess that is still fine because it would trip ASAN?

When an SDNode is deallocated it gets remembered in a free list in the RecylingAllocator. The opcode field had __asan_unpoison_memory_region called on it. See SelectionDAG::DeallocateNode

SGTM

This revision was landed with ongoing or failed builds.Mar 4 2021, 11:17 PM
This revision was automatically updated to reflect the committed changes.