Page MenuHomePhabricator

Add __builtin_signbit semantic checking
ClosedPublic

Authored by aaron.ballman on May 27 2018, 2:12 PM.

Details

Summary

r242675 changed the signature for the signbit builtin but did not introduce proper semantic checking to ensure the arguments are as-expected. This lead to regressions like the one reported in PR28172 where codegen would crash. This patch addresses this by properly grouping the signbit builtins along with the other fp classification builtins.

Diff Detail

Event Timeline

aaron.ballman created this revision.May 27 2018, 2:12 PM
aaron.ballman added inline comments.
test/Sema/builtins.c
228

I'll revert this sneaky whitespace either as part of the commit, or as an updated patch if one is required.

The checking by itself looks sane-ish, but i don't have any reasonable knowledge in this area.

lib/Sema/SemaChecking.cpp
925

The name of the function is unfortunate given that you call it for __builtin_signbit(int).

test/Sema/builtins.c
260

What about

(void)__builtin_signbit(1.0);
260

Hm, is __builtin_signbit() taking an integer, or float?
If integer, the comment about floating point classification is slightly misleading.

264

1 will be promoted to float?
I'd add one more test with native float:

(void)__builtin_signbit(1.0);
268

Same, would be good to see __builtin_signbitf(1.0);, __builtin_signbitf(1.0L);.

aaron.ballman marked 5 inline comments as done.

Updating based on review feedback.

lib/Sema/SemaChecking.cpp
925

signbit() is an FP classification macro (see C17 7.12.3 "Classification macros"), so I don't think the name is unfortunate.

test/Sema/builtins.c
260

It accepts a floating-point type.

268

It turns out that double and long double were fine, but the promotion from float for this case was a problem. I fixed that as well.

lebedev.ri accepted this revision.Jun 12 2018, 9:30 AM

Looks good to me, but you probably want a bit for a second opinion.

This revision is now accepted and ready to land.Jun 12 2018, 9:30 AM
aaron.ballman closed this revision.EditedJun 19 2018, 7:40 AM

I've committed in r335048; if @rsmith has concerns, they can be addressed post commit.