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

[jyu2]

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

[rnk]

I think we should consider generalizing this to throwing from any noexcept function. We could add a special case diagnostic to explain that destructors and delete operators are noexcept by default in C++11.

It's also probably a good idea to silence this warning if there are any try scopes around the exception being thrown.

[lebedev.ri]

Relevant D31370, D19201

[jyu2]

Reid,
Thank you so much for your comments. I upload new patch to address your suggestion.
1> Emit warning for throw exception in all noexcept function. And special diagnostic note for destructor and delete operators.
2> Silence this warning when the throw inside try block.

Let me know if more information is needed.

Thanks.

Jennifer

[aaron.ballman]

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

[Prazek]

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

[jyu2]

In D33333#760149, @aaron.ballman wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

In D33333#760353, @Prazek wrote:

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

[aaron.ballman]

In D33333#760419, @jyu2 wrote:

In D33333#760149, @aaron.ballman wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

Sort of correct. clang-tidy doesn't necessarily do more intensive checking than the compiler. It's more an AST matching source checking tool for checks that are either expensive or have a higher false-positive rate than we'd want to see in the frontend.

I think that this particular check should probably be in the frontend, like you're doing. My biggest concern is that we do not duplicate functionality between the two tools. D19201 has has a considerable amount of review, so you should look at the test cases there and ensure you are handling them (including the fixits). Hopefully your patch ends up covering all of the functionality in the clang-tidy patch.

In D33333#760353, @Prazek wrote:

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

[jyu2]

In D33333#760431, @aaron.ballman wrote:

In D33333#760419, @jyu2 wrote:

In D33333#760149, @aaron.ballman wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

Sort of correct. clang-tidy doesn't necessarily do more intensive checking than the compiler. It's more an AST matching source checking tool for checks that are either expensive or have a higher false-positive rate than we'd want to see in the frontend.

I think that this particular check should probably be in the frontend, like you're doing. My biggest concern is that we do not duplicate functionality between the two tools. D19201 has has a considerable amount of review, so you should look at the test cases there and ensure you are handling them (including the fixits). Hopefully your patch ends up covering all of the functionality in the clang-tidy patch.

In D33333#760353, @Prazek wrote:

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

[aaron.ballman]

In D33333#761126, @jyu2 wrote:

In D33333#760431, @aaron.ballman wrote:

In D33333#760419, @jyu2 wrote:

In D33333#760149, @aaron.ballman wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

Sort of correct. clang-tidy doesn't necessarily do more intensive checking than the compiler. It's more an AST matching source checking tool for checks that are either expensive or have a higher false-positive rate than we'd want to see in the frontend.

I think that this particular check should probably be in the frontend, like you're doing. My biggest concern is that we do not duplicate functionality between the two tools. D19201 has has a considerable amount of review, so you should look at the test cases there and ensure you are handling them (including the fixits). Hopefully your patch ends up covering all of the functionality in the clang-tidy patch.

In D33333#760353, @Prazek wrote:

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:

void f() noexcept { // Should not diagnose
  try {
    throw 12;
  } catch (int) {
  }
}

void g() noexcept { // Should not diagnose
  try {
    throw 12;
  } catch (...) {
  }
}

void h() noexcept { // Should diagnose
  try {
    throw 12;
  } catch (const char *) {
  }
}

void i() noexcept { // Should diagnose
  try {
    throw 12;
  } catch (int) {
    throw;
  }
}

void j() noexcept { // Should diagnose
  try {
    throw 12;
  } catch (int) {
    throw "haha";
  }
}

void k() noexcept { // Should diagnose
  try {
    throw 12;
  } catch (...) {
    throw;
  }
}

I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

include/clang/Basic/DiagnosticGroups.td

140 ↗

(On Diff #99517)

No need to add a group for this; you can define one inline with the warning, but perhaps the exceptions group already covers it.

include/clang/Basic/DiagnosticSemaKinds.td

6335 ↗

(On Diff #99517)

Do not quote %0. Also, remove the full-stop and capitalized "Throwing". Diagnostics are not complete sentences. I think it should probably read "%0 has a non-throwing exception specification but can still throw, resulting in unexpected program termination" (or something along those lines).

6342 ↗

(On Diff #99517)

Rather than make the note spell out all the various ways a function can be nothrow, we should either say "nonthrowing function declare here" or specify which reason the function is not throwing using %select.

include/clang/Sema/Sema.h

4970 ↗

(On Diff #99517)

with non-throwing -> with a non-throwing

lib/Sema/SemaExprCXX.cpp

690 ↗

(On Diff #99517)

Please run clang-format over the patch (the asterisk binds to FD and the curly brace should be on this line).

692 ↗

(On Diff #99517)

Can use const auto * here.

693–696 ↗

(On Diff #99517)

Is there a reason for doing this rather than simply calling FunctionProtoType::isNothrow()?

712 ↗

(On Diff #99517)

Please spell out the type explicitly here.

713 ↗

(On Diff #99517)

Remove spurious parens.

716 ↗

(On Diff #99517)

Same here.

test/CXX/except/except.spec/p11.cpp

5 ↗

(On Diff #99517)

Please modify this test to expect diagnostics rather than remove the noexcept from the test. This is testing specific features of the standard.

test/SemaCXX/warn-throw-out-dtor.cpp

1 ↗

(On Diff #99517)

Please remove the svn props from this file.

[rnk]

Re: clang-tidy, I would rather implement this as a traditional compiler warning.

In D33333#761208, @aaron.ballman wrote:

In D33333#761126, @jyu2 wrote:

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:
...
I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

I agree with @jyu2, we should do something simple. I think it would be simple to handle all cases except for h, which requires semantic analysis to figure out if the thrown object would be caught by the catch parameter. Implementing that is more likely to introduce bugs in the compiler than it is to find bugs in user code. I doubt we need the CFG for any of this. The later examples are all throwing exceptions from catch blocks, which is the same as not having an open try scope.

[aaron.ballman]

In D33333#761224, @rnk wrote:

Re: clang-tidy, I would rather implement this as a traditional compiler warning.

In D33333#761208, @aaron.ballman wrote:

In D33333#761126, @jyu2 wrote:

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:
...
I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

I agree with @jyu2, we should do something simple. I think it would be simple to handle all cases except for h, which requires semantic analysis to figure out if the thrown object would be caught by the catch parameter. Implementing that is more likely to introduce bugs in the compiler than it is to find bugs in user code.

I'm not certain I agree with the assertion it's more likely to introduce bugs in the compiler than find bugs in user code. Machine-generated code runs into silly things like this, and it's nice to warn the user about it. However, it may be reasonable to do that in a follow-up patch -- but that patch is likely to have a lot of churn since it's not really plausible to implement with the way @jyu2 has structured this patch. By making this an analysis-based warning instead, that would alleviate my concerns (but would likely require implementing the CFG approach anyway).

I doubt we need the CFG for any of this. The later examples are all throwing exceptions from catch blocks, which is the same as not having an open try scope.

Sure, if you cut out the cases that require a CFG, you don't need a CFG for it. :-P Use of a CFG would eliminate obvious false-positives such as h().

Also, there should be some tests with function-try-blocks.

[jyu2]

In D33333#761233, @aaron.ballman wrote:

In D33333#761224, @rnk wrote:

Re: clang-tidy, I would rather implement this as a traditional compiler warning.

In D33333#761208, @aaron.ballman wrote:

In D33333#761126, @jyu2 wrote:

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:
...
I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

I agree with @jyu2, we should do something simple. I think it would be simple to handle all cases except for h, which requires semantic analysis to figure out if the thrown object would be caught by the catch parameter. Implementing that is more likely to introduce bugs in the compiler than it is to find bugs in user code.

I'm not certain I agree with the assertion it's more likely to introduce bugs in the compiler than find bugs in user code. Machine-generated code runs into silly things like this, and it's nice to warn the user about it. However, it may be reasonable to do that in a follow-up patch -- but that patch is likely to have a lot of churn since it's not really plausible to implement with the way @jyu2 has structured this patch. By making this an analysis-based warning instead, that would alleviate my concerns (but would likely require implementing the CFG approach anyway).

I doubt we need the CFG for any of this. The later examples are all throwing exceptions from catch blocks, which is the same as not having an open try scope.

Sure, if you cut out the cases that require a CFG, you don't need a CFG for it. :-P Use of a CFG would eliminate obvious false-positives such as h().

Also, there should be some tests with function-try-blocks.

Sure, I will add that try-block test for this.

I am agree with @rnk keep this simple.

[jyu2]

This is new version should address all @Aaron's commands, but CFG part.

I would not think we should go that far in the compiler for this.

Thanks.

[jyu2]

I missed two place which Aaron point out.
1> using isNothrow function instead NR_Nothrow.
2> A format problem.

Changed.
Thanks.

[jyu2]

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.

There are couple things I am worry about this implementation:
1> compile time...
2> Correctness...
3> Stack overflow for large CFG...

[aaron.ballman]

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.

include/clang/Basic/DiagnosticSemaKinds.td

6341 ↗

(On Diff #100796)

throw, may -> throw, which may

Also, remove the period at the end of the diagnostic.

6344 ↗

(On Diff #100796)

possible -> possibly

6347 ↗

(On Diff #100796)

nonthrowing -> non-throwing

(to be consistent with other diagnostics.)

lib/Sema/AnalysisBasedWarnings.cpp

290 ↗

(On Diff #100796)

Caughted -> Caught

292 ↗

(On Diff #100796)

MayCaught -> IsCaught ?

293–294 ↗

(On Diff #100796)

Please don't use auto unless the type is explicitly spelled out in the initializer (here and elsewhere).

298 ↗

(On Diff #100796)

You can combine this with the above check.

304 ↗

(On Diff #100796)

This does not seem quite correct. Consider:

struct S{};

void f() {
  try {
    throw S{};
  } catch (const S *s) {
  }
}

311 ↗

(On Diff #100796)

mayThrowBeCaughtedByHandlers -> isThrowCaughtByAHandler

314 ↗

(On Diff #100796)

h -> H (or Idx, or anything else that meets the coding standard).

317–318 ↗

(On Diff #100796)

Just return true here instead of setting a local variable. Return false below.

330 ↗

(On Diff #100796)

Can use const auto * here.

[aaron.ballman]

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]

Update to address review comments.

[jyu2]

lib/Sema/AnalysisBasedWarnings.cpp

334 ↗

(On Diff #100796)

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

[jyu2]

In D33333#770238, @aaron.ballman wrote:

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]

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]

lib/Sema/AnalysisBasedWarnings.cpp

334 ↗

(On Diff #100796)

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

[jyu2]

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

[aaron.ballman]

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]

Address Aaron's comments.

[jyu2]

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]

lib/Sema/AnalysisBasedWarnings.cpp

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?

304 ↗

(On Diff #101373)

This can still be hoisted higher in the function.

[jyu2]

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

[aaron.ballman]

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 #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.

1 ↗

(On Diff #100796)

This file still has the svn auto props.

[rnk]

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]

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

1 ↗

(On Diff #102828)

Ah, thank you Reid, I forgot about that.

[jyu2]

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

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.

1 ↗

(On Diff #100796)

I am so sorry. Remove it.

[jyu2]

Update patch

[aaron.ballman]

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]

update patch

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

27 ↗

(On Diff #102828)

Okay I add two tests ShouldDiagnoes and ShouldNotDiagnoes.

[jyu2]

Update test.

[aaron.ballman]

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 {.

[sberg]

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

[baloghadamsoftware]

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?

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_exception_caught_by_earlier_handler : Warning<
   "exception of type %0 will be caught by earlier handler">,
   InGroup<Exceptions>;
 def note_previous_exception_handler : Note<"for type %0">;
 def err_exceptions_disabled : Error<
   "cannot use '%0' with exceptions disabled">;
 def err_objc_exceptions_disabled : Error<
   "cannot use '%0' with Objective-C exceptions disabled">;
+def warn_throw_in_noexcept_func 
+    : Warning<"%0 has a non-throwing exception specification but can still "
+      "throw, resulting in unexpected program termination">,
+      InGroup<Exceptions>;
+def note_throw_in_dtor 
+    : Note<"destructor or deallocator has a (possibly implicit) non-throwing "
+      "excepton specification">;
+def note_throw_in_function 
+    : Note<"non-throwing function declare here">;
 def err_seh_try_outside_functions : Error<
   "cannot use SEH '__try' in blocks, captured regions, or Obj-C method decls">;
 def err_mixing_cxx_try_seh_try : Error<
   "cannot use C++ 'try' in the same function as SEH '__try'">;
 def err_seh_try_unsupported : Error<
   "SEH '__try' is not supported on this target">;
 def note_conflicting_try_here : Note<
   "conflicting %0 here">;

cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp

if (cfg->getExit().pred_empty())
     return;
 
   // Emit diagnostic if a recursive function call is detected for all paths.
   if (checkForRecursiveFunctionCall(FD, cfg))
     S.Diag(Body->getLocStart(), diag::warn_infinite_recursive_function);
 }
 
 //===----------------------------------------------------------------------===//
+// Check for throw in a non-throwing function.
+//===----------------------------------------------------------------------===//
+enum ThrowState {
+  FoundNoPathForThrow,
+  FoundPathForThrow,
+  FoundPathWithNoThrowOutFunction,
+};
+
+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;
+  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();
+  if (CaughtType == ThrowType)
+    return true;
+  const CXXRecordDecl *CaughtAsRecordType =
+      CaughtType->getPointeeCXXRecordDecl();
+  const CXXRecordDecl *ThrowTypeAsRecordType = ThrowType->getAsCXXRecordDecl();
+  if (CaughtAsRecordType && ThrowTypeAsRecordType)
+    return ThrowTypeAsRecordType->isDerivedFrom(CaughtAsRecordType);
+  return false;
+}
+
+static bool isThrowCaughtByHandlers(const CXXThrowExpr *CE,
+                                    const CXXTryStmt *TryStmt) {
+  for (unsigned H = 0, E = TryStmt->getNumHandlers(); H < E; ++H) {
+    if (isThrowCaught(CE, TryStmt->getHandler(H)))
+      return true;
+  }
+  return false;
+}
+
+static bool doesThrowEscapePath(CFGBlock Block, SourceLocation &OpLoc) {
+  for (const auto &B : Block) {
+    if (B.getKind() != CFGElement::Statement)
+      continue;
+    const auto *CE = dyn_cast<CXXThrowExpr>(B.getAs<CFGStmt>()->getStmt());
+    if (!CE)
+      continue;
+
+    OpLoc = CE->getThrowLoc();
+    for (const auto &I : Block.succs()) {
+      if (!I.isReachable())
+        continue;
+      if (const auto *Terminator =
+              dyn_cast_or_null<CXXTryStmt>(I->getTerminator()))
+        if (isThrowCaughtByHandlers(CE, Terminator))
+          return false;
+    }
+    return true;
+  }
+  return false;
+}
+
+static bool hasThrowOutNonThrowingFunc(SourceLocation &OpLoc, CFG *BodyCFG) {
+
+  unsigned ExitID = BodyCFG->getExit().getBlockID();
+
+  SmallVector<ThrowState, 16> States(BodyCFG->getNumBlockIDs(),
+                                     FoundNoPathForThrow);
+  States[BodyCFG->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;
+
+  SmallVector<CFGBlock *, 16> Stack;
+  Stack.push_back(&BodyCFG->getEntry());
+  while (!Stack.empty()) {
+    CFGBlock *CurBlock = Stack.back();
+    Stack.pop_back();
+
+    unsigned ID = CurBlock->getBlockID();
+    ThrowState CurState = States[ID];
+    if (CurState == FoundPathWithNoThrowOutFunction) {
+      if (ExitID == ID)
+        continue;
+
+      if (doesThrowEscapePath(*CurBlock, OpLoc))
+        CurState = FoundPathForThrow;
+    }
+
+    // Loop over successor blocks and add them to the Stack if their state
+    // changes.
+    for (const auto &I : CurBlock->succs())
+      if (I.isReachable()) {
+        unsigned NextID = I->getBlockID();
+        if (NextID == ExitID && CurState == FoundPathForThrow) {
+          States[NextID] = CurState;
+        } else if (States[NextID] < CurState) {
+          States[NextID] = CurState;
+          Stack.push_back(I);
+        }
+      }
+  }
+  // Return true if the exit node is reachable, and only reachable through
+  // a throw expression.
+  return States[ExitID] == FoundPathForThrow;
+}
+
+static void EmitDiagForCXXThrowInNonThrowingFunc(Sema &S, SourceLocation OpLoc,
+                                                 const FunctionDecl *FD) {
+  if (!S.getSourceManager().isInSystemHeader(OpLoc)) {
+    S.Diag(OpLoc, diag::warn_throw_in_noexcept_func) << FD;
+    if (S.getLangOpts().CPlusPlus11 &&
+        (isa<CXXDestructorDecl>(FD) ||
+         FD->getDeclName().getCXXOverloadedOperator() == OO_Delete ||
+         FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete))
+      S.Diag(FD->getLocation(), diag::note_throw_in_dtor);
+    else
+      S.Diag(FD->getLocation(), diag::note_throw_in_function);
+  }
+}
+
+static void checkThrowInNonThrowingFunc(Sema &S, const FunctionDecl *FD,
+                                        AnalysisDeclContext &AC) {
+  CFG *BodyCFG = AC.getCFG();
+  if (!BodyCFG)
+    return;
+  if (BodyCFG->getExit().pred_empty())
+    return;
+  SourceLocation OpLoc;
+  if (hasThrowOutNonThrowingFunc(OpLoc, BodyCFG))
+    EmitDiagForCXXThrowInNonThrowingFunc(S, OpLoc, FD);
+}
+
+static bool isNoexcept(const FunctionDecl *FD) {
+  const auto *FPT = FD->getType()->castAs<FunctionProtoType>();
+  if (FPT->getExceptionSpecType() != EST_None &&
+      FPT->isNothrow(FD->getASTContext()))
+    return true;
+  return false;
+}
+
+//===----------------------------------------------------------------------===//
 // Check for missing return value.
 //===----------------------------------------------------------------------===//
 
 enum ControlFlowKind {
   UnknownFallThrough,
   NeverFallThrough,
   MaybeFallThrough,
   AlwaysFallThrough,
AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
   // Check for infinite self-recursion in functions
   if (!Diags.isIgnored(diag::warn_infinite_recursive_function,
                        D->getLocStart())) {
     if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
       checkRecursiveFunction(S, FD, Body, AC);
     }
   }
 
+  // Check for throw out of non-throwing function.
+  if (!Diags.isIgnored(diag::warn_throw_in_noexcept_func, D->getLocStart()))
+    if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
+      if (S.getLangOpts().CPlusPlus && isNoexcept(FD))
+        checkThrowInNonThrowingFunc(S, FD, AC);
+
   // If none of the previous checks caused a CFG build, trigger one here
   // for -Wtautological-overlap-compare
   if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
                                D->getLocStart())) {
     AC.getCFG();
   }
 
   // Collect statistics about the CFG if it was built.

cfe/trunk/test/CXX/except/except.spec/p11.cpp

 // RUN: %clang_cc1 -std=c++11 -fexceptions -fcxx-exceptions -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 // This is the "let the user shoot themselves in the foot" clause.
-void f() noexcept {
-  throw 0; // no-error
+void f() noexcept { // expected-note {{non-throwing function declare here}}
+  throw 0; // expected-warning {{has a non-throwing exception specification but}} 
 }
-void g() throw() {
-  throw 0; // no-error
+void g() throw() { // expected-note {{non-throwing function declare here}}
+  throw 0; // expected-warning {{has a non-throwing exception specification but}} 
 }
 void h() throw(int) {
   throw 0.0; // no-error
 }