[CFG] Relax Wexceptions warning on rethrow

[erichkeane]

Summary

As reported here: https://bugs.llvm.org/show_bug.cgi?id=34973

"catch(...)" should catch EVERYTHING, even a rethrow. This
patch changes the order in which things are checked to ensure
that a '...' catch will get a rethrow.

[erichkeane]

Have a question on the behavior here, see inline.

lib/Sema/AnalysisBasedWarnings.cpp

299 ↗

(On Diff #119353)

The other potential fix here is to simply change this line here to "return true;". This would result the warning being suppressed on 'rethrow' if there is any catch statements.

I wonder what the opinion of the reviewers is on this one.

test/SemaCXX/warn-throw-out-noexcept-func.cpp

244 ↗

(On Diff #119353)

Doh, I should have changed the name here... Still want your feedback on above though.

[erichkeane]

I've convinced myself that a re-throw with a catch around it should not be diagnosed, otherwise there is no way to suppress the warning in this case. It ends up being a false-positive in many cases.

[aaron.ballman]

In D39013#900046, @erichkeane wrote:

I've convinced myself that a re-throw with a catch around it should not be diagnosed, otherwise there is no way to suppress the warning in this case. It ends up being a false-positive in many cases.

Wouldn't a catch-all handler suppress the warning in that case? e.g.,

void f() noexcept {
  try {
    throw; // rethrows
  } catch (int) {
    // We hope this catches it, and if it always does, then this is a fp warning.
  } catch (...) {
    // However, without cross-TU analysis, we can't know that detail and it seems plausible that
    // the active exception object is something else, so the catch-all is not unreasonable and adds
    // extra safety. The diagnostic (even if a fp) requires the developer to think about this case.
    assert(false);
  }
}

lib/Sema/AnalysisBasedWarnings.cpp

299 ↗

(On Diff #119353)

I'm a little bit concerned about it, truth be told. The current behavior has false positives, and this patch trades those for false negatives. The question boils down to which is the more likely scenario. Rethrow from within a try block that catches the exception being rethrown seems like a rather unlikely scenario to me. Do you have examples of this being a false positive with real-world code?

I do agree that the catch-all handler should silence the diagnostic, however.

[erichkeane]

Wouldn't a catch-all handler suppress the warning in that case? e.g.,

Yep, you're right of course. I'll revert to my first patch and update this. I'd surmised that false-positives were worse than false-negatives, but since there is a trivial workaround, I'm ok with that. Thanks for the quick review!

lib/Sema/AnalysisBasedWarnings.cpp

299 ↗

(On Diff #119353)

I don't have any examples, just the bug report.

[erichkeane]

Changes as requested by @aaron.ballman

[aaron.ballman]

LGTM! Thank you for the fix!

cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp

 enum ThrowState {
   FoundNoPathForThrow,
   FoundPathForThrow,
   FoundPathWithNoThrowOutFunction,
 };
 
 static bool isThrowCaught(const CXXThrowExpr *Throw,
                           const CXXCatchStmt *Catch) {
+  const Type *CaughtType = Catch->getCaughtType().getTypePtrOrNull();
+  if (!CaughtType)
+    return true;
   const Type *ThrowType = nullptr;
   if (Throw->getSubExpr())
     ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
   if (!ThrowType)
     return false;
-  const Type *CaughtType = Catch->getCaughtType().getTypePtrOrNull();
-  if (!CaughtType)
-    return true;
   if (ThrowType->isReferenceType())
     ThrowType = ThrowType->castAs<ReferenceType>()
                     ->getPointeeType()
                     ->getUnqualifiedDesugaredType();
   if (CaughtType->isReferenceType())
     CaughtType = CaughtType->castAs<ReferenceType>()
                      ->getPointeeType()
                      ->getUnqualifiedDesugaredType();

cfe/trunk/test/SemaCXX/warn-throw-out-noexcept-func.cpp

 }
 void n_ShouldNotDiag() noexcept {
   try {
     S s = S{};
     throw s;
   } catch (const S &s) {
   }
 }
-void o_ShouldDiag() noexcept { //expected-note {{function declared non-throwing here}}
+// As seen in p34973, this should not throw the warning.  If there is an active
+// exception, catch(...) catches everything. 
+void o_ShouldNotDiag() noexcept {
+  try {
+    throw;
+  } catch (...) {
+  }
+}
+
+void p_ShouldDiag() noexcept { //expected-note {{function declared non-throwing here}}
   try {
     throw; //expected-warning {{has a non-throwing exception specification but}}
-  } catch (...) {
+  } catch (int){
+  }
+}
+
+void q_ShouldNotDiag() noexcept {
+  try {
+    throw;
+  } catch (int){
+  } catch (...){
   }
 }
 
 #define NOEXCEPT noexcept
 void with_macro() NOEXCEPT { //expected-note {{function declared non-throwing here}}
   throw 1; // expected-warning {{has a non-throwing exception specification but}}
 }