This is an archive of the discontinued LLVM Phabricator instance.

Differential D9235

[Sema] Prevent crash while trying to merge types
ClosedPublic

Authored by davide on Apr 23 2015, 3:46 PM.

Details

Reviewers
aaron.ballman
rsmith
Commits
rG3c613d608da6: [Sema] Check if a builtin is FunctionPrototype().
rC235806: [Sema] Check if a builtin is FunctionPrototype().
rL235806: [Sema] Check if a builtin is FunctionPrototype().
Summary

This fixes bug 23086.
__builtin_isnan(...) causes clang to crash in sema. The proposed patch is an attempt to fix.

Diff Detail

Event Timeline

davide updated this revision to Diff 24336.Apr 23 2015, 3:46 PM
davide retitled this revision from to [Sema] Prevent crash while trying to merge types.
davide updated this object.
davide edited the test plan for this revision. (Show Details)
davide added reviewers: rsmith, aaron.ballman.
davide set the repository for this revision to rL LLVM.
davide added a subscriber: Unknown Object (MLST).
rsmith added inline comments.Apr 23 2015, 4:07 PM
lib/Sema/SemaDecl.cpp
2988–2991
  • This function should not return true without a diagnostic having been emitted.
  • NewProto cannot be null here due to the check on line 2979.
  • OldProto should not be null here due to the check on line 2978.

Perhaps the root cause of the problem is that we set the wrong value for hasPrototype up on line 1752? We should be checking whether the builtin type is in fact a FunctionProtoType rather than just assuming it is.

davide updated this revision to Diff 24347.Apr 23 2015, 6:41 PM
davide removed rL LLVM as the repository for this revision.

Yes, thanks. That actually worked and passes the test suite.
Does the check look fine to you?

rsmith accepted this revision.Apr 24 2015, 8:30 AM
rsmith edited edge metadata.

Yes, this seems like a reasonable approach (please also include the testcase from your previous patch).

This revision is now accepted and ready to land.Apr 24 2015, 8:30 AM
Closed by commit rL235806: [Sema] Check if a builtin is FunctionPrototype(). (authored by davide). · Explain WhyApr 25 2015, 1:23 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Sema/
5 lines
test/
Sema/
4 lines

Diff 24336

lib/Sema/SemaDecl.cpp

Show First 20 Lines • Show All 992 Lines • ▼ Show 20 Lines
Old->getNumParams() == New->getNumParams()) { Old->getNumParams() == New->getNumParams()) {
SmallVector<QualType, 16> ArgTypes; SmallVector<QualType, 16> ArgTypes;
SmallVector<GNUCompatibleParamWarning, 16> Warnings; SmallVector<GNUCompatibleParamWarning, 16> Warnings;
const FunctionProtoType *OldProto const FunctionProtoType *OldProto
= Old->getType()->getAs<FunctionProtoType>(); = Old->getType()->getAs<FunctionProtoType>();
const FunctionProtoType *NewProto const FunctionProtoType *NewProto
= New->getType()->getAs<FunctionProtoType>(); = New->getType()->getAs<FunctionProtoType>();
// The function prototype might be null, in which case
// there's not point to keep going and trying to merge types.
if (!OldProto || !NewProto)
return true;
rsmithUnsubmitted
Not Done
ReplyInline Actions
  • This function should not return true without a diagnostic having been emitted.
  • NewProto cannot be null here due to the check on line 2979.
  • OldProto should not be null here due to the check on line 2978.

Perhaps the root cause of the problem is that we set the wrong value for hasPrototype up on line 1752? We should be checking whether the builtin type is in fact a FunctionProtoType rather than just assuming it is.

rsmith: * This function should not return `true` without a diagnostic having been emitted. * `NewProto`…
// Determine whether this is the GNU C extension. // Determine whether this is the GNU C extension.
QualType MergedReturn = Context.mergeTypes(OldProto->getReturnType(), QualType MergedReturn = Context.mergeTypes(OldProto->getReturnType(),
NewProto->getReturnType()); NewProto->getReturnType());
bool LooseCompatible = !MergedReturn.isNull(); bool LooseCompatible = !MergedReturn.isNull();
for (unsigned Idx = 0, End = Old->getNumParams(); for (unsigned Idx = 0, End = Old->getNumParams();
LooseCompatible && Idx != End; ++Idx) { LooseCompatible && Idx != End; ++Idx) {
ParmVarDecl *OldParm = Old->getParamDecl(Idx); ParmVarDecl *OldParm = Old->getParamDecl(Idx);
ParmVarDecl *NewParm = New->getParamDecl(Idx); ParmVarDecl *NewParm = New->getParamDecl(Idx);
▲ Show 20 Lines • Show All 992 Lines • Show Last 20 Lines

test/Sema/crash-invalid-builtin.c

// RUN: %clang_cc1 -triple=x86_64-apple-darwin -fsyntax-only -verify %s
// PR23086
__builtin_isinf(...); // expected-warning {{type specifier missing, defaults to 'int'}} expected-error {{ISO C requires a named parameter before '...'}}