Add __builtin_signbit semantic checking

[aaron.ballman]

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.

[aaron.ballman]

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.

[aaron.ballman]

Ping

[lebedev.ri]

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]

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]

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

[aaron.ballman]

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

lib/Sema/SemaChecking.cpp

   }
 
   // The acquire, release, and no fence variants are ARM and AArch64 only.
   case Builtin::BI_interlockedbittestandset_acq:
   case Builtin::BI_interlockedbittestandset_rel:
   case Builtin::BI_interlockedbittestandset_nf:
   case Builtin::BI_interlockedbittestandreset_acq:
   case Builtin::BI_interlockedbittestandreset_rel:
   case Builtin::BI_interlockedbittestandreset_nf:
     if (CheckBuiltinTargetSupport(
             *this, BuiltinID, TheCall,
             {llvm::Triple::arm, llvm::Triple::thumb, llvm::Triple::aarch64}))
       return ExprError();
     break;
 
   // The 64-bit bittest variants are x64, ARM, and AArch64 only.
   case Builtin::BI_bittest64:

     if (SemaBuiltinFPClassification(TheCall, 6))
       return ExprError();
     break;
   case Builtin::BI__builtin_isfinite:
   case Builtin::BI__builtin_isinf:
   case Builtin::BI__builtin_isinf_sign:
   case Builtin::BI__builtin_isnan:
   case Builtin::BI__builtin_isnormal:
+  case Builtin::BI__builtin_signbit:
+  case Builtin::BI__builtin_signbitf:
+  case Builtin::BI__builtin_signbitl:
     if (SemaBuiltinFPClassification(TheCall, 1))
       return ExprError();
     break;
   case Builtin::BI__builtin_shufflevector:
     return SemaBuiltinShuffleVector(TheCall);
     // TheCall will be freed by the smart pointer here, but that's fine, since
     // SemaBuiltinShuffleVector guts it, but then doesn't release it.
   case Builtin::BI__builtin_prefetch:

     return false;
 
   // This operation requires a non-_Complex floating-point number.
   if (!OrigArg->getType()->isRealFloatingType())
     return Diag(OrigArg->getLocStart(),
                 diag::err_typecheck_call_invalid_unary_fp)
       << OrigArg->getType() << OrigArg->getSourceRange();
 
-  // If this is an implicit conversion from float -> float or double, remove it.
+  // If this is an implicit conversion from float -> float, double, or
+  // long double, remove it.
   if (ImplicitCastExpr *Cast = dyn_cast<ImplicitCastExpr>(OrigArg)) {
     // Only remove standard FloatCasts, leaving other casts inplace
     if (Cast->getCastKind() == CK_FloatingCast) {
       Expr *CastArg = Cast->getSubExpr();
       if (CastArg->getType()->isSpecificBuiltinType(BuiltinType::Float)) {
-          assert((Cast->getType()->isSpecificBuiltinType(BuiltinType::Double) ||
-                  Cast->getType()->isSpecificBuiltinType(BuiltinType::Float)) &&
-               "promotion from float to either float or double is the only expected cast here");
+        assert(
+            (Cast->getType()->isSpecificBuiltinType(BuiltinType::Double) ||
+             Cast->getType()->isSpecificBuiltinType(BuiltinType::Float) ||
+             Cast->getType()->isSpecificBuiltinType(BuiltinType::LongDouble)) &&
+            "promotion from float to either float, double, or long double is "
+            "the only expected cast here");
         Cast->setSubExpr(nullptr);
         TheCall->setArg(NumArgs-1, CastArg);
       }
     }
   }
   
   return false;
 }

test/Sema/builtins.c

         strlcpy(buf, b, sizeof(b)); // expected-warning {{size argument in 'strlcpy' call appears to be size of the source; expected the size of the destination}} \\
                                     // expected-note {{change size argument to be the size of the destination}}
         __builtin___strlcpy_chk(buf, b, sizeof(b), __builtin_object_size(buf, 0)); // expected-warning {{size argument in '__builtin___strlcpy_chk' call appears to be size of the source; expected the size of the destination}} \
                                     // expected-note {{change size argument to be the size of the destination}} \
             // expected-warning {{'__builtin___strlcpy_chk' will always overflow destination buffer}}
 
         strlcat(buf, b, sizeof(b)); // expected-warning {{size argument in 'strlcat' call appears to be size of the source; expected the size of the destination}} \
                                     // expected-note {{change size argument to be the size of the destination}}
             
         __builtin___strlcat_chk(buf, b, sizeof(b), __builtin_object_size(buf, 0)); // expected-warning {{size argument in '__builtin___strlcat_chk' call appears to be size of the source; expected the size of the destination}} \
                                                                                    // expected-note {{change size argument to be the size of the destination}} \
                                                            // expected-warning {{'__builtin___strlcat_chk' will always overflow destination buffer}}
 }
 
 // rdar://11076881
 char * Test20(char *p, const char *in, unsigned n)
 {

 
     return buf;
 }
 
 void test21(const int *ptr) {
   __sync_fetch_and_add(ptr, 1); // expected-error{{address argument to atomic builtin cannot be const-qualified ('const int *' invalid)}}
   __atomic_fetch_add(ptr, 1, 0);  // expected-error {{address argument to atomic operation must be a pointer to non-const type ('const int *' invalid)}}
 }
+
+void test22(void) {
+  (void)__builtin_signbit(); // expected-error{{too few arguments to function call, expected 1, have 0}}
+  (void)__builtin_signbit(1.0, 2.0, 3.0); // expected-error{{too many arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbit(1); // expected-error {{floating point classification requires argument of floating point type (passed in 'int')}}
+  (void)__builtin_signbit(1.0);
+  (void)__builtin_signbit(1.0f);
+  (void)__builtin_signbit(1.0L);
+
+  (void)__builtin_signbitf(); // expected-error{{too few arguments to function call, expected 1, have 0}}
+  (void)__builtin_signbitf(1.0, 2.0, 3.0); // expected-error{{too many arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbitf(1);
+  (void)__builtin_signbitf(1.0);
+  (void)__builtin_signbitf(1.0f);
+  (void)__builtin_signbitf(1.0L);
+
+  (void)__builtin_signbitl(); // expected-error{{too few arguments to function call, expected 1, have 0}}
+  (void)__builtin_signbitl(1.0, 2.0, 3.0); // expected-error{{too many arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbitl(1);
+  (void)__builtin_signbitl(1.0);
+  (void)__builtin_signbitl(1.0f);
+  (void)__builtin_signbitl(1.0L);
+}