This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mayurpandey on Jul 13 2016, 9:26 PM.

Details

Summary

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

Diff Detail

Event Timeline

mayurpandey retitled this revision from to Fix for Bug 28172 : clang crashes on invalid code (with too few arguments to __builtin_signbit) without any proper diagnostics..
mayurpandey updated this object.
mayurpandey added a subscriber: cfe-commits.
george.burgess.iv edited edge metadata.

LGTM; thanks for the patch!

This revision is now accepted and ready to land.Jul 15 2016, 11:52 AM
majnemer requested changes to this revision.Jul 15 2016, 11:59 AM
majnemer added a reviewer: majnemer.
majnemer added a subscriber: majnemer.

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

int x = __builtin_signbit("1");
This revision now requires changes to proceed.Jul 15 2016, 11:59 AM

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 edited edge metadata.

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

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 edited edge metadata.

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

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

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 accepted this revision.Nov 17 2016, 12:19 PM
majnemer edited edge metadata.

LGTM with nits

lib/Sema/SemaChecking.cpp
111

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

This revision is now accepted and ready to land.Nov 17 2016, 12:19 PM
mayurpandey edited edge metadata.

Hi,

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

Thanks,
Mayur

shafik added a subscriber: shafik.Sep 21 2023, 8:51 AM

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

So likely we should just close this.

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2023, 8:51 AM