Fix false positive with unreachable C++ catch handlers

[aaron.ballman]

Summary

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

[erichkeane]

clang/docs/ReleaseNotes.rst

199

This link seems broken? I get 404 from it.

clang/lib/Sema/SemaStmt.cpp

4354

Do we do this because we don't want the pointee unqualified later? Else these seem identical to me.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

199

Correct: (which is not yet publicly published) from the summary. That link will become valid whenever CWG issues the next update of the public issues list.

clang/lib/Sema/SemaStmt.cpp

4354

We want to strip the top-level qualifiers, and the previous code was stripping qualifiers off the pointee, not the pointer.

[aaron.ballman]

Ping

[shafik]

Apologies for late review.

clang/lib/Sema/SemaStmt.cpp

4405

Can we just make llvm::DenseMap<QualType, CXXCatchStmt *> a typedef so we don't have the verbose type in the function parameter right below and further down.

clang/docs/ReleaseNotes.rst

- Fix __VA_OPT__ implementation so that it treats the concatenation of a
   non-placemaker token and placemaker token as a non-placemaker token.
   (`#60268 <https://github.com/llvm/llvm-project/issues/60268>`_)
 - Fix crash when taking the address of a consteval lambda call operator.
   (`#57682 <https://github.com/llvm/llvm-project/issues/57682>`_)
 - Clang now support export declarations in the language linkage.
   (`#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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

clang/lib/Sema/SemaStmt.cpp

 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 top
   /// level pointer and cv-qualifiers.
   CatchHandlerType(QualType Q) : QT(Q), IsPointer(false) {
     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
   /// type passed in had the pointer qualifier, does not need to get an
   /// unqualified type.
   CatchHandlerType(QualType QT, bool IsPointer)
       : QT(QT), IsPointer(IsPointer) {}
 
static bool isEqual(const CatchHandlerType &LHS,
     return LHS == RHS;
   }
 };
 }
 
 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()) {
+        // 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 = Ctx.getCanonicalType(S->getType());
+          FoundHandlerType = Check;
           return true;
         }
       }
+    }
     return false;
   }
 };
 }
 
 /// ActOnCXXTryBlock - Takes a try compound-statement and a number of
 /// handlers and creates a try statement from them.
 StmtResult Sema::ActOnCXXTryBlock(SourceLocation TryLoc, Stmt *TryBlock,
if (!getLangOpts().Borland && FSI->FirstSEHTryLoc.isValid()) {
     Diag(TryLoc, diag::err_mixing_cxx_try_seh_try) << 0;
     Diag(FSI->FirstSEHTryLoc, diag::note_conflicting_try_here) << "'__try'";
   }
 
   const unsigned NumHandlers = Handlers.size();
   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]);
 
     // 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->getBeginLoc(), diag::err_early_catch_all));
       continue;
     } else if (H->getExceptionDecl()->isInvalidDecl())
       continue;
 
     // 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
     // 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());
+      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();
           Diag(Problem->getExceptionDecl()->getTypeSpecStartLoc(),
                 diag::note_previous_exception_handler)
               << 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(),
            diag::warn_exception_caught_by_earlier_handler)
           << H->getCaughtType();
       Diag(Problem->getExceptionDecl()->getTypeSpecStartLoc(),
            diag::note_previous_exception_handler)
           << Problem->getCaughtType();

clang/test/CXX/drs/dr3xx.cpp

 
 namespace dr308 { // dr308: yes
   // This is mostly an ABI library issue.
   struct A {};
   struct B : A {};
   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 &'}}
       // unreachable
     } catch (const B&) { // expected-warning {{exception of type 'const B &' will be caught by earlier handler}}
       // get here instead
     }
   }

clang/test/SemaCXX/unreachable-catch-clauses.cpp

 // RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
 
 class BaseEx {};
 class Ex1: public BaseEx {};
 typedef Ex1 Ex2;
 
 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