[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

296–299

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

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

296–299

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

296–299

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!

lib/Sema/AnalysisBasedWarnings.cpp

 };
 
 static bool isThrowCaught(const CXXThrowExpr *Throw,
                           const CXXCatchStmt *Catch) {
   const Type *ThrowType = nullptr;
   if (Throw->getSubExpr())
     ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
   if (!ThrowType)
-    return false;
+    return true;
   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();

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; //expected-warning {{has a non-throwing exception specification but}}
+    throw;
   } catch (...) {
   }
 }
+// In fact, a rethrow with ANY catch should be trusted here, otherwise it is
+// going to result in many false positives.
+void p_ShouldNotDiag() noexcept {
+  try {
+    throw;
+  } catch (int){
+  }
+}
 
 #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}}
 }
 
 void with_try_block() try {
   throw 2;