This is an archive of the discontinued LLVM Phabricator instance.

[ARM] IselLowering unsigned overflow to crash using APInt in PerformSHLSimplify
ClosedPublic

Authored by Peter on Dec 2 2022, 5:06 PM.

Details

Summary

This diff fixes issue https://github.com/llvm/llvm-project/issues/59317

We should check if bitwidth is lower than the shift amount before we subtract them to avoid unsigned overflow.

Diff Detail

Event Timeline

Peter created this revision.Dec 2 2022, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 5:06 PM
Peter requested review of this revision.Dec 2 2022, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 5:06 PM
RKSimon added a subscriber: RKSimon.Dec 3 2022, 3:15 AM
RKSimon added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
13778

I'm not sure if you can encounter types > 64 bits here - but you might be better doing the test as

if (C2Int.uge(C2Width))
  return SDValue();
uint64_t C2Value = C2Int.getZExtValue();
Peter updated this revision to Diff 480167.Dec 5 2022, 11:05 AM
Peter marked an inline comment as done.

Update and remove getZExtValue()

llvm/lib/Target/ARM/ARMISelLowering.cpp
13778

I 100% agree with you. I meant to change that in another patch as it could be some other issue, but I'll just do it here.
However, I couldn't find a file that can crash it, probably legalizer has already split i128 into smaller types prior to this, so I'll just update the code without new tests.

getZExtValue and getSExtValue in APInt can be particularly problematic, I see people use it without checking if it is allowed and crash the compiler.
I have found another bug with it that actually fits your concern. (https://github.com/llvm/llvm-project/issues/59316)
Probably we should move this discussion to there.

dmgreen accepted this revision.Dec 6 2022, 12:22 AM
dmgreen added a subscriber: dmgreen.

The change sounds good to me, LGTM.

This revision is now accepted and ready to land.Dec 6 2022, 12:22 AM