This is an archive of the discontinued LLVM Phabricator instance.

Prevent crash when TurnSwitchRangeIntoICmp receives default unreachable destination
ClosedPublic

Authored by swamulism on Jun 13 2022, 9:17 PM.

Details

Summary

TurnSwitchRangeIntoICmp crashes when given a switch with a default destination of unreachable
Addresses issue #53208
https://github.com/llvm/llvm-project/issues/53208

Diff Detail

Event Timeline

swamulism created this revision.Jun 13 2022, 9:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 9:17 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
swamulism requested review of this revision.Jun 13 2022, 9:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 9:17 PM
swamulism added reviewers: hans, danielcdh.

can you put the bug number (e.g. #12345) in the description?

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
5190

destination

5222

this comment makes it sound like at this point your comment is always true

5223

LLVM style is no braces for a one line if

llvm/test/Transforms/SimplifyCFG/switch-default-undef.ll
1

can you use llvm/utils/update_test_checks.py?

hans added a comment.Jun 14 2022, 2:22 AM

Nice!

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
5222

Also, should it be "unreachable" rather than "undef"?

llvm/test/Transforms/SimplifyCFG/switch-default-undef.ll
1

Instead of creating a new file, could you add this as a test case to llvm/test/Transforms/SimplifyCFG/switch-range-to-icmp.ll instead?

9

In this switch, the default destination doesn't look unreachable. Is it made unreachable by some other transformation that happens before TurnSwitchRangeIntoICmp()? Can you make the test show the input that TurnSwitchRangeIntoICmp operates on directly?

Updating D127712: Prevent crash when TurnSwitchRangeIntoICmp recieves default null dest

swamulism retitled this revision from Prevent crash when TurnSwitchRangeIntoICmp recieves default null dest to Prevent crash when TurnSwitchRangeIntoICmp receives default unreachable destination.Jun 15 2022, 12:56 AM
swamulism edited the summary of this revision. (Show Details)
swamulism added inline comments.Jun 15 2022, 1:06 AM
llvm/test/Transforms/SimplifyCFG/switch-default-undef.ll
9

It is made unreachable by some other transformation.
Using the IR when the crash occurs in TurnSwitchRangeIntoICmp() as an input actually doesn't cause a crash (I assume it gets optimized out before we get here).
So, I guess there is some other combination of transformations that causes TurnSwitchRangeIntoICmp() to get a switch looking like this:

switch i8 %sw, label %unreachable [
  i8 10, label %group2
  i8 9, label %group2
]
nikic added a subscriber: nikic.Jun 15 2022, 1:17 AM

It looks like you only uploaded the diff to the previous version, rather than the whole patch.

swamulism updated this revision to Diff 437073.Jun 15 2022, 1:22 AM

Updating D127712: Prevent crash when TurnSwitchRangeIntoICmp receives default unreachable destination

swamulism added a comment.EditedJun 15 2022, 1:26 AM

It looks like you only uploaded the diff to the previous version, rather than the whole patch.

Oops. Figured out how to use arcanist haha (I think)
Thanks for the catch

swamulism updated this revision to Diff 437078.Jun 15 2022, 1:31 AM

Push whole patch

nikic added inline comments.Jun 15 2022, 2:06 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
5228

As this assert indicates, this case was supposed to be handled earlier already, namely by the ConstantFoldTerminator call. The reason it doesn't handle it is that https://github.com/llvm/llvm-project/blob/4c2bccfda3892ae13e97b6bfdbc99ec8cf5d095d/llvm/lib/Transforms/Utils/Local.cpp#L194-L198 will pick the unreachable block as the "only dest" and eliminate all unreachable cases. But that then leaves us with the actual only destination, which would get folded by a second call to ConstantFoldTerminator. Possibly the function should be improved to always find the non-unreachable dest.

But in any case, adding a conservative bailout here is reasonable, though the assertion message here is a bit outdated.

llvm/test/Transforms/SimplifyCFG/switch-range-to-icmp.ll
150 ↗(On Diff #437078)

Please give this function a name (like pr53208_single_reachable_dest or something).

update test fn name

swamulism added inline comments.Jun 15 2022, 10:38 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
5228

It looks like this is optimized after anyways, but yeah ideally it shouldn't get to this point in the first place.

This revision is now accepted and ready to land.Jun 16 2022, 12:55 AM
hans accepted this revision.Jun 16 2022, 5:23 AM

lgtm

Do you have commit access, or do you need someone to commit this for you?

lgtm

Do you have commit access, or do you need someone to commit this for you?

I need someone to commit this for me.
Thanks for reviewing!

This revision was landed with ongoing or failed builds.Jun 16 2022, 7:17 AM
This revision was automatically updated to reflect the committed changes.