Page MenuHomePhabricator

Fix constexpr __builtin_*_overflow issue when unsigned->signed operand.
ClosedPublic

Authored by erichkeane on May 30 2019, 7:33 AM.

Details

Summary

As reported here https://bugs.llvm.org/show_bug.cgi?id=42000, it was
possible to get the constexpr version of __builtin_*_overflow to give
the wrong answer.

This was because when extending the operands to fit the largest type (so
that the math could be done), the decision on whether to sign/zero
extend the operands was based on the result signedness, not on the
operands signedness.

In the reported case, (unsigned char)255 - (int)100 needed
to have each extended to the int in order to do the math. However, when
extending the first operand to 'int', we incorrectly sign extended it
instead of zero extending. Thus, the result didnt fit back into the
unsigned char.

The fix for this was simply to choose zero/sign extension based on the
sign of the operand itself.

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane created this revision.May 30 2019, 7:33 AM
efriedma added inline comments.May 30 2019, 11:49 AM
clang/lib/AST/ExprConstant.cpp
9457 ↗(On Diff #202189)

Can you just write LHS = APSInt(LHS.extOrTrunc(MaxBits), !IsSigned);, or something like that?

erichkeane marked an inline comment as done.May 30 2019, 11:52 AM
erichkeane added inline comments.
clang/lib/AST/ExprConstant.cpp
9457 ↗(On Diff #202189)

I believe I can! I was unaware of this function.

erichkeane marked an inline comment as done.
This revision is now accepted and ready to land.May 30 2019, 1:49 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 2:35 PM