From: Aaron Ballman Date: Wed, 8 Apr 2015 00:05:29 +0000 (+0000) Subject: Generate a diagnostic when a catch handler cannot execute due to class hierarchy... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=c3f7ae01e2cd11964e4aa3b49622ae9419023fa7;p=clang Generate a diagnostic when a catch handler cannot execute due to class hierarchy inversion with regards to other catch handlers for the same block. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@234375 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 6f4cce79f2..bb528db21d 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -107,6 +107,8 @@ def Documentation : DiagGroup<"documentation", DocumentationDeprecatedSync]>; def EmptyBody : DiagGroup<"empty-body">; +def Exceptions : DiagGroup<"exceptions">; + def GNUEmptyInitializer : DiagGroup<"gnu-empty-initializer">; def GNUEmptyStruct : DiagGroup<"gnu-empty-struct">; def ExtraTokens : DiagGroup<"extra-tokens">; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 03635ab3e2..901d50028d 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -5475,7 +5475,8 @@ def err_bad_memptr_lhs : Error< "left hand operand to %0 must be a %select{|pointer to }1class " "compatible with the right hand operand, but is %2">; def warn_exception_caught_by_earlier_handler : Warning< - "exception of type %0 will be caught by earlier handler">; + "exception of type %0 will be caught by earlier handler">, + InGroup; def note_previous_exception_handler : Note<"for type %0">; def err_exceptions_disabled : Error< "cannot use '%0' with exceptions disabled">; diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index 0ac9c893b3..8bf1403f38 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -15,6 +15,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/ASTDiagnostic.h" #include "clang/AST/CharUnits.h" +#include "clang/AST/CXXInheritance.h" #include "clang/AST/DeclObjC.h" #include "clang/AST/EvaluatedExprVisitor.h" #include "clang/AST/ExprCXX.h" @@ -23,12 +24,14 @@ #include "clang/AST/StmtCXX.h" #include "clang/AST/StmtObjC.h" #include "clang/AST/TypeLoc.h" +#include "clang/AST/TypeOrdering.h" #include "clang/Lex/Preprocessor.h" #include "clang/Sema/Initialization.h" #include "clang/Sema/Lookup.h" #include "clang/Sema/Scope.h" #include "clang/Sema/ScopeInfo.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallString.h" @@ -3253,36 +3256,111 @@ Sema::ActOnObjCAutoreleasePoolStmt(SourceLocation AtLoc, Stmt *Body) { return new (Context) ObjCAutoreleasePoolStmt(AtLoc, Body); } -namespace { +class CatchHandlerType { + QualType QT; + unsigned IsPointer : 1; -class TypeWithHandler { - QualType t; - CXXCatchStmt *stmt; -public: - TypeWithHandler(const QualType &type, CXXCatchStmt *statement) - : t(type), stmt(statement) {} + // This is a special constructor to be used only with DenseMapInfo's + // getEmptyKey() and getTombstoneKey() functions. + friend struct llvm::DenseMapInfo; + enum Unique { ForDenseMap }; + CatchHandlerType(QualType QT, Unique) : QT(QT), IsPointer(false) {} - // An arbitrary order is fine as long as it places identical - // types next to each other. - bool operator<(const TypeWithHandler &y) const { - if (t.getAsOpaquePtr() < y.t.getAsOpaquePtr()) - return true; - if (t.getAsOpaquePtr() > y.t.getAsOpaquePtr()) +public: + /// Used when creating a CatchHandlerType from a handler type; will determine + /// whether the type is a pointer or reference and will strip off the the top + /// level pointer and cv-qualifiers. + CatchHandlerType(QualType Q) : QT(Q), IsPointer(false) { + if (QT->isPointerType()) + IsPointer = true; + + if (IsPointer || QT->isReferenceType()) + QT = QT->getPointeeType(); + QT = QT.getUnqualifiedType(); + } + + /// Used when creating a CatchHandlerType from a base class type; pretends the + /// type passed in had the pointer qualifier, does not need to get an + /// unqualified type. + CatchHandlerType(QualType QT, bool IsPointer) + : QT(QT), IsPointer(IsPointer) {} + + QualType underlying() const { return QT; } + bool isPointer() const { return IsPointer; } + + friend bool operator==(const CatchHandlerType &LHS, + const CatchHandlerType &RHS) { + // If the pointer qualification does not match, we can return early. + if (LHS.IsPointer != RHS.IsPointer) return false; - else - return getTypeSpecStartLoc() < y.getTypeSpecStartLoc(); + // Otherwise, check the underlying type without cv-qualifiers. + return LHS.QT == RHS.QT; + } +}; + +namespace llvm { +template <> struct DenseMapInfo { + static CatchHandlerType getEmptyKey() { + return CatchHandlerType(DenseMapInfo::getEmptyKey(), + CatchHandlerType::ForDenseMap); } - bool operator==(const TypeWithHandler& other) const { - return t == other.t; + static CatchHandlerType getTombstoneKey() { + return CatchHandlerType(DenseMapInfo::getTombstoneKey(), + CatchHandlerType::ForDenseMap); } - CXXCatchStmt *getCatchStmt() const { return stmt; } - SourceLocation getTypeSpecStartLoc() const { - return stmt->getExceptionDecl()->getTypeSpecStartLoc(); + static unsigned getHashValue(const CatchHandlerType &Base) { + return DenseMapInfo::getHashValue(Base.underlying()); } + + static bool isEqual(const CatchHandlerType &LHS, + const CatchHandlerType &RHS) { + return LHS == RHS; + } +}; + +// It's OK to treat CatchHandlerType as a POD type. +template <> struct isPodLike { + static const bool value = true; }; +} +namespace { +class CatchTypePublicBases { + ASTContext &Ctx; + const llvm::DenseMap &TypesToCheck; + const bool CheckAgainstPointer; + + CXXCatchStmt *FoundHandler; + CanQualType FoundHandlerType; + +public: + CatchTypePublicBases( + ASTContext &Ctx, + const llvm::DenseMap &T, bool C) + : Ctx(Ctx), TypesToCheck(T), CheckAgainstPointer(C), + FoundHandler(nullptr) {} + + CXXCatchStmt *getFoundHandler() const { return FoundHandler; } + CanQualType getFoundHandlerType() const { return FoundHandlerType; } + + static bool FindPublicBasesOfType(const CXXBaseSpecifier *S, CXXBasePath &, + void *User) { + auto &PBOT = *reinterpret_cast(User); + if (S->getAccessSpecifier() == AccessSpecifier::AS_public) { + CatchHandlerType Check(S->getType(), PBOT.CheckAgainstPointer); + auto M = PBOT.TypesToCheck; + auto I = M.find(Check); + if (I != M.end()) { + PBOT.FoundHandler = I->second; + PBOT.FoundHandlerType = PBOT.Ctx.getCanonicalType(S->getType()); + return true; + } + } + return false; + } +}; } /// ActOnCXXTryBlock - Takes a try compound-statement and a number of @@ -3292,7 +3370,7 @@ StmtResult Sema::ActOnCXXTryBlock(SourceLocation TryLoc, Stmt *TryBlock, // Don't report an error if 'try' is used in system headers. if (!getLangOpts().CXXExceptions && !getSourceManager().isInSystemHeader(TryLoc)) - Diag(TryLoc, diag::err_exceptions_disabled) << "try"; + Diag(TryLoc, diag::err_exceptions_disabled) << "try"; if (getCurScope() && getCurScope()->isOpenMPSimdDirectiveScope()) Diag(TryLoc, diag::err_omp_simd_region_cannot_use_stmt) << "try"; @@ -3306,55 +3384,73 @@ StmtResult Sema::ActOnCXXTryBlock(SourceLocation TryLoc, Stmt *TryBlock, } const unsigned NumHandlers = Handlers.size(); - assert(NumHandlers > 0 && + assert(!Handlers.empty() && "The parser shouldn't call this if there are no handlers."); - SmallVector TypesWithHandlers; - + llvm::DenseMap HandledTypes; for (unsigned i = 0; i < NumHandlers; ++i) { - CXXCatchStmt *Handler = cast(Handlers[i]); - if (!Handler->getExceptionDecl()) { - if (i < NumHandlers - 1) - return StmtError(Diag(Handler->getLocStart(), - diag::err_early_catch_all)); + CXXCatchStmt *H = cast(Handlers[i]); + // Diagnose when the handler is a catch-all handler, but it isn't the last + // handler for the try block. [except.handle]p5. Also, skip exception + // declarations that are invalid, since we can't usefully report on them. + if (!H->getExceptionDecl()) { + if (i < NumHandlers - 1) + return StmtError(Diag(H->getLocStart(), diag::err_early_catch_all)); + continue; + } else if (H->getExceptionDecl()->isInvalidDecl()) continue; - } - - const QualType CaughtType = Handler->getCaughtType(); - const QualType CanonicalCaughtType = Context.getCanonicalType(CaughtType); - TypesWithHandlers.push_back(TypeWithHandler(CanonicalCaughtType, Handler)); - } - - // Detect handlers for the same type as an earlier one. - if (NumHandlers > 1) { - llvm::array_pod_sort(TypesWithHandlers.begin(), TypesWithHandlers.end()); - - TypeWithHandler prev = TypesWithHandlers[0]; - for (unsigned i = 1; i < TypesWithHandlers.size(); ++i) { - TypeWithHandler curr = TypesWithHandlers[i]; - if (curr == prev) { - Diag(curr.getTypeSpecStartLoc(), - diag::warn_exception_caught_by_earlier_handler) - << curr.getCatchStmt()->getCaughtType().getAsString(); - Diag(prev.getTypeSpecStartLoc(), - diag::note_previous_exception_handler) - << prev.getCatchStmt()->getCaughtType().getAsString(); + // Walk the type hierarchy to diagnose when this type has already been + // handled (duplication), or cannot be handled (derivation inversion). We + // ignore top-level cv-qualifiers, per [except.handle]p3 + CatchHandlerType HandlerCHT = Context.getCanonicalType(H->getCaughtType()); + + // We can ignore whether the type is a reference or a pointer; we need the + // underlying declaration type in order to get at the underlying record + // decl, if there is one. + QualType Underlying = HandlerCHT.underlying(); + if (auto *RD = Underlying->getAsCXXRecordDecl()) { + if (!RD->hasDefinition()) + continue; + // Check that none of the public, unambiguous base classes are in the + // map ([except.handle]p1). Give the base classes the same pointer + // qualification as the original type we are basing off of. This allows + // comparison against the handler type using the same top-level pointer + // as the original type. + CXXBasePaths Paths; + Paths.setOrigin(RD); + CatchTypePublicBases CTPB(Context, HandledTypes, HandlerCHT.isPointer()); + if (RD->lookupInBases(CatchTypePublicBases::FindPublicBasesOfType, &CTPB, + Paths)) { + const CXXCatchStmt *Problem = CTPB.getFoundHandler(); + if (!Paths.isAmbiguous(CTPB.getFoundHandlerType())) { + Diag(H->getExceptionDecl()->getTypeSpecStartLoc(), + diag::warn_exception_caught_by_earlier_handler) + << H->getCaughtType(); + Diag(Problem->getExceptionDecl()->getTypeSpecStartLoc(), + diag::note_previous_exception_handler) + << Problem->getCaughtType(); + } } + } - prev = curr; + // Add the type the list of ones we have handled; diagnose if we've already + // handled it. + auto R = HandledTypes.insert(std::make_pair(H->getCaughtType(), H)); + if (!R.second) { + const CXXCatchStmt *Problem = R.first->second; + Diag(H->getExceptionDecl()->getTypeSpecStartLoc(), + diag::warn_exception_caught_by_earlier_handler) + << H->getCaughtType(); + Diag(Problem->getExceptionDecl()->getTypeSpecStartLoc(), + diag::note_previous_exception_handler) + << Problem->getCaughtType(); } } FSI->setHasCXXTry(TryLoc); - // FIXME: We should detect handlers that cannot catch anything because an - // earlier handler catches a superclass. Need to find a method that is not - // quadratic for this. - // Neither of these are explicitly forbidden, but every compiler detects them - // and warns. - return CXXTryStmt::Create(Context, TryLoc, TryBlock, Handlers); } diff --git a/test/CXX/drs/dr3xx.cpp b/test/CXX/drs/dr3xx.cpp index cea4d64e7e..5ac4f013b7 100644 --- a/test/CXX/drs/dr3xx.cpp +++ b/test/CXX/drs/dr3xx.cpp @@ -170,9 +170,9 @@ namespace dr308 { // dr308: yes void f() { try { throw D(); - } catch (const A&) { + } catch (const A&) { // expected-note {{for type 'const dr308::A &'}} // unreachable - } catch (const B&) { + } catch (const B&) { // expected-warning {{exception of type 'const dr308::B &' will be caught by earlier handler}} // get here instead } } diff --git a/test/Misc/warning-flags.c b/test/Misc/warning-flags.c index 657b44dc18..86274ad0bd 100644 --- a/test/Misc/warning-flags.c +++ b/test/Misc/warning-flags.c @@ -18,7 +18,7 @@ This test serves two purposes: The list of warnings below should NEVER grow. It should gradually shrink to 0. -CHECK: Warnings without flags (94): +CHECK: Warnings without flags (93): CHECK-NEXT: ext_excess_initializers CHECK-NEXT: ext_excess_initializers_in_char_array_initializer CHECK-NEXT: ext_expected_semi_decl_list @@ -68,7 +68,6 @@ CHECK-NEXT: warn_drv_pch_not_first_include CHECK-NEXT: warn_dup_category_def CHECK-NEXT: warn_duplicate_protocol_def CHECK-NEXT: warn_enum_value_overflow -CHECK-NEXT: warn_exception_caught_by_earlier_handler CHECK-NEXT: warn_expected_qualified_after_typename CHECK-NEXT: warn_extraneous_char_constant CHECK-NEXT: warn_fe_cc_log_diagnostics_failure diff --git a/test/SemaCXX/exceptions.cpp b/test/SemaCXX/exceptions.cpp index 6ac51b32e4..9802a1a1d6 100644 --- a/test/SemaCXX/exceptions.cpp +++ b/test/SemaCXX/exceptions.cpp @@ -146,6 +146,82 @@ namespace Decay { void rval_ref() throw (int &&); // expected-error {{rvalue reference type 'int &&' is not allowed in exception specification}} expected-warning {{C++11}} +namespace HandlerInversion { +struct B {}; +struct D : B {}; +struct D2 : D {}; + +void f1() { + try { + } catch (B &b) { // expected-note {{for type 'HandlerInversion::B &'}} + } catch (D &d) { // expected-warning {{exception of type 'HandlerInversion::D &' will be caught by earlier handler}} + } +} + +void f2() { + try { + } catch (B *b) { // expected-note {{for type 'HandlerInversion::B *'}} + } catch (D *d) { // expected-warning {{exception of type 'HandlerInversion::D *' will be caught by earlier handler}} + } +} + +void f3() { + try { + } catch (D &d) { // Ok + } catch (B &b) { + } +} + +void f4() { + try { + } catch (B &b) { // Ok + } +} + +void f5() { + try { + } catch (int) { + } catch (float) { + } +} + +void f6() { + try { + } catch (B &b) { // expected-note {{for type 'HandlerInversion::B &'}} + } catch (D2 &d) { // expected-warning {{exception of type 'HandlerInversion::D2 &' will be caught by earlier handler}} + } +} + +void f7() { + try { + } catch (B *b) { // Ok + } catch (D &d) { // Ok + } + + try { + } catch (B b) { // Ok + } catch (D *d) { // Ok + } +} + +void f8() { + try { + } catch (const B &b) { // expected-note {{for type 'const HandlerInversion::B &'}} + } catch (D2 &d) { // expected-warning {{exception of type 'HandlerInversion::D2 &' will be caught by earlier handler}} + } + + try { + } catch (B &b) { // expected-note {{for type 'HandlerInversion::B &'}} + } catch (const D2 &d) { // expected-warning {{exception of type 'const HandlerInversion::D2 &' will be caught by earlier handler}} + } + + try { + } catch (B b) { // expected-note {{for type 'HandlerInversion::B'}} + } catch (D &d) { // expected-warning {{exception of type 'HandlerInversion::D &' will be caught by earlier handler}} + } +} +} + namespace ConstVolatileThrow { struct S { S() {} // expected-note{{candidate constructor not viable}} diff --git a/test/SemaCXX/unreachable-catch-clauses.cpp b/test/SemaCXX/unreachable-catch-clauses.cpp index c75067f393..8dcb47b13c 100644 --- a/test/SemaCXX/unreachable-catch-clauses.cpp +++ b/test/SemaCXX/unreachable-catch-clauses.cpp @@ -8,7 +8,6 @@ void f(); void test() try {} -catch (BaseEx &e) { f(); } -catch (Ex1 &e) { f(); } // expected-note {{for type class Ex1 &}} -catch (Ex2 &e) { f(); } // expected-warning {{exception of type Ex2 & will be caught by earlier handler}} - +catch (BaseEx &e) { f(); } // expected-note 2{{for type 'BaseEx &'}} +catch (Ex1 &e) { f(); } // expected-warning {{exception of type 'Ex1 &' will be caught by earlier handler}} +catch (Ex2 &e) { f(); } // expected-warning {{exception of type 'Ex2 &' (aka 'Ex1 &') will be caught by earlier handler}}