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

7401–7403 ↗

(On Diff #69354)

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

111

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.

lib/Sema/SemaChecking.cpp

 /// 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());
   if (ResultType.isNull())
     return true;
 
   TheCall->setArg(0, Arg.get());
   TheCall->setType(ResultType);
   return false;
 }
 
 static bool SemaBuiltinOverflow(Sema &S, CallExpr *TheCall) {
   if (checkArgCount(S, TheCall, 3))
case llvm::Triple::thumb:
       break;
     default:
       if (SemaBuiltinVAStart(TheCall))
         return ExprError();
       break;
     }
     break;
   }
+  case Builtin::BI__builtin_signbit:
+    if (checkArgCount(*this, TheCall, 1))
+      return true;
+    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}}
+}