aaron.ballman created this revision.
aaron.ballman added reviewers: erichkeane, shafik, clang-language-wg.
Herald added a project: All.
aaron.ballman requested review of this revision.
Herald added a project: clang.
This addresses an issue found by WG21 and tracked by CWG2699 (which is not yet
publicly published). The basic problem is that Clang issues a diagnostic about
not being able to reach a handler, but that handler *is* reached at runtime.
Clang's diagnostic behavior was matching the standard wording, and our runtime
behavior was matching the standard's intent.
This fixes the diagnostic so that it matches the runtime behavior more closely,
and reduces the number of false positives. This is the direction of choice
taken by Core for CWG2699 and it seems unlikely that WG21 will change direction
here.
Fixes #61177
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D145408
Files:
clang/docs/ReleaseNotes.rst
clang/lib/Sema/SemaStmt.cpp
clang/test/CXX/drs/dr3xx.cpp
clang/test/SemaCXX/unreachable-catch-clauses.cpp
Index: clang/test/SemaCXX/unreachable-catch-clauses.cpp
===================================================================
--- clang/test/SemaCXX/unreachable-catch-clauses.cpp
+++ 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
Index: clang/test/CXX/drs/dr3xx.cpp
===================================================================
--- clang/test/CXX/drs/dr3xx.cpp
+++ 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 &'}}
Index: clang/lib/Sema/SemaStmt.cpp
===================================================================
--- clang/lib/Sema/SemaStmt.cpp
+++ 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<CatchHandlerType, CXXCatchStmt *> &TypesToCheck;
- const bool CheckAgainstPointer;
+ const llvm::DenseMap<QualType, CXXCatchStmt *> &TypesToCheck;
CXXCatchStmt *FoundHandler;
- CanQualType FoundHandlerType;
+ QualType FoundHandlerType;
+ QualType TestAgainstType;
public:
- CatchTypePublicBases(
- ASTContext &Ctx,
- const llvm::DenseMap<CatchHandlerType, CXXCatchStmt *> &T, bool C)
- : Ctx(Ctx), TypesToCheck(T), CheckAgainstPointer(C),
- FoundHandler(nullptr) {}
+ CatchTypePublicBases(ASTContext &Ctx,
+ const llvm::DenseMap<QualType, CXXCatchStmt *> &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<QualType, CXXCatchStmt *> HandledBaseTypes;
llvm::DenseMap<CatchHandlerType, CXXCatchStmt *> HandledTypes;
for (unsigned i = 0; i < NumHandlers; ++i) {
CXXCatchStmt *H = cast<CXXCatchStmt>(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(),
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -180,6 +180,10 @@
(`#60405 <https://github.com/llvm/llvm-project/issues/60405>`_)
- Fix aggregate initialization inside lambda constexpr.
(`#60936 <https://github.com/llvm/llvm-project/issues/60936>`_)
+- No longer issue a false positive diagnostic about a catch handler that cannot
+ be reached despite being reachable. This fixes
+ `#61177 <https://github.com/llvm/llvm-project/issues/61177>`_ in anticipation
+ of `CWG2699 <https://wg21.link/CWG2699>_` being accepted by WG21.
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
