Page MenuHomePhabricator

[AST] Fix handling of long double and bool in __builtin_bit_cast
AcceptedPublic

Authored by erik.pilkington on Mar 17 2020, 2:09 PM.

Details

Reviewers
rsmith
ldionne
jfb
Summary

On x86, long double has 6 unused trailing bytes. This patch changes the constant evaluator to treat them as though they were padding bytes, so reading from them results in an indeterminate value, and nothing is written for them. Also, fix a similar bug with bool, but instead of treating the unused bits as padding, enforce that they're zero.

Fixes a bug Louis pointed out in: https://reviews.llvm.org/D75960

Diff Detail

Event Timeline

ldionne accepted this revision.Mar 20 2020, 8:23 AM

LGTM at high level, but I'm not really familiar with the code.

clang/lib/AST/ExprConstant.cpp
6365

semanticsSizeInBits is the number of bits actually used in the type?

This revision is now accepted and ready to land.Mar 20 2020, 8:23 AM
jfb added a comment.Mar 20 2020, 9:15 AM

Maybe you should test nullptr as well, given that it's all padding?

clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
407

Sanity-check: 0 and 1 work? IIRC we had a discussion about whether false was 0 and true was 1, and we concluded that false was indeed 0 but true wasn't specified to be 1.

erik.pilkington marked 5 inline comments as done.

Add tests for bit_cast<bool>((char)1);

clang/lib/AST/ExprConstant.cpp
6365

Yeah

clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
226–250
In D76323#1933791, @jfb wrote:

Maybe you should test nullptr as well, given that it's all padding?

There are some existing nullptr padding tests here ^^^, which seem to cover this.

407

Yeah, they work, I added a test in the update. I don't think it really matters that true could technically be 2 in the standard here, since the representation on our implementation is always 1.

jfb accepted this revision.Mar 23 2020, 3:16 PM
rsmith added inline comments.Mar 23 2020, 4:06 PM
clang/lib/AST/ExprConstant.cpp
6364–6371

This appears more complex than necessary. bitcastToAPInt already returned the correct number of bits (eg, 80 for x86 fp80) so you don't need to compute that yourself. How about leaving this function alone and changing visitInt to store the number of (value representation) bits in the APSInt rather than the full (object representation) width of the type? As is, visitInt would also be wrong in the same way if the integer type had any full bytes of padding.

6462

If you want this to be a general check for integer types with padding, you should zext or sext as appropriate for the signedness of the truncated value. (Eg, you could set the signedness of Truncated to match the integer type, and use extend rather than zext here.)

erik.pilkington marked 5 inline comments as done.

Address review comments.

clang/lib/AST/ExprConstant.cpp
6364–6371

Sure, this causes us to have to handle bool in visitInt, but I think we should have been doing that anyways.