[clang] 0950332 - Fix false positive with unreachable C++ catch handlers
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 14 08:08:48 PDT 2023
Author: Aaron Ballman
Date: 2023-03-14T11:08:39-04:00
New Revision: 0950332e91df0281c386874c45d7ce33b7da495b
URL: https://github.com/llvm/llvm-project/commit/0950332e91df0281c386874c45d7ce33b7da495b
DIFF: https://github.com/llvm/llvm-project/commit/0950332e91df0281c386874c45d7ce33b7da495b.diff
LOG: Fix false positive with unreachable C++ catch handlers
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 https://github.com/llvm/llvm-project/issues/61177
Differential Revision: https://reviews.llvm.org/D145408
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/lib/Sema/SemaStmt.cpp
clang/test/CXX/drs/dr3xx.cpp
clang/test/SemaCXX/unreachable-catch-clauses.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 66a9210c35b66..a52af53bbd28b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -193,6 +193,10 @@ Bug Fixes in This Version
(`#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
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index f15603fd0bd4a..eda4e01f8f0f0 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -4351,9 +4351,9 @@ class CatchHandlerType {
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 @@ template <> struct DenseMapInfo<CatchHandlerType> {
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 @@ StmtResult Sema::ActOnCXXTryBlock(SourceLocation TryLoc, Stmt *TryBlock,
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 @@ StmtResult Sema::ActOnCXXTryBlock(SourceLocation TryLoc, Stmt *TryBlock,
// 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 @@ StmtResult Sema::ActOnCXXTryBlock(SourceLocation TryLoc, Stmt *TryBlock,
// 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 @@ StmtResult Sema::ActOnCXXTryBlock(SourceLocation TryLoc, Stmt *TryBlock,
<< 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
index cca9470bcfd6c..e3fe8d932b30a 100644
--- a/clang/test/CXX/drs/dr3xx.cpp
+++ b/clang/test/CXX/drs/dr3xx.cpp
@@ -161,6 +161,15 @@ namespace dr308 { // dr308: yes
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
index 8dcb47b13c771..5041332779e64 100644
--- a/clang/test/SemaCXX/unreachable-catch-clauses.cpp
+++ b/clang/test/SemaCXX/unreachable-catch-clauses.cpp
@@ -9,5 +9,25 @@ void f();
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
More information about the cfe-commits
mailing list