diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -193,6 +193,10 @@ (`#60405 `_) - Fix aggregate initialization inside lambda constexpr. (`#60936 `_) +- No longer issue a false positive diagnostic about a catch handler that cannot + be reached despite being reachable. This fixes + `#61177 `_ in anticipation + of `CWG2699 _` being accepted by WG21. Bug Fixes to Compiler Builtins ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp --- a/clang/lib/Sema/SemaStmt.cpp +++ b/clang/lib/Sema/SemaStmt.cpp @@ -4351,9 +4351,9 @@ if (QT->isPointerType()) IsPointer = true; + QT = QT.getUnqualifiedType(); if (IsPointer || QT->isReferenceType()) QT = QT->getPointeeType(); - QT = QT.getUnqualifiedType(); } /// Used when creating a CatchHandlerType from a base class type; pretends the @@ -4402,31 +4402,43 @@ namespace { class CatchTypePublicBases { ASTContext &Ctx; - const llvm::DenseMap &TypesToCheck; - const bool CheckAgainstPointer; + const llvm::DenseMap &TypesToCheck; CXXCatchStmt *FoundHandler; - CanQualType FoundHandlerType; + QualType FoundHandlerType; + QualType TestAgainstType; public: - CatchTypePublicBases( - ASTContext &Ctx, - const llvm::DenseMap &T, bool C) - : Ctx(Ctx), TypesToCheck(T), CheckAgainstPointer(C), - FoundHandler(nullptr) {} + CatchTypePublicBases(ASTContext &Ctx, + const llvm::DenseMap &T, + QualType QT) + : Ctx(Ctx), TypesToCheck(T), FoundHandler(nullptr), TestAgainstType(QT) {} CXXCatchStmt *getFoundHandler() const { return FoundHandler; } - CanQualType getFoundHandlerType() const { return FoundHandlerType; } + QualType getFoundHandlerType() const { return FoundHandlerType; } bool operator()(const CXXBaseSpecifier *S, CXXBasePath &) { if (S->getAccessSpecifier() == AccessSpecifier::AS_public) { - CatchHandlerType Check(S->getType(), CheckAgainstPointer); + QualType Check = S->getType().getCanonicalType(); const auto &M = TypesToCheck; auto I = M.find(Check); if (I != M.end()) { - FoundHandler = I->second; - FoundHandlerType = Ctx.getCanonicalType(S->getType()); - return true; + // We're pretty sure we found what we need to find. However, we still + // need to make sure that we properly compare for pointers and + // references, to handle cases like: + // + // } catch (Base *b) { + // } catch (Derived &d) { + // } + // + // where there is a qualification mismatch that disqualifies this + // handler as a potential problem. + if (I->second->getCaughtType()->isPointerType() == + TestAgainstType->isPointerType()) { + FoundHandler = I->second; + FoundHandlerType = Check; + return true; + } } } return false; @@ -4465,6 +4477,7 @@ assert(!Handlers.empty() && "The parser shouldn't call this if there are no handlers."); + llvm::DenseMap HandledBaseTypes; llvm::DenseMap HandledTypes; for (unsigned i = 0; i < NumHandlers; ++i) { CXXCatchStmt *H = cast(Handlers[i]); @@ -4482,8 +4495,7 @@ // 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 = - (QualType)Context.getCanonicalType(H->getCaughtType()); + CatchHandlerType HandlerCHT = H->getCaughtType().getCanonicalType(); // 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 @@ -4499,10 +4511,12 @@ // as the original type. CXXBasePaths Paths; Paths.setOrigin(RD); - CatchTypePublicBases CTPB(Context, HandledTypes, HandlerCHT.isPointer()); + CatchTypePublicBases CTPB(Context, HandledBaseTypes, + H->getCaughtType().getCanonicalType()); if (RD->lookupInBases(CTPB, Paths)) { const CXXCatchStmt *Problem = CTPB.getFoundHandler(); - if (!Paths.isAmbiguous(CTPB.getFoundHandlerType())) { + if (!Paths.isAmbiguous( + CanQualType::CreateUnsafe(CTPB.getFoundHandlerType()))) { Diag(H->getExceptionDecl()->getTypeSpecStartLoc(), diag::warn_exception_caught_by_earlier_handler) << H->getCaughtType(); @@ -4511,11 +4525,16 @@ << Problem->getCaughtType(); } } + // Strip the qualifiers here because we're going to be comparing this + // type to the base type specifiers of a class, which are ignored in a + // base specifier per [class.derived.general]p2. + HandledBaseTypes[Underlying.getUnqualifiedType()] = H; } // 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)); + auto R = HandledTypes.insert( + std::make_pair(H->getCaughtType().getCanonicalType(), H)); if (!R.second) { const CXXCatchStmt *Problem = R.first->second; Diag(H->getExceptionDecl()->getTypeSpecStartLoc(), diff --git a/clang/test/CXX/drs/dr3xx.cpp b/clang/test/CXX/drs/dr3xx.cpp --- a/clang/test/CXX/drs/dr3xx.cpp +++ b/clang/test/CXX/drs/dr3xx.cpp @@ -161,6 +161,15 @@ struct C : A {}; struct D : B, C {}; void f() { + // NB: the warning here is correct despite being the opposite of the + // comments in the catch handlers. The "unreachable" comment is correct + // because there is an ambiguous base path to A from the D that is thrown. + // The warnings generated are also correct because the handlers handle + // const B& and const A& and we don't check to see if other derived classes + // exist that would cause an ambiguous base path. We issue the diagnostic + // despite the potential for a false positive because users are not + // expected to have ambiguous base paths all that often, so the false + // positive rate should be acceptably low. try { throw D(); } catch (const A&) { // expected-note {{for type 'const A &'}} diff --git a/clang/test/SemaCXX/unreachable-catch-clauses.cpp b/clang/test/SemaCXX/unreachable-catch-clauses.cpp --- a/clang/test/SemaCXX/unreachable-catch-clauses.cpp +++ b/clang/test/SemaCXX/unreachable-catch-clauses.cpp @@ -9,5 +9,25 @@ void test() try {} 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}} +catch (Ex1 &e) { f(); } // expected-warning {{exception of type 'Ex1 &' will be caught by earlier handler}} \ + expected-note {{for type 'Ex1 &'}} +// FIXME: It would be nicer to only issue one warning on the below line instead +// of two. We get two diagnostics because the first one is noticing that there +// is a class hierarchy inversion where the earlier base class handler will +// catch throwing the derived class and the second one is because Ex2 and Ex1 +// are the same type after canonicalization. +catch (Ex2 &e) { f(); } // expected-warning 2{{exception of type 'Ex2 &' (aka 'Ex1 &') will be caught by earlier handler}} + +namespace GH61177 { +void func() { + const char arr[4] = "abc"; + + // We should not issue an "exception will be caught by earlier handler" + // diagnostic, as that is a lie. + try { + throw arr; + } catch (char *p) { + } catch (const char *p) { + } +} +} // GH61177