r306149 - Emit warning when throw exception in destruct or dealloc functions which has a
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 26 14:24:47 PDT 2017
On 23 June 2017 at 13:22, Erich Keane via cfe-commits <
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
> 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/8bca866f/attachment-0001.html>
More information about the cfe-commits
mailing list