This is an archive of the discontinued LLVM Phabricator instance.

[NFC][CLANG] Fix Static Code Analyzer Concerns with bad bit right shift operation in getNVPTXLaneID()
ClosedPublic

Authored by Manna on May 26 2023, 8:36 PM.

Details

Summary

In getNVPTXLaneID(CodeGenFunction &), the value of LaneIDBits is 4294967295 since function call llvm::Log2_32(CGF->getTarget()->getGridValue().GV_Warp_Size) might return 4294967295.

unsigned LaneIDBits =
     llvm::Log2_32(CGF.getTarget().getGridValue().GV_Warp_Size);
unsigned LaneIDMask = ~0u >> (32u - LaneIDBits);

The shift amount (32U - LaneIDBits) might be 33, So right shifting by more than 31 bits will have undefined behavior.

This patch adds an assert to guard the LaneIDBits overflow issue for valid LaneIDMask value.

Diff Detail

Event Timeline

Manna created this revision.May 26 2023, 8:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 8:36 PM
Manna requested review of this revision.May 26 2023, 8:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 8:36 PM
Manna edited the summary of this revision. (Show Details)May 26 2023, 8:37 PM
Manna updated this revision to Diff 526259.May 27 2023, 5:52 AM

I have updated patch.

Manna edited the summary of this revision. (Show Details)May 27 2023, 6:00 AM
Manna edited the summary of this revision. (Show Details)
tra added a subscriber: tra.May 30 2023, 9:37 AM

In practice we're guaranteed by GPU architecture that the warp size will always be small enough to fit in 32 bits.

Also log2_32 will never return a value larger than 32.

Does this assert help with anything else other than potential undefined behavior?

tahonermann accepted this revision.Jun 22 2023, 11:14 AM

The static analysis results look suspect since there does not appear to be a way for Log2_32 to return a value larger than 31. Regardless, the change looks safe to me; GV_Warp_Size should always be greater than 0, so Log2_32 should always return a non-negative number that is less than 32.

This revision is now accepted and ready to land.Jun 22 2023, 11:14 AM
Manna added a comment.Jun 22 2023, 1:22 PM

Thank you @tahonermann and @tra for reviews and comments!