Page MenuHomePhabricator

[ubsan] Detect signed overflow UB in remainder operations
ClosedPublic

Authored by vsk on Feb 1 2017, 6:54 PM.

Details

Summary

Teach ubsan to diagnose remainder operations which have undefined behavior due to signed overflow.

My copy of the C11 spec draft (6.5.5.6) says that: if the quotient a/b is representable, (a/b)*b + a%b shall equal a; otherwise, the behavior of both a/b and a%b is undefined. I take this to mean INT_MIN % -1 has signed overflow UB, so we should check for that.

Loosely depends on: https://reviews.llvm.org/D29369

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Feb 1 2017, 6:54 PM
regehr edited edge metadata.Feb 1 2017, 9:10 PM

Does this check need to be sensitive to the dialect of C/C++ that the user asked for? I know that it used to be the case that the standard could be read either way for this case, but as you observe it is now unambiguously UB.

vsk added a comment.Feb 2 2017, 12:25 AM

Does this check need to be sensitive to the dialect of C/C++ that the user asked for? I know that it used to be the case that the standard could be read either way for this case, but as you observe it is now unambiguously UB.

No, I don't think the check should be sensitive to language dialect.

You're right about the C99 spec not being very explicit about what 'a % b' means if 'a / b' is not representable. It's simply the "remainder" of 'a / b'. However, clang treats 'INT_MIN % -1' as having UB in C99 mode (rightly, imho), so it'd be nice to have a diagnostic for it.

efriedma added inline comments.Feb 24 2017, 11:51 AM
lib/CodeGen/CGExprScalar.cpp
2380 ↗(On Diff #86759)

I don't think you need the isIntegerType check here?

vsk updated this revision to Diff 89734.Feb 24 2017, 3:20 PM
  • Add a small test that shows why the 'isIntegerType' check is required (we'd crash otherwise).
efriedma accepted this revision.Feb 24 2017, 3:44 PM

LGTM.

This revision is now accepted and ready to land.Feb 24 2017, 3:44 PM
This revision was automatically updated to reflect the committed changes.