On 23 June 2017 at 13:22, Erich Keane via cfe-commits < cfe-commits@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@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
