Fix for Bug 28172 : clang crashes on invalid code (with too few arguments to __builtin_signbit) without any proper diagnostics.

[mayurpandey]

Summary

clang was crashing when no argument was provided to __builtin_signbit function. Added a check to handle the error. No regressions on testing.

[george.burgess.iv]

LGTM; thanks for the patch!

[majnemer]

I feel like this needs further validation. For example, we crash on:

int x = __builtin_signbit("1");

[mayurpandey]

The crash on :
int x = __builtin_signbit("1");

is not a side effect of this patch. It was failing prior to the patch as well. I can work on a new patch for this crash.

[mayurpandey]

Hi,

Updated the patch to handle the second crash. __builtin_signbit("1") was crashing. I am not fully sure whether the check : if (!Ty->isRealFloatingType()) is correct. Please review and let me know whether it is fine and if not what all changes are needed.

Thanks,
Mayur

[majnemer]

This looks much better. Please add a test that uses a template:

template <typename T>
int f(T t) { return __builtin_signbit(t); }

include/clang/Basic/DiagnosticSemaKinds.td

7596–7598

Hmm, I think it would be better if you added this near err_typecheck_cond_expect_int_float, named it err_typecheck_cond_expect_float, and gave it a similar error message.

[mayurpandey]

Hi,

Updated the patch to add a template based testcase. As suggested by you, I tried updating the error diagnostic to err_typecheck_cond_expect_float and to issue diagnostic in similar fashion to err_typecheck_cond_expect_int_float, something like this :
def err_typecheck_cond_expect_float : Error<

"used type %0 where floating point type is required">;

But on updating this clang was crashing with the following message:
clang-3.9: llvm/tools/clang/include/clang/Basic/Diagnostic.h:1167: clang::DiagnosticsEngine::ArgumentKind clang::Diagnostic::getArgKind(unsigned int) const: Assertion `Idx < getNumArgs() && "Argument index out of range!"' failed.

So reverted it to as it was earlier and similar to other builtin error err_builtin_annotation_first_arg.

Please review the changes and let me know if any other change is needed.

Thanks,
Mayur

[mayurpandey]

Hi David,

The crash that we see can be seen with other diagnostics for builtin if we use the diagnostic format you suggested. I tried the same thing with err_builtin_annotation_first_arg. The same crash can be seen. So shall I fix this crash with this bug or file a new bug for this crash and work on it?

Thanks,
Mayur

[mayurpandey]

Hi,

Updated the patch to incorporate the review comments. I had missed adding ValArg->getType() when emitting the diagnostic which was cauing the crash.

Testing done, no regressions.

Thanks,
Mayur

[majnemer]

LGTM with nits

lib/Sema/SemaChecking.cpp

110

Can you change this ValArg->getType() to Ty?

[mayurpandey]

Hi,

Updated the patch. Can you please commit it on my behalf as I don't have commit access.

Thanks,
Mayur

[shafik]

It looks like the examples no longer crash: https://godbolt.org/z/Yn8qK3fnT

So likely we should just close this.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_typecheck_call_invalid_ordered_compare : Error<
   "ordered compare requires two args of floating point type"
   "%diff{ ($ and $)|}0,1">;
 def err_typecheck_call_invalid_unary_fp : Error<
   "floating point classification requires argument of floating point type "
   "(passed in %0)">;
 def err_typecheck_cond_expect_int_float : Error<
   "used type %0 where integer or floating point type is required">;
+def err_typecheck_cond_expect_float : Error<
+  "used type %0 where floating point type is required">;
 def err_typecheck_cond_expect_scalar : Error<
   "used type %0 where arithmetic or pointer type is required">;
 def err_typecheck_cond_expect_nonfloat : Error<
   "used type %0 where floating point type is not allowed">;
 def ext_typecheck_cond_one_void : Extension<
   "C99 forbids conditional expressions with only one void side">;
 def err_typecheck_cast_to_incomplete : Error<
   "cast to incomplete type %0">;
def err_block_returning_array_function : Error<
   "block cannot return %select{array|function}0 type %1">;
 
 // Builtin annotation
 def err_builtin_annotation_first_arg : Error<
   "first argument to __builtin_annotation must be an integer">;
 def err_builtin_annotation_second_arg : Error<
   "second argument to __builtin_annotation must be a non-wide string constant">;
 
 // CFString checking
 def err_cfstring_literal_not_string_constant : Error<
   "CFString literal is not a string constant">;
 def warn_cfstring_truncated : Warning<
   "input conversion stopped due to an input byte that does not "
   "belong to the input codeset UTF-8">,
   InGroup<DiagGroup<"CFString-literal">>;
 
 // os_log checking
 // TODO: separate diagnostic for os_trace()
 def err_os_log_format_not_string_constant : Error<

lib/Sema/SemaChecking.cpp

S.Diag(StrArg->getLocStart(), diag::err_builtin_annotation_second_arg)
       << StrArg->getSourceRange();
     return true;
   }
 
   TheCall->setType(Ty);
   return false;
 }
 
+static bool SemaBuiltinSignbit(Sema &S, CallExpr *TheCall) {
+  if (checkArgCount(S, TheCall, 1))
+    return true;
+
+  // Argument should be an float, double or long double.
+  Expr *ValArg = TheCall->getArg(0);
+  QualType Ty = ValArg->getType();
+  if (!Ty->isRealFloatingType()) {
+    S.Diag(ValArg->getLocStart(), diag::err_typecheck_cond_expect_float)
+      << Ty << ValArg->getSourceRange();
+    return true;
+  }
+
+  return false;
+}
+
 /// Check that the argument to __builtin_addressof is a glvalue, and set the
 /// result type to the corresponding pointer type.
 static bool SemaBuiltinAddressof(Sema &S, CallExpr *TheCall) {
   if (checkArgCount(S, TheCall, 1))
     return true;
 
   ExprResult Arg(TheCall->getArg(0));
   QualType ResultType = S.CheckAddressOfOperand(Arg, TheCall->getLocStart());
case llvm::Triple::thumb:
       break;
     default:
       if (SemaBuiltinVAStart(TheCall))
         return ExprError();
       break;
     }
     break;
   }
+  case Builtin::BI__builtin_signbit:
+    if (SemaBuiltinSignbit(*this, TheCall))
+      return ExprError();
+    break;
   case Builtin::BI__builtin_isgreater:
   case Builtin::BI__builtin_isgreaterequal:
   case Builtin::BI__builtin_isless:
   case Builtin::BI__builtin_islessequal:
   case Builtin::BI__builtin_islessgreater:
   case Builtin::BI__builtin_isunordered:
     if (SemaBuiltinUnorderedCompare(TheCall))
       return ExprError();

test/Sema/builtins.c

char * Test20(char *p, const char *in, unsigned n)
     __builtin___memcpy_chk (&buf[5], "abcde", 5, __builtin_object_size (&buf[5], 0));
 
     __builtin___memcpy_chk (&buf[5], "abcde", n, __builtin_object_size (&buf[5], 0));
 
     __builtin___memcpy_chk (&buf[6], "abcde", 5, __builtin_object_size (&buf[6], 0)); // expected-warning {{'__builtin___memcpy_chk' will always overflow destination buffer}}
 
     return buf;
 }
+
+int test21(double a) {
+  return __builtin_signbit();  // expected-error {{too few arguments}}
+}
+
+int test22(void) {
+  return __builtin_signbit("1");  // expected-error {{floating point type is required}}
+}

test/SemaCXX/builtins.cpp

namespace addressof {
   S *ptmp = __builtin_addressof(S{}); // expected-error {{taking the address of a temporary}}
 }
 
 void no_ms_builtins() {
   __assume(1); // expected-error {{use of undeclared}}
   __noop(1); // expected-error {{use of undeclared}}
   __debugbreak(); // expected-error {{use of undeclared}}
 }
+
+template <typename T>
+int test_signbit(T t) { return __builtin_signbit(t); } // expected-error {{floating point type is required}}
+
+int test_signbit_call () {
+return test_signbit("1"); // expected-note {{instantiation of function template specialization}} 
+}