Page MenuHomePhabricator

Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
ClosedPublic

Authored by jyu2 on May 18 2017, 12:14 PM.

Details

Summary

Throwing in the destructor is not good (C++11 change try to not allow see below). But in reality, those codes are exist.
C++11 [class.dtor]p3:

A declaration of a destructor that does not have an exception-specification is implicitly considered to have the same exception specification as an implicit declaration.

With this change, the application worked before may now run into runtime termination. May gold here is to emit a warning to provide only possible info to where the code may need to be changed.

First there is no way, in compile time to identify the “throw” really throw out of the function. Things like the call which throw out… To keep this simple, when “throw” is seen, checking its enclosing function(only destructor and dealloc functions) with noexcept(true) specifier emit warning.

Here is implementation detail:
A new member function CheckCXXThrowInNonThrowingFunc is added for class Sema in Sema.h. It is used in the call to both BuildCXXThrow and TransformCXXThrowExpr.

The function basic check if the enclosing function with non-throwing noexcept specifer, if so emit warning for it.

The example of warning message like:
k1.cpp:18:3: warning: ''~dependent_warn'' has a (possible implicit) non-throwing

    noexcept specifier. Throwing exception may cause termination.
    [-Wthrow-in-dtor]
throw 1;
^

k1.cpp:43:30: note: in instantiation of member function

'dependent_warn<noexcept_fun>::~dependent_warn' requested here

dependent_warn<noexcept_fun> f; // cause warning

Let me know if more information is needed.

Thanks.

Jennifer

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.Jun 1 2017, 9:55 AM
lib/Sema/AnalysisBasedWarnings.cpp
324 ↗(On Diff #100796)

Perhaps it's more clear as: doesThrowEscapePath()?

334 ↗(On Diff #100796)

You can drop the else here and just set HasThrowOutFunc to true.

337 ↗(On Diff #100796)

If OpLoc cannot be null, you should take it by reference. If it can be null, then you should guard against that here (and drop the parens).

338–340 ↗(On Diff #100796)

You can use a range-based for loop over Block.succs()

376 ↗(On Diff #100796)

Elide braces.

384 ↗(On Diff #100796)

Can use range-based for loop over succs().

420 ↗(On Diff #100796)

Elide braces.

2282 ↗(On Diff #100796)

Space after //.
out in non-throwing -> out of a non-throwing

test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #100796)

Please drop the svn auto props.

jyu2 updated this revision to Diff 101167.Jun 1 2017, 9:49 PM
jyu2 marked an inline comment as done.

Update to address review comments.

jyu2 marked 8 inline comments as done.Jun 1 2017, 9:53 PM
jyu2 added inline comments.
lib/Sema/AnalysisBasedWarnings.cpp
334 ↗(On Diff #100796)

Can not do that, don't if block has throw expression yet.

jyu2 added a comment.Jun 1 2017, 9:56 PM
In D33333#768332, @jyu2 wrote:

Okay this CFG version of this change. In this change I am basic using same algorithm with -Winfinite-recursion.

In addition to my original implementation, I add handler type checking which basic using https://reviews.llvm.org/D19201 method.

Thank you, I think this is a step in the right direction!

There are couple things I am worry about this implementation:
1> compile time...

Do you have any timing data on whether this has a negative performance impact?

2> Correctness...

Your implementation looks reasonable to me, but with further review (and good tests), we should have a better grasp on correctness.

3> Stack overflow for large CFG...

I would be surprised if that were a problem, but is something we could address if it ever arises.

Hope that is okay.

jyu2 marked 12 inline comments as done.Jun 1 2017, 9:59 PM
jyu2 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
6341 ↗(On Diff #100796)

use you suggested.

lib/Sema/AnalysisBasedWarnings.cpp
290 ↗(On Diff #100796)

:-(

304 ↗(On Diff #100796)

Good catch. Remove that check.

jyu2 marked 2 inline comments as done.Jun 2 2017, 9:00 AM
jyu2 added inline comments.
lib/Sema/AnalysisBasedWarnings.cpp
334 ↗(On Diff #100796)

Yes, Eric just point out, you are right, I can remove the line of "else"

jyu2 updated this revision to Diff 101373.Jun 5 2017, 12:59 AM

Add more test include 1> throw/catch reference types. 2> try block. 3>unreachable code.

jyu2 marked 2 inline comments as done.Jun 5 2017, 1:00 AM
jyu2 marked 3 inline comments as done.Jun 5 2017, 1:05 AM

Adding Richard to the review for some wider perspective than just mine on the overall design.

lib/Sema/AnalysisBasedWarnings.cpp
296 ↗(On Diff #101373)

If ThrowType can be null, there should be a null pointer check here. If it cannot be null, you should use getTypePtr() above instead of getTypePtrOrNull().

304 ↗(On Diff #101373)

The check for a null CaughtType can be hoisted above the if statements, which can then be simplified.

312 ↗(On Diff #101373)

There's really no point to using a local variable for this. You can return true above and return false here.

315 ↗(On Diff #101373)

isThrowCaughtByHandlers (drop the Be)

317 ↗(On Diff #101373)

Can you hoist the getNumHandlers() call out? e.g., for (unsigned H = 0, E = TryStmt->getNumHandlers(); H < E; ++H)

318 ↗(On Diff #101373)

No need for the local variable, can just call getHandler() in the call expression.

325 ↗(On Diff #101373)

Since OpLoc cannot be null, pass it by reference instead.

337 ↗(On Diff #101373)

This should use const CFGBlock::AdjacentBlock & to avoid the copy, but could also use const auto & if you wanted (since it's a range-based for loop).

338 ↗(On Diff #101373)

Instead of testing that I can be dereferenced (which is a bit odd since I is not a pointer, but uses a conversion operator), can you test I->isReachable() instead?

I think a better way to write this might be:

if (!I->isReachable())
  continue;

if (const CXXTryStmt *Terminator = dyn_cast_or_null<CXXTryStmt>(I->getTerminator())) {
}
344 ↗(On Diff #101373)

There's no need for this local variable. You can return true here.

349 ↗(On Diff #101373)

We don't usually mark locals as const unless it's a pointer or reference.

373 ↗(On Diff #101373)

Same comment here as above.

374 ↗(On Diff #101373)

Same comment here as above.

375 ↗(On Diff #101373)

You can drop the parens around I. Also, next_ID should be something like NextID per the coding style guidelines.

405 ↗(On Diff #101373)

cfg should be renamed per coding style guidelines. How about BodyCFG?

416 ↗(On Diff #101373)

No need for the if statement since castAs<> will assert if the function type is not FunctionProtoType.

290 ↗(On Diff #100796)

This should be isThrowCaught() (drop the Be).

292 ↗(On Diff #100796)

Should be IsCaught by coding convention.

330 ↗(On Diff #100796)

This is marked as done but doesn't appear to have been done.

test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #100796)

This does not appear to be done.

jyu2 updated this revision to Diff 102754.Jun 15 2017, 4:49 PM

Address Aaron's comments.

jyu2 updated this revision to Diff 102759.Jun 15 2017, 5:00 PM
jyu2 marked 13 inline comments as done.
jyu2 marked 7 inline comments as done.Jun 15 2017, 5:05 PM
jyu2 added inline comments.
lib/Sema/AnalysisBasedWarnings.cpp
296 ↗(On Diff #101373)

Good catch. Add code and test to handle this

312 ↗(On Diff #101373)

Right. Changed

315 ↗(On Diff #101373)

removed "Be"

aaron.ballman added inline comments.Jun 16 2017, 6:15 AM
lib/Sema/AnalysisBasedWarnings.cpp
304 ↗(On Diff #101373)

This can still be hoisted higher in the function.

297 ↗(On Diff #102759)

nit: !ThrowType

299 ↗(On Diff #102759)

No need for testing ThrowType for null here.

305 ↗(On Diff #102759)

No need for testing CaughtType for null here.

332 ↗(On Diff #102759)

const auto *

341 ↗(On Diff #102759)

Oops, I should have suggested const auto * here with my original comment (sorry about that).

393 ↗(On Diff #102759)

For consistency with other code in the compiler, can you move the Sema param to be the first parameter?

jyu2 updated this revision to Diff 102828.Jun 16 2017, 8:06 AM

Thanks Aaron!!! I just upload new patch to address your comments. I now understand your point on when I can use auto.

jyu2 marked 7 inline comments as done.Jun 16 2017, 8:08 AM

A few more nits, but this feels like it's getting close to ready (at least, to me).

test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #100796)

This file still has the svn auto props.

1 ↗(On Diff #102828)

I believe you can drop the -fcxx-exceptions as it should be implied by -fexceptions.

27 ↗(On Diff #102828)

Can you add a test case like:

struct Throws {
  ~Throws() noexcept(false);
};

struct ShouldDiagnose {
  Throws T;
  ~ShouldDiagnose() {}
};

I would expect ~ShouldDiagnose() to be diagnosed as allowing exceptions to escape because of the destructor for Throws.

rnk added inline comments.Jun 16 2017, 1:45 PM
test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #102828)

It isn't at the -cc1 level, you need -fcxx-exceptions there. -fexceptions controls landingpad cleanup emission.

aaron.ballman added inline comments.Jun 16 2017, 1:46 PM
test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #102828)

Ah, thank you Reid, I forgot about that.

jyu2 added inline comments.Jun 16 2017, 1:56 PM
test/SemaCXX/warn-throw-out-noexcept-func.cpp
1 ↗(On Diff #100796)

I am so sorry. Remove it.

1 ↗(On Diff #102828)

I can remove -fexceptions, since I only care for syntax.

27 ↗(On Diff #102828)

In C++11, destructors are implicitly throw() unless any member or base of the type has a destructor with a different exception specification.

In the case of:
struct Throws {

~Throws() noexcept(false);

};

struct ShouldDiagnose {

Throws T;
~ShouldDiagnose() {}

};

You should not see diagnose for ~ShouldDiagnose() , since ShouldDiagnose has a member ofr Throws which has destructor with noexcept(false); therefor

~ShouldDiagnose has  noexcept(false).

But I add test case which remove (false) part.

jyu2 updated this revision to Diff 102868.Jun 16 2017, 1:57 PM

Update patch

aaron.ballman added inline comments.Jun 16 2017, 1:59 PM
test/SemaCXX/warn-throw-out-noexcept-func.cpp
27 ↗(On Diff #102828)

Good point! A test case with noexcept(false) would be handy as would one where ~ShouldDiagnose() is marked noexcept(true) explicitly rather than picking up the noexcept(false) implicitly.

jyu2 updated this revision to Diff 102877.Jun 16 2017, 2:27 PM

update patch

test/SemaCXX/warn-throw-out-noexcept-func.cpp
27 ↗(On Diff #102828)

Okay I add two tests ShouldDiagnoes and ShouldNotDiagnoes.

jyu2 marked 2 inline comments as done.Jun 19 2017, 10:25 AM
Prazek removed a subscriber: Prazek.Jun 19 2017, 1:58 PM
jyu2 updated this revision to Diff 103519.Jun 21 2017, 9:14 PM

Update test.

aaron.ballman accepted this revision.Jun 22 2017, 5:25 AM

Aside from a few small nits with the test cases, this LGTM! You should hold off on committing for a day or two in case Richard or others have comments on the patch.

test/SemaCXX/warn-throw-out-noexcept-func.cpp
5–6 ↗(On Diff #103519)

The first test case showing a diagnostic or a note in the test should spell out the full text of the diagnostics. The remainder of the diagnostic checks can be shortened somewhat, but this ensures the full diagnostic text is properly checked.

8–11 ↗(On Diff #103519)

Why name this ShouldDiag if it doesn't diagnose?

126 ↗(On Diff #103519)

Add a space before the noexcept.

131 ↗(On Diff #103519)

Add a space before the {.

This revision is now accepted and ready to land.Jun 22 2017, 5:25 AM
This revision was automatically updated to reflect the committed changes.
sberg added a subscriber: sberg.Jun 29 2017, 11:01 AM

see also https://reviews.llvm.org/rL306715 "Fixed -Wexceptions derived-to-base false positives"

I tested the latest revision (this fronted patch already included) on my test file in D33537. Disregarding of the (not so important) check-specific parameters (EnabledFunctions and IgnoredExceptions) I do not get warnings for any indirect throws (indirect_implicit() and indirect_explicit()). So for me this frontend check does not seem to be using the CFG. Furthermore, I do not get warning for throw_and_catch_some() where 1.1 is a double thus catch(int &) should not catch it. The same happens in throw_catch_rethrow_the_rest(), where catch(int &) should not catch 1.1, but catch(...) should catch and rethrow it. Is this latter one a bug?