[clang-tidy] handle exceptions properly in `ExceptionAnalyzer`

[isuckatcs]

Summary

Prior to this patch ExceptionAnalyzer reported many false
positives due to insufficient checking of conversion rules
inside exception handlers.

One false positive example from the previous version:

void foo() noexcept {
  try {
    int a = 1;
    throw &a;
  } catch (const int *) {
  }
}

In function foo() the &a is caught by the handler, but clang-tidy
reports the following warning:

warning: an exception may be thrown in function 'foo' which should not throw exceptions [bugprone-exception-escape]

The standard says the following about exception handling:

[N4849 14.4 Handling an exception]

(3) A handler is a match for an exception object of type E if

• The handler is of type cv T or cv T& and E and T are the same type (ignoring the top-level cv-qualifiers), or
• the handler is of type cv T or cv T& and T is an unambiguous public base class of E, or
• the handler is of type cv T or const T& where T is a pointer or pointer-to-member type and E is a pointer or pointer-to-member type that can be converted to T by one or more of
  • a standard pointer conversion (7.3.11) not involving conversions to pointers to private or protected or ambiguous classes
  • a function pointer conversion (7.3.13)
  • a qualification conversion (7.3.5), or
• the handler is of type cv T or const T& where T is a pointer or pointer-to-member type and E is std::nullptr_t.

This patch is supposed to implement all of these checks.

[isuckatcs]

Updated

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

68

Please don't use auto unless type is spelled in same statement or iterator. Same below.

[isuckatcs]

Addressed inline comment

[njames93]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

81

This harms readability and it doesn't cover extended qualifiers. I believe Qualifiers::isStrictSuperSet would be a better candidate to use here.

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

105

These check not lines are useless and should be removed, same goes below.

[isuckatcs]

Addressed comments and added support for multi-level pointers too.

[isuckatcs]

I've found a strange scenario.

The following conversions are allowed

double *a[2][3];
double const * const (*ap)[3] = a; // OK
double * const (*ap1)[] = a;       // OK since C++20

However if the same conversion is supposed to be performed in a catch() statement, it's not happening and the thrown object is not caught.
See it on godbolt.

Quoteing cppreference:

When an exception is thrown by any statement in compound-statement, the exception object of type E is matched against the types of the formal parameters T of each catch-clause in handler-seq, in the order in which the catch clauses are listed. The exception is a match if any of the following is true:

...
- T is (possibly cv-qualified) U or const U& (since C++14), and U is a pointer or pointer to member type, and E is also a pointer or pointer to member type that is implicitly convertible to U by one or more of
  - a standard pointer conversion other than one to a private, protected, or ambiguous base class
  - **a qualification conversion**
  - a function pointer conversion (since C++17)
...

Checking the quality conversion related part of cppreference lists the examples I quoted before as performable conversions.

[isuckatcs]

ping

[xazax.hun]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

60

Did you take a look at ASTContext::hasCvrSimilarType? I wonder if it is already doing what you want.

[xazax.hun]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

60

Oh, maybe it is not, but it might also make sense to take a look at ASTContext::typesAreCompatible.

[xazax.hun]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

95

If none of the functions I recommended works for you, I still strongly suggest reusing utilities from ASTContext, like UnwrapSimilarTypes and UnwrapSimilarArrayTypes.

[isuckatcs]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

60

Did you take a look at ASTContext::hasCvrSimilarType? I wonder if it is already doing what you want.

This function compares the types by removing the CVR qualifiers.

Oh, maybe it is not, but it might also make sense to take a look at ASTContext::typesAreCompatible.

This one only compares the canonical types if the language is C++.

if (getLangOpts().CPlusPlus)
    return hasSameType(LHS, RHS);
----------------------------
bool hasSameType(QualType T1, QualType T2) const {
    return getCanonicalType(T1) == getCanonicalType(T2);
  }

95

I didn't check all of the functions inside ASTContext, but here we have very specific rules, we need to check. One utility might help with one check or two, but won't replace the need to go ove all of them. Also I think it's easier to understand which section checks what, if I keep them separated.

In an ealier comment I link to the section on cppreference, which discusses what these rules are. Also there I found one controversial example, that's only detected by MSVC. I wonder if you know why that happens.

[xazax.hun]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

60

Could you rename the arguments like To, From or Exception, Catch or something to make the direction of the conversion clearer?

95

I see. It is unfortunate that we cannot simply reuse the corresponding functionality from Sema. The code mostly looks good to me, apart from some nits.

111

If this rule only applies to C++20 and above, consider adding a language version check for LangOpts.

130–131

Looking at the documentation of DesugaredType:

This takes off typedefs, typeof's etc. If the outer level of the type is already concrete, it returns it unmodified. This is similar to getting the canonical type, but it doesn't remove all typedefs. For example, it returns "T*" as "T*", (not as "int*"), because the pointer is concrete.

I wonder if we actually want to compare canonical types rather than desugared types.

[xazax.hun]

Also, could you open a bug report about the strange exception behavior on GitHub? Hopefully someone working on conformance can take a look.

[isuckatcs]

Addressed comments

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

95

It is unfortunate that we cannot simply reuse the corresponding functionality from Sema.

It is indeed unfortunate, though I wonder if we want to move this functionality to ASTContext, so that it can be reused outside the ExceptionAnalyzer.

[xazax.hun]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

81

Here I am also wondering if we actually want canonical types.

95

I am not opposed to that, but I think in that case we want this method to be used in the regular compilation pipeline to make sure it is correct.

[isuckatcs]

• Renamed and moved isMultiLevelConvertiblePointer() to ASTContext
• Added a unit test to test the said function

[xazax.hun]

Since now the patch touches Clang proper, adding one more reviewer.

[xazax.hun]

Overall looks good to me, I wonder if the tests could be less manual though. E.g., instead of asserting true/false, checking if the assignment would compile. This way we can be sure that the method in ASTContext matches the behavior of the compiler (and we get notified when the two diverge). If we could extract the corresponding code from Sema, it would be even better, but I do not insist as that might be a lot of work depending on how it interacts with other conversions.

[erichkeane]

Not a great reviewer here as I'm not particularly sure what is going on, but I 'looked at the clang parts.

clang/include/clang/AST/ASTContext.h

2828 ↗

(On Diff #489940)

Please document what this is doing...

2829 ↗

(On Diff #489940)

Why is this static if you need lang-opts? This should be retrieved by the ASTContext itself.

2831 ↗

(On Diff #489940)

What does this name mean here? It isn't clear to me.

clang/include/clang/AST/Type.h

7364 ↗

(On Diff #489940)

This changes the meaning here, and this is a commonly used thing. Why are you doing this?

clang/lib/AST/ASTContext.cpp

13465 ↗

(On Diff #489940)

Perhaps this should be asserted on!

13466 ↗

(On Diff #489940)

I find myself shocked we don't have something like this already, but what do we mean by 'qualification convertible'? Is that a term of art I'm missing?

13485 ↗

(On Diff #489940)

I would expect at least the 'to' here to assert as well. Passing a 'not pointer' as the 'two' when youre testing 'convertible pointer' is odd and a mistake?

13489 ↗

(On Diff #489940)

debatable whether we can use 'auto' here. I'd lean toward 'no'.

13492 ↗

(On Diff #489940)

I think the recursion here is making this too complicated iwth the "IsConstSoFar" and "Level", I'd probably suggest splitting these tasks up.

[isuckatcs]

Addressed comments
Updated tests

[isuckatcs]

E.g., instead of asserting true/false, checking if the assignment would compile.

This is actually biased. If the result of the compilation is different from the result we get from this function, it can also mean a bug in the compiler.

Take a look at this example on godbolt. The snippet here is only valid from C++20, but GCC compiles it even in C++17.

clang/include/clang/AST/ASTContext.h

2828 ↗

(On Diff #489940)

This was actually documented, but at the definition. My bad.

2829 ↗

(On Diff #489940)

By taking the language options externally, we might be able to produce more descriptive warning messages in the future.
E.g.: This conversion is only valid from C++20, etc.

Also calling this function doesn't depend on ASTContext any other way, so it can be called even if we don't have access to the ASTContext for some reason.

I don't know however if it makes sense to worry about these uses at all.

2831 ↗

(On Diff #489940)

This is documented at the use site of this variable. I couldn't come up with a better name for it.

// If at the current level To is more cv-qualified than From [...],
// then there must be a 'const' at every single level (other than level zero)
// of To up until the current level
bool MoreCVQualified =
    To.getQualifiers().isStrictSupersetOf(From.getQualifiers());
if (MoreCVQualified)
  Convertible &= IsToConstSoFar;

clang/include/clang/AST/Type.h

7364 ↗

(On Diff #489940)

I missed that it changes the meaning. Though I need this use case, so I reverted these changes and created a static global function instead.

clang/lib/AST/ASTContext.cpp

13465 ↗

(On Diff #489940)

I personally don't want to assert it, as it won't crash in C mode, it's just the fact that some rules here are different in C.
E.g. (from cppreference):

char** p = 0;
const char* const * p2 = p; // error in C, OK in C++

(The example above is checked properly but I didn't dig deeper into the other C rules, that's why I said that it shouldn't be called)

13466 ↗

(On Diff #489940)

I didn't come up with this name. It is what this conversion is called by the standard.

It is §7.3.5 in N4849 and you can also find it under the same name on cppreference.

13485 ↗

(On Diff #489940)

Well, technically MemberPointerType is also accepted and it's not derived from PointerType. Though I added an assertion here, but I left the check so that we don't crash in release mode.

13492 ↗

(On Diff #489940)

Splitting those up would just make it more complicated I think. We would need to perform nearly the same checks in both branches, though I keep thinking about it.

[isuckatcs]

This patch got a little bit out of control at the last revision, so I decided to remove every change from clang and add everything to the ExceptionAnalyzer only.
The reason for that is with exceptions we have less conversions to check than we have inside the compiler, which can lead to confusion.

For example:

class A {};
class B : public A {};

int A::* pa;
int B::* pb = pa; <-- valid outside of exception handler, invalid in exception handler

We can have the conversion int B::* pb = pa; because of 7.3.12 Pointer-to-member conversions, which is by standard not performed when an exception needs to be caught.
See godbolt. (MSVC does catch A::* with a B::* handler for some reason, maybe I miss some flag)

For the above reason, sadly we can't test the changes the way you suggested @xazax.hun, like checking if the assigment compiles or not.

[isuckatcs]

• Reverted the changes related to clang itself
• Added more checks for exception handling
• isQualificationConvertiblePointer is now iterative, not recursive
• Added more test cases

[isuckatcs]

applied clang-format

[isuckatcs]

ping

[xazax.hun]

Overall, I like the structure of this patch and the references back to the standard. But every time we compare type pointers, they might compare inequal when one of the types have sugar on it while the other does not. Please review all of those comparisons to see where do we need to get the canonical types instead, and add some tests with type aliases.

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

66

Will this work with typedefs and other sugar or do you need to get the canonical type here? I do not see test coverage for that scenario.

84

What about Type::getPointeeOrArrayElementType?

[isuckatcs]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

84

I didn't use that for two reasons.

1. It returns a Type instead of a QualType, so the qualifiers are lost.
2. It uses Type::getBaseElementTypeUnsafe() for arrays, which discards all array types for multi-dimensional arrays. E.g.: int[2][3] will become int and I want to take only one step backward, so I want int[2][3] to become int[2].

I was thinking about inserting this function into Type or QualType but because of the different behaviour with arrays I decided to use it as a helper in this source file only instead. I agree that the naming is confusing a bit, but it is literally what the function does.

[isuckatcs]

Addressed comments

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

66

In this specific case it will work because getAsCXXRecordDecl() comes from the CanProxyBase utility class, which already canonicalizes the type, but I agree that it's not intuitive and unreadable.

I also modified the current test cases to have aliases.

[xazax.hun]

Thanks!

[isuckatcs]

Added tests for function pointers

[isuckatcs]

@xazax.hun can we merge this or should we wait for someone else's approval too?

[xazax.hun]

I think at this point it is ok to merge. Any other comments can be addressed in follow-up commits.

[isuckatcs]

fixed failing test

[DavidSpickett]

The test was timing out on Arm/AArch64 bots so I have reverted this change.

I think because there were other test failures on the initial run, that obscured the time out. This is one of the first builds to include this change: https://lab.llvm.org/buildbot/#/builders/188/builds/26480 Note that there is an unrelated failure, and the stdio shows a timeout.

PASS: lit :: shtest-define.py (70393 of 70394)
command timed out: 1200 seconds without output running [b'ninja', b'check-all'], attempting to kill

I confirmed that on the bot itself, it was exception-escape that was holding things up. Local bisect backs that up.

If this is not happening outside of Arm/AArch64 I can help you understand what went wrong (though I will be finishing work for the week shortly, back Monday).

[DavidSpickett]

If it helps, the bot I linked is pretty vanilla. You can find all the details in the buildbot UI but just in case:

cmake -G Ninja ../llvm/llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=True '-DLLVM_LIT_ARGS='"'"'-v'"'"'' -DCMAKE_INSTALL_PREFIX=../stage1.install '-DLLVM_TARGETS_TO_BUILD='"'"'AArch64'"'"'' '-DLLVM_ENABLE_PROJECTS=llvm;clang;clang-tools-extra'

We build with clang 15.0.0 release. Though I guess it's not a compiler related issue.

[isuckatcs]

I'm not sure if I can run clang targetting AArch64 on an X86 machine but I tried it regardless and I get compiler errors on startup, so I can't debug this issue at the moment.
Besided that I don't know why it times out on that buildbot. The build job that is performed on every revision passed without any problems.

[erichkeane]

In D135495#4151238, @isuckatcs wrote:

I'm not sure if I can run clang targetting AArch64 on an X86 machine but I tried it regardless and I get compiler errors on startup, so I can't debug this issue at the moment.
Besided that I don't know why it times out on that buildbot. The build job that is performed on every revision passed without any problems.

As long as you don't need the library (which, if you have a preprocessed file or a test, should absolutely not be necessary), you can just do the -cc1 option of -triple <whatever>, or the non- -cc1 option of -target <whatever>.

Both use basically the same options list, and that bot seems to be "aarch64-unknown-linux-gnu" (see under cmake-stage-1 the llvm host triple).

[isuckatcs]

Fixed the timed out test

[isuckatcs]

As long as you don't need the library (which, if you have a preprocessed file or a test, should absolutely not be necessary), you can just do the -cc1 option of -triple <whatever>, or the non- -cc1 option of -target <whatever>.

Both use basically the same options list, and that bot seems to be "aarch64-unknown-linux-gnu" (see under cmake-stage-1 the llvm host triple)

Thanks for this hint, it helped a lot!

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

static bool isBaseOf(const Type *DerivedType, const Type *BaseType) {
   if (!DerivedClass || !BaseClass)
     return false;
 
   return !DerivedClass->forallBases(
       [BaseClass](const CXXRecordDecl *Cur) { return Cur != BaseClass; });
 }
 
 bool ExceptionAnalyzer::ExceptionInfo::filterByCatch(const Type *BaseClass) {
   llvm::SmallVector<const Type *, 8> TypesToDelete;
   for (const Type *T : ThrownExceptions) {
     if (T == BaseClass || isBaseOf(T, BaseClass))
       TypesToDelete.push_back(T);
+    else if (T->isPointerType() && BaseClass->isPointerType()) {
+      auto BPointeeTy = BaseClass->getAs<PointerType>()->getPointeeType();
+      auto TPointeeTy = T->getAs<PointerType>()->getPointeeType();
+
+      auto BCVR = BPointeeTy.getCVRQualifiers();
+      auto TCVR = TPointeeTy.getCVRQualifiers();
+
+      // In case the unqualified types are the same, the exception will be
+      // caught if
+      //  1.) the thrown type doesn't have qualifiers
+      //  2.) the handler has the same qualifiers as the thrown type
+      //  3.) the handle has more qualifiers than the thrown type
+      if (BPointeeTy->getUnqualifiedDesugaredType() ==
+              TPointeeTy->getUnqualifiedDesugaredType() &&
+          (TCVR == 0 || (BCVR ^ TCVR) == 0 || (BCVR & TCVR) > BCVR)) {
+        TypesToDelete.push_back(T);
+      }
+    }
   }
 
   for (const Type *T : TypesToDelete)
     ThrownExceptions.erase(T);
 
   reevaluateBehaviour();
   return TypesToDelete.size() > 0;
 }
 
 ExceptionAnalyzer::ExceptionInfo &
 ExceptionAnalyzer::ExceptionInfo::filterIgnoredExceptions(
     const llvm::StringSet<> &IgnoredTypes, bool IgnoreBadAlloc) {
   llvm::SmallVector<const Type *, 8> TypesToDelete;
   // Note: Using a 'SmallSet' with 'llvm::remove_if()' is not possible.
   // Therefore this slightly hacky implementation is required.
   for (const Type *T : ThrownExceptions) {
     if (const auto *TD = T->getAsTagDecl()) {
       if (TD->getDeclName().isIdentifier()) {
         if ((IgnoreBadAlloc &&
              (TD->getName() == "bad_alloc" && TD->isInStdNamespace())) ||
             (IgnoredTypes.count(TD->getName()) > 0))
           TypesToDelete.push_back(T);
       }
     }
   }
   for (const Type *T : TypesToDelete)
     ThrownExceptions.erase(T);
 
   reevaluateBehaviour();
   return *this;
 }
 
 void ExceptionAnalyzer::ExceptionInfo::clear() {
   Behaviour = State::NotThrowing;
   ContainsUnknown = false;
   ThrownExceptions.clear();
 }
 
 void ExceptionAnalyzer::ExceptionInfo::reevaluateBehaviour() {
   if (ThrownExceptions.size() == 0)
     if (ContainsUnknown)
       Behaviour = State::Unknown;
     else
       Behaviour = State::NotThrowing;
   else
     Behaviour = State::Throwing;
 }
 
 ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
     const FunctionDecl *Func,
     llvm::SmallSet<const FunctionDecl *, 32> &CallStack) {
   if (CallStack.count(Func))
     return ExceptionInfo::createNonThrowing();
 
   if (const Stmt *Body = Func->getBody()) {
     CallStack.insert(Func);
     ExceptionInfo Result =
         throwsException(Body, ExceptionInfo::Throwables(), CallStack);

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

try {
     if (n) throw 1;
     throw 1.1;
   } catch(int &) {
   } catch(...) {
     throw;
   }
 }
 
+void throw_catch_pointer_c() noexcept {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_pointer_c' which should not throw exceptions
+  try {
+    int a = 1;
+    throw &a;
+  } catch(const int *) {}
+}
+
+void throw_catch_pointer_v() noexcept {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_pointer_v' which should not throw exceptions
+  try {
+    int a = 1;
+    throw &a;
+  } catch(volatile int *) {}
+}
+
+void throw_catch_pointer_cv() noexcept {
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_pointer_cv' which should not throw exceptions
+  try {
+    int a = 1;
+    throw &a;
+  } catch(const volatile int *) {}
+}
+
+void throw_c_catch_pointer() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer' which should not throw exceptions
+  try {
+    int a = 1;
+    const int *p = &a;
+    throw p;
+  } catch(int *) {}
+}
+
+void throw_c_catch_pointer_v() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer_v' which should not throw exceptions
+  try {
+    int a = 1;
+    const int *p = &a;
+    throw p;
+  } catch(volatile int *) {}
+}
+
+void throw_v_catch_pointer() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_v_catch_pointer' which should not throw exceptions
+  try {
+    int a = 1;
+    volatile int *p = &a;
+    throw p;
+  } catch(int *) {}
+}
+
+void throw_v_catch_pointer_c() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_v_catch_pointer_c' which should not throw exceptions
+  try {
+    int a = 1;
+    volatile int *p = &a;
+    throw p;
+  } catch(const int *) {}
+}
+
+void throw_cv_catch_pointer_c() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_cv_catch_pointer_c' which should not throw exceptions
+  try {
+    int a = 1;
+    const volatile int *p = &a;
+    throw p;
+  } catch(const int *) {}
+}
+
+void throw_cv_catch_pointer_v() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_cv_catch_pointer_v' which should not throw exceptions
+  try {
+    int a = 1;
+    const volatile int *p = &a;
+    throw p;
+  } catch(volatile int *) {}
+}
+
 class base {};
 class derived: public base {};
 
 void throw_derived_catch_base() noexcept {
   // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base' which should not throw exceptions
   try {
     throw derived();
   } catch(base &) {