[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
baloghadamsoftware added a comment. I tested the latest revision (this fronted patch already included) on my test file in https://reviews.llvm.org/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? Repository: rL LLVM https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
sberg added a comment. see also https://reviews.llvm.org/rL306715 "Fixed -Wexceptions derived-to-base false positives" Repository: rL LLVM https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
This revision was automatically updated to reflect the committed changes.
Closed by commit rL306149: Emit warning when throw exception in destruct or
dealloc functions which has a (authored by erichkeane).
Changed prior to commit:
https://reviews.llvm.org/D3?vs=103519=103765#toc
Repository:
rL LLVM
https://reviews.llvm.org/D3
Files:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/test/CXX/except/except.spec/p11.cpp
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6351,6 +6351,15 @@
"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;
+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<
Index: cfe/trunk/test/CXX/except/except.spec/p11.cpp
===
--- cfe/trunk/test/CXX/except/except.spec/p11.cpp
+++ cfe/trunk/test/CXX/except/except.spec/p11.cpp
@@ -1,12 +1,11 @@
// 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
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -279,6 +279,150 @@
}
//===--===//
+// 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()
+->getPointeeType()
+->getUnqualifiedDesugaredType();
+ if (CaughtType->isReferenceType())
+CaughtType = CaughtType->castAs()
+ ->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 ) {
+ for (const auto : Block) {
+if (B.getKind() != CFGElement::Statement)
+ continue;
+const auto *CE = dyn_cast(B.getAs()->getStmt());
+if (!CE)
+ continue;
+
+OpLoc = CE->getThrowLoc();
+for (const auto : Block.succs()) {
+ if (!I.isReachable())
+continue;
+ if (const auto *Terminator =
+ dyn_cast_or_null(I->getTerminator()))
+if (isThrowCaughtByHandlers(CE, Terminator))
+ return false;
+}
+return true;
+ }
+ return false;
+}
+
+static bool hasThrowOutNonThrowingFunc(SourceLocation , CFG
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
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.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:5-6
+}; // implicitly noexcept(true)
+A_ShouldDiag::~A_ShouldDiag() { // expected-note {{destructor or deallocator
has a}}
+ throw 1; // expected-warning {{has a non-throwing exception specification
but}}
+}
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.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:8-11
+struct B_ShouldDiag {
+ int i;
+ ~B_ShouldDiag() noexcept(true) {}
+};
Why name this ShouldDiag if it doesn't diagnose?
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:126
+struct Throws {
+ ~Throws()noexcept(false);
+};
Add a space before the `noexcept`.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:131
+ Throws T;
+ ~ShouldDiagnose() noexcept{ //expected-note {{destructor or deallocator has
a}}
+throw; // expected-warning {{has a non-throwing exception specification
but}}
Add a space before the `{`.
Repository:
rL LLVM
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 102877.
jyu2 added a comment.
update patch
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/AnalysisBasedWarnings.cpp
test/CXX/except/except.spec/p11.cpp
test/SemaCXX/warn-throw-out-noexcept-func.cpp
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -279,6 +279,150 @@
}
//===--===//
+// 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()
+->getPointeeType()
+->getUnqualifiedDesugaredType();
+ if (CaughtType->isReferenceType())
+CaughtType = CaughtType->castAs()
+ ->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 ) {
+ for (const auto : Block) {
+if (B.getKind() != CFGElement::Statement)
+ continue;
+const auto *CE = dyn_cast(B.getAs()->getStmt());
+if (!CE)
+ continue;
+
+OpLoc = CE->getThrowLoc();
+for (const auto : Block.succs()) {
+ if (!I.isReachable())
+continue;
+ if (const auto *Terminator =
+ dyn_cast_or_null(I->getTerminator()))
+if (isThrowCaughtByHandlers(CE, Terminator))
+ return false;
+}
+return true;
+ }
+ return false;
+}
+
+static bool hasThrowOutNonThrowingFunc(SourceLocation , CFG *BodyCFG) {
+
+ unsigned ExitID = BodyCFG->getExit().getBlockID();
+
+ SmallVector States(BodyCFG->getNumBlockIDs(),
+ FoundNoPathForThrow);
+ States[BodyCFG->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;
+
+ SmallVector Stack;
+ Stack.push_back(>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 : 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 , 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(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 , const FunctionDecl *FD,
+AnalysisDeclContext ) {
+ CFG *BodyCFG = AC.getCFG();
+ if (!BodyCFG)
+
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 added inline comments.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:27
+}
+
+struct N : A {
aaron.ballman wrote:
> jyu2 wrote:
> > aaron.ballman wrote:
> > > 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`.
> > 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.
> 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.
Okay I add two tests ShouldDiagnoes and ShouldNotDiagnoes.
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
aaron.ballman added inline comments.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:27
+}
+
+struct N : A {
jyu2 wrote:
> aaron.ballman wrote:
> > 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`.
> 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.
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.
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 102868.
jyu2 added a comment.
Update patch
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/AnalysisBasedWarnings.cpp
test/CXX/except/except.spec/p11.cpp
test/SemaCXX/warn-throw-out-noexcept-func.cpp
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -279,6 +279,150 @@
}
//===--===//
+// 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()
+->getPointeeType()
+->getUnqualifiedDesugaredType();
+ if (CaughtType->isReferenceType())
+CaughtType = CaughtType->castAs()
+ ->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 ) {
+ for (const auto : Block) {
+if (B.getKind() != CFGElement::Statement)
+ continue;
+const auto *CE = dyn_cast(B.getAs()->getStmt());
+if (!CE)
+ continue;
+
+OpLoc = CE->getThrowLoc();
+for (const auto : Block.succs()) {
+ if (!I.isReachable())
+continue;
+ if (const auto *Terminator =
+ dyn_cast_or_null(I->getTerminator()))
+if (isThrowCaughtByHandlers(CE, Terminator))
+ return false;
+}
+return true;
+ }
+ return false;
+}
+
+static bool hasThrowOutNonThrowingFunc(SourceLocation , CFG *BodyCFG) {
+
+ unsigned ExitID = BodyCFG->getExit().getBlockID();
+
+ SmallVector States(BodyCFG->getNumBlockIDs(),
+ FoundNoPathForThrow);
+ States[BodyCFG->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;
+
+ SmallVector Stack;
+ Stack.push_back(>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 : 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 , 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(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 , const FunctionDecl *FD,
+AnalysisDeclContext ) {
+ CFG *BodyCFG = AC.getCFG();
+ if (!BodyCFG)
+
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 added inline comments.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:1
+// RUN: %clang_cc1 %s -fdelayed-template-parsing -fcxx-exceptions
-fexceptions -fsyntax-only -Wexceptions -verify -std=c++11
+struct A {
aaron.ballman wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Please drop the svn auto props.
> > This does not appear to be done.
> This file still has the svn auto props.
I am so sorry. Remove it.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:1
+// RUN: %clang_cc1 %s -fdelayed-template-parsing -fcxx-exceptions
-fexceptions -fsyntax-only -Wexceptions -verify -std=c++11
+struct A {
aaron.ballman wrote:
> rnk wrote:
> > aaron.ballman wrote:
> > > I believe you can drop the -fcxx-exceptions as it should be implied by
> > > -fexceptions.
> > It isn't at the -cc1 level, you need -fcxx-exceptions there. -fexceptions
> > controls landingpad cleanup emission.
> Ah, thank you Reid, I forgot about that.
I can remove -fexceptions, since I only care for syntax.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:27
+}
+
+struct N : A {
aaron.ballman wrote:
> 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`.
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.
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
aaron.ballman added inline comments.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:1
+// RUN: %clang_cc1 %s -fdelayed-template-parsing -fcxx-exceptions
-fexceptions -fsyntax-only -Wexceptions -verify -std=c++11
+struct A {
rnk wrote:
> aaron.ballman wrote:
> > I believe you can drop the -fcxx-exceptions as it should be implied by
> > -fexceptions.
> It isn't at the -cc1 level, you need -fcxx-exceptions there. -fexceptions
> controls landingpad cleanup emission.
Ah, thank you Reid, I forgot about that.
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
rnk added inline comments.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:1
+// RUN: %clang_cc1 %s -fdelayed-template-parsing -fcxx-exceptions
-fexceptions -fsyntax-only -Wexceptions -verify -std=c++11
+struct A {
aaron.ballman wrote:
> I believe you can drop the -fcxx-exceptions as it should be implied by
> -fexceptions.
It isn't at the -cc1 level, you need -fcxx-exceptions there. -fexceptions
controls landingpad cleanup emission.
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
aaron.ballman added a comment.
A few more nits, but this feels like it's getting close to ready (at least, to
me).
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:1
+// RUN: %clang_cc1 %s -fdelayed-template-parsing -fcxx-exceptions
-fexceptions -fsyntax-only -Wexceptions -verify -std=c++11
+struct A {
aaron.ballman wrote:
> aaron.ballman wrote:
> > Please drop the svn auto props.
> This does not appear to be done.
This file still has the svn auto props.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:1
+// RUN: %clang_cc1 %s -fdelayed-template-parsing -fcxx-exceptions
-fexceptions -fsyntax-only -Wexceptions -verify -std=c++11
+struct A {
I believe you can drop the -fcxx-exceptions as it should be implied by
-fexceptions.
Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:27
+}
+
+struct N : A {
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`.
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 102828.
jyu2 added a comment.
Thanks Aaron!!! I just upload new patch to address your comments. I now
understand your point on when I can use auto.
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/AnalysisBasedWarnings.cpp
test/CXX/except/except.spec/p11.cpp
test/SemaCXX/warn-throw-out-noexcept-func.cpp
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -279,6 +279,150 @@
}
//===--===//
+// 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()
+->getPointeeType()
+->getUnqualifiedDesugaredType();
+ if (CaughtType->isReferenceType())
+CaughtType = CaughtType->castAs()
+ ->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 ) {
+ for (const auto : Block) {
+if (B.getKind() != CFGElement::Statement)
+ continue;
+const auto *CE = dyn_cast(B.getAs()->getStmt());
+if (!CE)
+ continue;
+
+OpLoc = CE->getThrowLoc();
+for (const auto : Block.succs()) {
+ if (!I.isReachable())
+continue;
+ if (const auto *Terminator =
+ dyn_cast_or_null(I->getTerminator()))
+if (isThrowCaughtByHandlers(CE, Terminator))
+ return false;
+}
+return true;
+ }
+ return false;
+}
+
+static bool hasThrowOutNonThrowingFunc(SourceLocation , CFG *BodyCFG) {
+
+ unsigned ExitID = BodyCFG->getExit().getBlockID();
+
+ SmallVector States(BodyCFG->getNumBlockIDs(),
+ FoundNoPathForThrow);
+ States[BodyCFG->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;
+
+ SmallVector Stack;
+ Stack.push_back(>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 : 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 , 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(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 , const FunctionDecl *FD,
+
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
aaron.ballman added inline comments.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:304
+ ->getUnqualifiedDesugaredType();
+ if (CaughtType == nullptr || CaughtType == ThrowType)
+return true;
aaron.ballman wrote:
> The check for a null `CaughtType` can be hoisted above the if statements,
> which can then be simplified.
This can still be hoisted higher in the function.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:297
+
+ if (ThrowType == nullptr)
+return false;
nit: `!ThrowType`
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:299
+return false;
+ if (ThrowType && ThrowType->isReferenceType())
+ThrowType = ThrowType->castAs()
No need for testing `ThrowType` for null here.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:305
+return true;
+ if (CaughtType && CaughtType->isReferenceType())
+CaughtType = CaughtType->castAs()
No need for testing `CaughtType` for null here.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:332
+ continue;
+const CXXThrowExpr *CE =
+dyn_cast(B.getAs()->getStmt());
`const auto *`
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:341
+continue;
+ if (const CXXTryStmt *Terminator =
+ dyn_cast_or_null(I->getTerminator()))
Oops, I should have suggested `const auto *` here with my original comment
(sorry about that).
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:393
+
+static void EmitDiagForCXXThrowInNonThrowingFunc(SourceLocation OpLoc, Sema ,
+ const FunctionDecl *FD) {
For consistency with other code in the compiler, can you move the `Sema` param
to be the first parameter?
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 marked 7 inline comments as done.
jyu2 added inline comments.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:296
+
+ if (ThrowType->isReferenceType())
+ThrowType = ThrowType->castAs()
aaron.ballman wrote:
> 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()`.
Good catch. Add code and test to handle this
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:312
+isCaught = ThrowTypeAsRecordType->isDerivedFrom(CaughtAsRecordType);
+ return isCaught;
+}
aaron.ballman wrote:
> There's really no point to using a local variable for this. You can return
> `true` above and return `false` here.
Right. Changed
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:315
+
+static bool isThrowBeCaughtByHandlers(const CXXThrowExpr *CE,
+ const CXXTryStmt *TryStmt) {
aaron.ballman wrote:
> `isThrowCaughtByHandlers` (drop the Be)
removed "Be"
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 102759.
jyu2 marked 13 inline comments as done.
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/AnalysisBasedWarnings.cpp
test/CXX/except/except.spec/p11.cpp
test/SemaCXX/warn-throw-out-noexcept-func.cpp
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -279,6 +279,152 @@
}
//===--===//
+// Check for throw in a non-throwing function.
+//===--===//
+enum ThrowState {
+ FoundNoPathForThrow,
+ FoundPathForThrow,
+ FoundPathWithNoThrowOutFunction,
+};
+
+static bool isThrowCaught(const CXXThrowExpr *Throw,
+ const CXXCatchStmt *Catch) {
+ const Type *CaughtType = Catch->getCaughtType().getTypePtrOrNull();
+ const Type *ThrowType = nullptr;
+ if (Throw->getSubExpr())
+ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
+
+ if (ThrowType == nullptr)
+return false;
+ if (ThrowType && ThrowType->isReferenceType())
+ThrowType = ThrowType->castAs()
+->getPointeeType()
+->getUnqualifiedDesugaredType();
+ if (CaughtType == nullptr)
+return true;
+ if (CaughtType && CaughtType->isReferenceType())
+CaughtType = CaughtType->castAs()
+ ->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 ) {
+ for (const auto : Block) {
+if (B.getKind() != CFGElement::Statement)
+ continue;
+const CXXThrowExpr *CE =
+dyn_cast(B.getAs()->getStmt());
+if (!CE)
+ continue;
+
+OpLoc = CE->getThrowLoc();
+for (const auto : Block.succs()) {
+ if (!I.isReachable())
+continue;
+ if (const CXXTryStmt *Terminator =
+ dyn_cast_or_null(I->getTerminator()))
+if (isThrowCaughtByHandlers(CE, Terminator))
+ return false;
+}
+return true;
+ }
+ return false;
+}
+
+static bool hasThrowOutNonThrowingFunc(SourceLocation , CFG *BodyCFG) {
+
+ unsigned ExitID = BodyCFG->getExit().getBlockID();
+
+ SmallVector States(BodyCFG->getNumBlockIDs(),
+ FoundNoPathForThrow);
+ States[BodyCFG->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;
+
+ SmallVector Stack;
+ Stack.push_back(>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 : 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(SourceLocation OpLoc, Sema ,
+ const FunctionDecl *FD) {
+ if (!S.getSourceManager().isInSystemHeader(OpLoc)) {
+S.Diag(OpLoc, diag::warn_throw_in_noexcept_func) << FD;
+if (S.getLangOpts().CPlusPlus11 &&
+(isa(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 , const FunctionDecl *FD,
+
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 102754.
jyu2 added a comment.
Address Aaron's comments.
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/AnalysisBasedWarnings.cpp
test/CXX/except/except.spec/p11.cpp
test/SemaCXX/warn-throw-out-noexcept-func.cpp
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -279,6 +279,152 @@
}
//===--===//
+// Check for throw in a non-throwing function.
+//===--===//
+enum ThrowState {
+ FoundNoPathForThrow,
+ FoundPathForThrow,
+ FoundPathWithNoThrowOutFunction,
+};
+
+static bool isThrowCaught(const CXXThrowExpr *Throw,
+ const CXXCatchStmt *Catch) {
+ const Type *CaughtType = Catch->getCaughtType().getTypePtrOrNull();
+ const Type *ThrowType = nullptr;
+ if (Throw->getSubExpr())
+ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
+
+ if (ThrowType == nullptr)
+return false;
+ if (ThrowType && ThrowType->isReferenceType())
+ThrowType = ThrowType->castAs()
+->getPointeeType()
+->getUnqualifiedDesugaredType();
+ if (CaughtType == nullptr)
+return true;
+ if (CaughtType && CaughtType->isReferenceType())
+CaughtType = CaughtType->castAs()
+ ->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 ) {
+ for (const clang::CFGElement : Block) {
+if (B.getKind() != CFGElement::Statement)
+ continue;
+const CXXThrowExpr *CE =
+dyn_cast(B.getAs()->getStmt());
+if (!CE)
+ continue;
+
+OpLoc = CE->getThrowLoc();
+for (const auto : Block.succs()) {
+ if (!I.isReachable())
+continue;
+ if (const CXXTryStmt *Terminator =
+ dyn_cast_or_null(I->getTerminator()))
+if (isThrowCaughtByHandlers(CE, Terminator))
+ return false;
+}
+return true;
+ }
+ return false;
+}
+
+static bool hasThrowOutNonThrowingFunc(SourceLocation , CFG *BodyCFG) {
+
+ unsigned ExitID = BodyCFG->getExit().getBlockID();
+
+ SmallVector States(BodyCFG->getNumBlockIDs(),
+ FoundNoPathForThrow);
+ States[BodyCFG->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;
+
+ SmallVector Stack;
+ Stack.push_back(>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 : 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(SourceLocation OpLoc, Sema ,
+ const FunctionDecl *FD) {
+ if (!S.getSourceManager().isInSystemHeader(OpLoc)) {
+S.Diag(OpLoc, diag::warn_throw_in_noexcept_func) << FD;
+if (S.getLangOpts().CPlusPlus11 &&
+(isa(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 , const FunctionDecl *FD,
+
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.
Adding Richard to the review for some wider perspective than just mine on the
overall design.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:296
+
+ if (ThrowType->isReferenceType())
+ThrowType = ThrowType->castAs()
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()`.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:304
+ ->getUnqualifiedDesugaredType();
+ if (CaughtType == nullptr || CaughtType == ThrowType)
+return true;
The check for a null `CaughtType` can be hoisted above the if statements, which
can then be simplified.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:312
+isCaught = ThrowTypeAsRecordType->isDerivedFrom(CaughtAsRecordType);
+ return isCaught;
+}
There's really no point to using a local variable for this. You can return
`true` above and return `false` here.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:315
+
+static bool isThrowBeCaughtByHandlers(const CXXThrowExpr *CE,
+ const CXXTryStmt *TryStmt) {
`isThrowCaughtByHandlers` (drop the Be)
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:317
+ const CXXTryStmt *TryStmt) {
+ for (unsigned H = 0; H < TryStmt->getNumHandlers(); ++H) {
+const CXXCatchStmt *CS = TryStmt->getHandler(H);
Can you hoist the `getNumHandlers()` call out? e.g., `for (unsigned H = 0, E =
TryStmt->getNumHandlers(); H < E; ++H)`
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:318
+ for (unsigned H = 0; H < TryStmt->getNumHandlers(); ++H) {
+const CXXCatchStmt *CS = TryStmt->getHandler(H);
+if (isThrowBeCaught(CE, CS))
No need for the local variable, can just call `getHandler()` in the call
expression.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:325
+
+static bool doesThrowEscapePath(CFGBlock , SourceLocation *OpLoc) {
+ bool HasThrowOutFunc = false;
Since `OpLoc` cannot be null, pass it by reference instead.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:337
+*OpLoc = CE->getThrowLoc();
+for (const CFGBlock::AdjacentBlock I : Block.succs())
+ if (I && I->getTerminator() && isa(I->getTerminator())) {
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).
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:338
+for (const CFGBlock::AdjacentBlock I : Block.succs())
+ if (I && I->getTerminator() && isa(I->getTerminator())) {
+const CXXTryStmt *Terminator = cast(I->getTerminator());
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(I->getTerminator())) {
}
```
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:344
+ }
+ return HasThrowOutFunc;
+}
There's no need for this local variable. You can return `true` here.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:349
+
+ const unsigned ExitID = cfg->getExit().getBlockID();
+
We don't usually mark locals as `const` unless it's a pointer or reference.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:373
+// changes.
+for (const CFGBlock::AdjacentBlock I : CurBlock->succs())
+ if (I) {
Same comment here as above.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:374
+for (const CFGBlock::AdjacentBlock I : CurBlock->succs())
+ if (I) {
+unsigned next_ID = (I)->getBlockID();
Same comment here as above.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:375
+ if (I) {
+unsigned next_ID = (I)->getBlockID();
+if (next_ID == ExitID && CurState == FoundPathForThrow) {
You can drop the parens around `I`. Also, `next_ID` should be something like
`NextID` per the coding style guidelines.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:405
+AnalysisDeclContext ) {
+ CFG *cfg = AC.getCFG();
+ if (!cfg)
`cfg` should be renamed per coding style guidelines. How about `BodyCFG`?
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:416
+static bool isNoexcept(const FunctionDecl *FD) {
+ if (const FunctionProtoType *FPT =
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 101373.
jyu2 added a comment.
Add more test include 1> throw/catch reference types. 2> try block.
3>unreachable code.
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/AnalysisBasedWarnings.cpp
test/CXX/except/except.spec/p11.cpp
test/SemaCXX/warn-throw-out-noexcept-func.cpp
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -279,6 +279,148 @@
}
//===--===//
+// Check for throw in a non-throwing function.
+//===--===//
+enum ThrowState {
+ FoundNoPathForThrow,
+ FoundPathForThrow,
+ FoundPathWithNoThrowOutFunction,
+};
+
+static bool isThrowBeCaught(const CXXThrowExpr *Throw,
+const CXXCatchStmt *Catch) {
+ bool isCaught = false;
+ const Type *ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
+ const Type *CaughtType = Catch->getCaughtType().getTypePtrOrNull();
+
+ if (ThrowType->isReferenceType())
+ThrowType = ThrowType->castAs()
+ ->getPointeeType()
+ ->getUnqualifiedDesugaredType();
+ if (CaughtType && CaughtType->isReferenceType())
+CaughtType = CaughtType->castAs()
+ ->getPointeeType()
+ ->getUnqualifiedDesugaredType();
+ if (CaughtType == nullptr || CaughtType == ThrowType)
+return true;
+
+ const CXXRecordDecl *CaughtAsRecordType =
+ CaughtType->getPointeeCXXRecordDecl();
+ const CXXRecordDecl *ThrowTypeAsRecordType = ThrowType->getAsCXXRecordDecl();
+ if (CaughtAsRecordType && ThrowTypeAsRecordType)
+isCaught = ThrowTypeAsRecordType->isDerivedFrom(CaughtAsRecordType);
+ return isCaught;
+}
+
+static bool isThrowBeCaughtByHandlers(const CXXThrowExpr *CE,
+ const CXXTryStmt *TryStmt) {
+ for (unsigned H = 0; H < TryStmt->getNumHandlers(); ++H) {
+const CXXCatchStmt *CS = TryStmt->getHandler(H);
+if (isThrowBeCaught(CE, CS))
+ return true;
+ }
+ return false;
+}
+
+static bool doesThrowEscapePath(CFGBlock , SourceLocation *OpLoc) {
+ bool HasThrowOutFunc = false;
+ for (const clang::CFGElement : Block) {
+if (B.getKind() != CFGElement::Statement)
+ continue;
+const CXXThrowExpr *CE =
+dyn_cast(B.getAs()->getStmt());
+if (!CE)
+ continue;
+
+HasThrowOutFunc = true;
+*OpLoc = CE->getThrowLoc();
+for (const CFGBlock::AdjacentBlock I : Block.succs())
+ if (I && I->getTerminator() && isa(I->getTerminator())) {
+const CXXTryStmt *Terminator = cast(I->getTerminator());
+if (isThrowBeCaughtByHandlers(CE, Terminator))
+ return false;
+ }
+ }
+ return HasThrowOutFunc;
+}
+
+static bool hasThrowOutNonThrowingFunc(SourceLocation *OpLoc, CFG *cfg) {
+
+ const unsigned ExitID = cfg->getExit().getBlockID();
+
+ SmallVector States(cfg->getNumBlockIDs(),
+ FoundNoPathForThrow);
+ States[cfg->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;
+
+ SmallVector Stack;
+ Stack.push_back(>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 CFGBlock::AdjacentBlock I : CurBlock->succs())
+ if (I) {
+unsigned next_ID = (I)->getBlockID();
+if (next_ID == ExitID && CurState == FoundPathForThrow) {
+ States[next_ID] = CurState;
+} else if (States[next_ID] < CurState) {
+ States[next_ID] = 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(SourceLocation OpLoc, Sema ,
+ const FunctionDecl *FD) {
+ if (!S.getSourceManager().isInSystemHeader(OpLoc)) {
+S.Diag(OpLoc, diag::warn_throw_in_noexcept_func) << FD;
+if (S.getLangOpts().CPlusPlus11 &&
+(isa(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
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 marked 2 inline comments as done. jyu2 added inline comments. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:334 + continue; +else + HasThrowOutFunc = true; jyu2 wrote: > aaron.ballman wrote: > > You can drop the `else` here and just set `HasThrowOutFunc` to true. > Can not do that, don't if block has throw expression yet. Yes, Eric just point out, you are right, I can remove the line of "else" https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 marked 12 inline comments as done.
jyu2 added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6341
+: Warning<"%0 has a non-throwing exception specification but can still "
+ "throw, may result in unexpected program termination.">,
+ InGroup;
aaron.ballman wrote:
> throw, may -> throw, which may
>
> Also, remove the period at the end of the diagnostic.
use you suggested.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:290
+
+static bool mayThrowBeCaughted(const CXXThrowExpr *Throw,
+ const CXXCatchStmt *Catch) {
aaron.ballman wrote:
> Caughted -> Caught
:-(
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:304
+ if (CaughtAsRecordType && ThrowTypeAsRecordType) {
+if (CaughtAsRecordType == ThrowTypeAsRecordType)
+ MayCaught = true;
aaron.ballman wrote:
> This does not seem quite correct. Consider:
> ```
> struct S{};
>
> void f() {
> try {
> throw S{};
> } catch (const S *s) {
> }
> }
> ```
Good catch. Remove that check.
https://reviews.llvm.org/D3
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 added a comment. In https://reviews.llvm.org/D3#770238, @aaron.ballman wrote: > In https://reviews.llvm.org/D3#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. https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 marked 8 inline comments as done. jyu2 added inline comments. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:334 + continue; +else + HasThrowOutFunc = true; aaron.ballman wrote: > You can drop the `else` here and just set `HasThrowOutFunc` to true. Can not do that, don't if block has throw expression yet. https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 101167.
jyu2 marked an inline comment as done.
jyu2 added a comment.
Update to address review comments.
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/AnalysisBasedWarnings.cpp
test/CXX/except/except.spec/p11.cpp
test/SemaCXX/warn-throw-out-noexcept-func.cpp
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -279,6 +279,150 @@
}
//===--===//
+// Check for throw in a non-throwing function.
+//===--===//
+enum ThrowState {
+ FoundNoPathForThrow,
+ FoundPathForThrow,
+ FoundPathWithNoThrowOutFunction,
+};
+
+static bool isThrowBeCaught(const CXXThrowExpr *Throw,
+const CXXCatchStmt *Catch) {
+ bool isCaught = false;
+ const Type *ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
+ const Type *CaughtType = Catch->getCaughtType().getTypePtrOrNull();
+
+ if (ThrowType->isReferenceType())
+ ThrowType = ThrowType->castAs()
+ ->getPointeeType()
+ ->getUnqualifiedDesugaredType();
+ if (CaughtType && CaughtType->isReferenceType())
+ CaughtType = CaughtType->castAs()
+ ->getPointeeType()
+ ->getUnqualifiedDesugaredType();
+ if (CaughtType == nullptr || CaughtType == ThrowType)
+return true;
+
+ const CXXRecordDecl *CaughtAsRecordType =
+ CaughtType->getPointeeCXXRecordDecl();
+ const CXXRecordDecl *ThrowTypeAsRecordType = ThrowType->getAsCXXRecordDecl();
+ if (CaughtAsRecordType && ThrowTypeAsRecordType)
+isCaught = ThrowTypeAsRecordType->isDerivedFrom(CaughtAsRecordType);
+ return isCaught;
+}
+
+static bool isThrowBeCaughtByHandlers(const CXXThrowExpr *CE,
+ const CXXTryStmt *TryStmt) {
+ for (unsigned H = 0; H < TryStmt->getNumHandlers(); ++H) {
+const CXXCatchStmt *CS = TryStmt->getHandler(H);
+if (isThrowBeCaught(CE, CS))
+ return true;
+ }
+ return false;
+}
+
+static bool doesThrowEscapePath(CFGBlock , SourceLocation *OpLoc) {
+ bool HasThrowOutFunc = false;
+ for (const clang::CFGElement : Block) {
+if (B.getKind() != CFGElement::Statement)
+ continue;
+const CXXThrowExpr *CE =
+dyn_cast(B.getAs()->getStmt());
+if (!CE)
+ continue;
+else
+ HasThrowOutFunc = true;
+
+*OpLoc = CE->getThrowLoc();
+for (const CFGBlock::AdjacentBlock I : Block.succs())
+ if (I && I->getTerminator() && isa(I->getTerminator())) {
+const CXXTryStmt *Terminator = cast(I->getTerminator());
+if (isThrowBeCaughtByHandlers(CE, Terminator))
+ return false;
+ }
+ }
+ return HasThrowOutFunc;
+}
+
+static bool hasThrowOutNonThrowingFunc(const FunctionDecl *FD,
+ SourceLocation *OpLoc, CFG *cfg) {
+
+ const unsigned ExitID = cfg->getExit().getBlockID();
+
+ SmallVector States(cfg->getNumBlockIDs(),
+ FoundNoPathForThrow);
+ States[cfg->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;
+
+ SmallVector Stack;
+ Stack.push_back(>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 CFGBlock::AdjacentBlock I : CurBlock->succs())
+ if (I) {
+unsigned next_ID = (I)->getBlockID();
+if (next_ID == ExitID && CurState == FoundPathForThrow) {
+ States[next_ID] = CurState;
+} else if (States[next_ID] < CurState) {
+ States[next_ID] = 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(SourceLocation OpLoc, Sema ,
+ const FunctionDecl *FD) {
+ if (!S.getSourceManager().isInSystemHeader(OpLoc)) {
+S.Diag(OpLoc, diag::warn_throw_in_noexcept_func) << FD;
+if (S.getLangOpts().CPlusPlus11 &&
+(isa(FD) ||
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Delete ||
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete))
+ S.Diag(FD->getLocation(), diag::note_throw_in_dtor);
+else
+
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
aaron.ballman added a comment.
In https://reviews.llvm.org/D3#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.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6341
+: Warning<"%0 has a non-throwing exception specification but can still "
+ "throw, may result in unexpected program termination.">,
+ InGroup;
throw, may -> throw, which may
Also, remove the period at the end of the diagnostic.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6344
+def note_throw_in_dtor
+: Note<"destructor or deallocator has a (possible implicit) non-throwing "
+ "excepton specification">;
possible -> possibly
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6347
+def note_throw_in_function
+: Note<"nonthrowing function declare here">;
def err_seh_try_outside_functions : Error<
nonthrowing -> non-throwing
(to be consistent with other diagnostics.)
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:290
+
+static bool mayThrowBeCaughted(const CXXThrowExpr *Throw,
+ const CXXCatchStmt *Catch) {
Caughted -> Caught
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:292
+ const CXXCatchStmt *Catch) {
+ bool MayCaught = false;
+ const auto *ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
MayCaught -> IsCaught ?
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:293-294
+ bool MayCaught = false;
+ const auto *ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
+ const auto *CaughtType = Catch->getCaughtType().getTypePtrOrNull();
+
Please don't use `auto` unless the type is explicitly spelled out in the
initializer (here and elsewhere).
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:298
+return true;
+ if (CaughtType == ThrowType)
+return true;
You can combine this with the above check.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:304
+ if (CaughtAsRecordType && ThrowTypeAsRecordType) {
+if (CaughtAsRecordType == ThrowTypeAsRecordType)
+ MayCaught = true;
This does not seem quite correct. Consider:
```
struct S{};
void f() {
try {
throw S{};
} catch (const S *s) {
}
}
```
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:311
+
+static bool mayThrowBeCaughtedByHandlers(const CXXThrowExpr *CE,
+ const CXXTryStmt *TryStmt) {
mayThrowBeCaughtedByHandlers -> isThrowCaughtByAHandler
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:314
+ bool Caught = false;
+ for (unsigned h = 0; h < TryStmt->getNumHandlers(); ++h) {
+const CXXCatchStmt *CS = TryStmt->getHandler(h);
h -> H (or Idx, or anything else that meets the coding standard).
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:317-318
+if (mayThrowBeCaughted(CE, CS)) {
+ Caught = true;
+ break;
+}
Just return true here instead of setting a local variable. Return false below.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:324
+
+static bool hasThrowOutNothrowingFuncInPath(CFGBlock ,
+SourceLocation *OpLoc) {
Perhaps it's more clear as: `doesThrowEscapePath()`?
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:330
+ continue;
+const CXXThrowExpr *CE =
+dyn_cast(B.getAs()->getStmt());
Can use `const auto *` here.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:334
+ continue;
+else
+ HasThrowOutFunc = true;
You can drop the `else` here and just set `HasThrowOutFunc` to true.
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:337
+
+(*OpLoc) = CE->getThrowLoc();
+for (CFGBlock::const_succ_iterator I = Block.succ_begin(),
If OpLoc cannot be null, you should take it by reference. If it can be null,
then you should guard against that
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 100796.
jyu2 added a comment.
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...
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/AnalysisBasedWarnings.cpp
test/CXX/except/except.spec/p11.cpp
test/SemaCXX/warn-throw-out-noexcept-func.cpp
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -279,6 +279,158 @@
}
//===--===//
+// Check for throw in a non-throwing function.
+//===--===//
+enum ThrowState {
+ FoundNoPathForThrow,
+ FoundPathForThrow,
+ FoundPathWithNoThrowOutFunction,
+};
+
+static bool mayThrowBeCaughted(const CXXThrowExpr *Throw,
+ const CXXCatchStmt *Catch) {
+ bool MayCaught = false;
+ const auto *ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();
+ const auto *CaughtType = Catch->getCaughtType().getTypePtrOrNull();
+
+ if (CaughtType == nullptr)
+return true;
+ if (CaughtType == ThrowType)
+return true;
+
+ const auto *CaughtAsRecordType = CaughtType->getPointeeCXXRecordDecl();
+ const auto *ThrowTypeAsRecordType = ThrowType->getAsCXXRecordDecl();
+ if (CaughtAsRecordType && ThrowTypeAsRecordType) {
+if (CaughtAsRecordType == ThrowTypeAsRecordType)
+ MayCaught = true;
+MayCaught = ThrowTypeAsRecordType->isDerivedFrom(CaughtAsRecordType);
+ }
+ return MayCaught;
+}
+
+static bool mayThrowBeCaughtedByHandlers(const CXXThrowExpr *CE,
+ const CXXTryStmt *TryStmt) {
+ bool Caught = false;
+ for (unsigned h = 0; h < TryStmt->getNumHandlers(); ++h) {
+const CXXCatchStmt *CS = TryStmt->getHandler(h);
+if (mayThrowBeCaughted(CE, CS)) {
+ Caught = true;
+ break;
+}
+ }
+ return Caught;
+}
+
+static bool hasThrowOutNothrowingFuncInPath(CFGBlock ,
+SourceLocation *OpLoc) {
+ bool HasThrowOutFunc = false;
+ for (const auto : Block) {
+if (B.getKind() != CFGElement::Statement)
+ continue;
+const CXXThrowExpr *CE =
+dyn_cast(B.getAs()->getStmt());
+if (!CE)
+ continue;
+else
+ HasThrowOutFunc = true;
+
+(*OpLoc) = CE->getThrowLoc();
+for (CFGBlock::const_succ_iterator I = Block.succ_begin(),
+ E = Block.succ_end();
+ I != E; ++I) {
+ if (*I && (*I)->getTerminator() &&
+ isa((*I)->getTerminator())) {
+const CXXTryStmt *Terminator = cast((*I)->getTerminator());
+if (mayThrowBeCaughtedByHandlers(CE, Terminator)) {
+ HasThrowOutFunc = false;
+ break;
+}
+ }
+}
+ }
+ return HasThrowOutFunc;
+}
+
+static bool hasThrowOutNonThrowingFunc(const FunctionDecl *FD,
+ SourceLocation *OpLoc, CFG *cfg) {
+
+ const unsigned ExitID = cfg->getExit().getBlockID();
+
+ SmallVector States(cfg->getNumBlockIDs(),
+ FoundNoPathForThrow);
+ States[cfg->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;
+
+ SmallVector Stack;
+ Stack.push_back(>getEntry());
+ while (!Stack.empty()) {
+CFGBlock *CurBlock = Stack.back();
+Stack.pop_back();
+
+unsigned ID = CurBlock->getBlockID();
+ThrowState CurState = States[ID];
+if (CurState == FoundPathWithNoThrowOutFunction) {
+ // Found a path to the exit node without a throw exression.
+ if (ExitID == ID)
+continue;
+
+ if (hasThrowOutNothrowingFuncInPath(*CurBlock, OpLoc)) {
+CurState = FoundPathForThrow;
+ }
+}
+
+// Loop over successor blocks and add them to the Stack if their state
+// changes.
+
+for (auto I = CurBlock->succ_begin(), E = CurBlock->succ_end(); I != E; ++I)
+ if (*I) {
+unsigned next_ID = (*I)->getBlockID();
+if (next_ID == ExitID && CurState == FoundPathForThrow) {
+ States[next_ID] = CurState;
+} else if (States[next_ID] < CurState) {
+ States[next_ID] = 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(SourceLocation OpLoc, Sema ,
+ const FunctionDecl
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 added a comment. In https://reviews.llvm.org/D3#760431, @aaron.ballman wrote: > In https://reviews.llvm.org/D3#760419, @jyu2 wrote: > > > In https://reviews.llvm.org/D3#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. https://reviews.llvm.org/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 https://reviews.llvm.org/D3#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? https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. In https://reviews.llvm.org/D3#760419, @jyu2 wrote: > In https://reviews.llvm.org/D3#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. https://reviews.llvm.org/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 https://reviews.llvm.org/D3#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. https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 added a comment. In https://reviews.llvm.org/D3#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 https://reviews.llvm.org/D3#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. https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
Prazek added a comment. 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) https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
aaron.ballman added a comment. 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. https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 99517.
jyu2 added a comment.
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
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/TreeTransform.h
test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.spec.concept/p1.cpp
test/CXX/except/except.spec/p11.cpp
test/SemaCXX/warn-throw-out-dtor.cpp
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -687,6 +687,48 @@
return BuildCXXThrow(OpLoc, Ex, IsThrownVarInScope);
}
+static bool isNoexcept(const FunctionDecl * FD)
+{
+ if (const FunctionProtoType *FPT = FD->getType()->castAs())
+if (FPT->getExceptionSpecType() != EST_None &&
+FPT->getNoexceptSpec(FD->getASTContext()) ==
+ FunctionProtoType::NR_Nothrow)
+ return true;
+ return false;
+}
+
+static bool isNoexceptTrue(const FunctionDecl * FD)
+{
+ // Avoid emitting error twice.
+ if (const FunctionDecl * TempFD = FD->getTemplateInstantiationPattern())
+if (isNoexcept(TempFD))
+ return false;
+ return isNoexcept(FD);
+}
+
+
+void Sema::CheckCXXThrowInNonThrowingFunc(SourceLocation OpLoc) {
+ bool isInCXXTryBlock = false;
+ for (auto *S = getCurScope(); S; S = S->getParent())
+if (S->getFlags() & (Scope::TryScope)) {
+ isInCXXTryBlock = true;
+ break;
+} else if (S->getFlags() & (Scope::FnScope))
+ break;
+ if (const FunctionDecl *FD = getCurFunctionDecl())
+if (!isInCXXTryBlock && !getSourceManager().isInSystemHeader(OpLoc))
+ if (isNoexceptTrue(FD)) {
+Diag(OpLoc, diag::warn_throw_in_noexcept_func) << FD;
+if (getLangOpts().CPlusPlus11 &&
+(isa(FD) ||
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Delete ||
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete))
+ Diag(FD->getLocation(), diag::note_throw_in_dtor);
+else
+ Diag(FD->getLocation(), diag::note_throw_in_function);
+ }
+}
+
ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex,
bool IsThrownVarInScope) {
// Don't report an error if 'throw' is used in system headers.
@@ -702,6 +744,8 @@
if (getCurScope() && getCurScope()->isOpenMPSimdDirectiveScope())
Diag(OpLoc, diag::err_omp_simd_region_cannot_use_stmt) << "throw";
+ CheckCXXThrowInNonThrowingFunc(OpLoc);
+
if (Ex && !Ex->isTypeDependent()) {
QualType ExceptionObjectTy = Context.getExceptionObjectType(Ex->getType());
if (CheckCXXThrowOperand(OpLoc, ExceptionObjectTy, Ex))
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -9873,9 +9873,10 @@
if (SubExpr.isInvalid())
return ExprError();
- if (!getDerived().AlwaysRebuild() &&
- SubExpr.get() == E->getSubExpr())
+ if (!getDerived().AlwaysRebuild() && SubExpr.get() == E->getSubExpr()) {
+getSema().CheckCXXThrowInNonThrowingFunc(E->getThrowLoc());
return E;
+ }
return getDerived().RebuildCXXThrowExpr(E->getThrowLoc(), SubExpr.get(),
E->isThrownVariableInScope());
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -4967,6 +4967,9 @@
ExprResult BuildCXXThrow(SourceLocation OpLoc, Expr *Ex,
bool IsThrownVarInScope);
bool CheckCXXThrowOperand(SourceLocation ThrowLoc, QualType ThrowTy, Expr *E);
+ /// Check if throw is used in function with non-throwing noexcept
+ /// specifier.
+ void CheckCXXThrowInNonThrowingFunc(SourceLocation OpLoc);
/// ActOnCXXTypeConstructExpr - Parse construction of a specified type.
/// Can be interpreted either as function-style casting ("int(x)")
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -6331,6 +6331,16 @@
"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' function assumed not to throw an exception but does. "
+ "Throwing exception may cause termination.">,
+ InGroup;
+def note_throw_in_dtor
+: Note<"destructor or
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
lebedev.ri added a comment. Relevant https://reviews.llvm.org/D31370, https://reviews.llvm.org/D19201 https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
rnk added a comment. 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. https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 created this revision.
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::~dependent_warn' requested here
dependent_warn f; // cause warning
Let me know if more information is needed.
Thanks.
Jennifer
https://reviews.llvm.org/D3
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/TreeTransform.h
test/SemaCXX/warn-throw-out-dtor.cpp
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -687,6 +687,37 @@
return BuildCXXThrow(OpLoc, Ex, IsThrownVarInScope);
}
+static bool isNoexcept(const FunctionDecl * FD)
+{
+ if (const FunctionProtoType *FPT = FD->getType()->castAs())
+if (isNoexceptExceptionSpec(FPT->getExceptionSpecType()) &&
+FPT->getNoexceptSpec(FD->getASTContext()) ==
+FunctionProtoType::NR_Nothrow)
+ return true;
+ return false;
+}
+
+static bool isNoexceptTrue(const FunctionDecl * FD)
+{
+ // Avoid emitting error twice.
+ if (const FunctionDecl * TempFD = FD->getTemplateInstantiationPattern())
+if (isNoexcept(TempFD))
+ return false;
+ return isNoexcept(FD);
+}
+
+void Sema::CheckCXXThrowInNonThrowingFunc(SourceLocation OpLoc) {
+
+ if (const FunctionDecl *FD = getCurFunctionDecl())
+if (getLangOpts().CPlusPlus11 &&
+!getSourceManager().isInSystemHeader(OpLoc) &&
+(isa(FD) ||
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Delete ||
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete))
+ if (isNoexceptTrue(FD))
+Diag(OpLoc, diag::warn_throw_in_dtor) << FD;
+}
+
ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex,
bool IsThrownVarInScope) {
// Don't report an error if 'throw' is used in system headers.
@@ -702,6 +733,8 @@
if (getCurScope() && getCurScope()->isOpenMPSimdDirectiveScope())
Diag(OpLoc, diag::err_omp_simd_region_cannot_use_stmt) << "throw";
+ CheckCXXThrowInNonThrowingFunc(OpLoc);
+
if (Ex && !Ex->isTypeDependent()) {
QualType ExceptionObjectTy = Context.getExceptionObjectType(Ex->getType());
if (CheckCXXThrowOperand(OpLoc, ExceptionObjectTy, Ex))
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -9873,9 +9873,10 @@
if (SubExpr.isInvalid())
return ExprError();
- if (!getDerived().AlwaysRebuild() &&
- SubExpr.get() == E->getSubExpr())
+ if (!getDerived().AlwaysRebuild() && SubExpr.get() == E->getSubExpr()) {
+getSema().CheckCXXThrowInNonThrowingFunc(E->getThrowLoc());
return E;
+ }
return getDerived().RebuildCXXThrowExpr(E->getThrowLoc(), SubExpr.get(),
E->isThrownVariableInScope());
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -4967,6 +4967,9 @@
ExprResult BuildCXXThrow(SourceLocation OpLoc, Expr *Ex,
bool IsThrownVarInScope);
bool CheckCXXThrowOperand(SourceLocation ThrowLoc, QualType ThrowTy, Expr *E);
+ /// Check if throw is used in function with non-throwing noexcept
+ /// specifier.
+ void CheckCXXThrowInNonThrowingFunc(SourceLocation OpLoc);
/// ActOnCXXTypeConstructExpr - Parse construction of a specified type.
/// Can be interpreted either as function-style
