r306149 - Emit warning when throw exception in destruct or dealloc functions which has a
Keane, Erich via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 26 14:40:57 PDT 2017
Sorry Richard, I thought I gave you enough time before committing this for Jen.
Adding her so she can respond to your comments and fix these.
Thanks,
Erich
From: metafoo at gmail.com [mailto:metafoo at gmail.com] On Behalf Of Richard Smith
Sent: Monday, June 26, 2017 2:25 PM
To: Keane, Erich <erich.keane at intel.com>
Cc: cfe-commits <cfe-commits at lists.llvm.org>
Subject: Re: r306149 - Emit warning when throw exception in destruct or dealloc functions which has a
On 23 June 2017 at 13:22, Erich Keane via cfe-commits <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>> wrote:
Author: erichkeane
Date: Fri Jun 23 15:22:19 2017
New Revision: 306149
URL: http://llvm.org/viewvc/llvm-project?rev=306149&view=rev
Log:
Emit warning when throw exception in destruct or dealloc functions which has a
(possible implicit) noexcept specifier
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. My goal 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
Differential Revision: https://reviews.llvm.org/D33333
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/test/CXX/except/except.spec/p11.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=306149&r1=306148&r2=306149&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Jun 23 15:22:19 2017
@@ -6351,6 +6351,15 @@ 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">,
How do you know it's unexpected? :) You also don't know that this leads to program termination: a set_unexpected handler could do something else, in principle. I would just delete the ", resulting in unexpected program termination" part here.
+ InGroup<Exceptions>;
+def note_throw_in_dtor
+ : Note<"destructor or deallocator has a (possibly implicit) non-throwing "
+ "excepton specification">;
Please figure out which case we're actually in, and just mention that one. You can use "hasImplicitExceptionSpec" in SemaExceptionSpec.cpp to determine whether the exception specification is implicit.
Also, typo "excepton".
+def note_throw_in_function
+ : Note<"non-throwing function declare here">;
declare -> declared, but something like "function declared non-throwing here" would be preferable
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<
Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=306149&r1=306148&r2=306149&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Fri Jun 23 15:22:19 2017
@@ -279,6 +279,150 @@ static void checkRecursiveFunction(Sema
}
//===----------------------------------------------------------------------===//
+// 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);
Please point this diagnostic at the exception specification when its location can be computed (use FD->getExceptionSpecSourceRange()).
+ }
+}
+
+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()))
Why the EST_None special case here? The isNothrow check would handle that just fine.
+ return true;
+ return false;
+}
+
+//===----------------------------------------------------------------------===//
// Check for missing return value.
//===----------------------------------------------------------------------===//
@@ -2127,6 +2271,12 @@ AnalysisBasedWarnings::IssueWarnings(sem
}
}
+ // 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,
Modified: cfe/trunk/test/CXX/except/except.spec/p11.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/except/except.spec/p11.cpp?rev=306149&r1=306148&r2=306149&view=diff
==============================================================================
--- cfe/trunk/test/CXX/except/except.spec/p11.cpp (original)
+++ cfe/trunk/test/CXX/except/except.spec/p11.cpp Fri Jun 23 15:22:19 2017
@@ -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
_______________________________________________
cfe-commits mailing list
cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170626/c77a4155/attachment-0001.html>
More information about the cfe-commits
mailing list