<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Sorry Richard, I thought I gave you enough time before committing this for Jen.
<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Adding her so she can respond to your comments and fix these.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Erich
<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></a></p>
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> metafoo@gmail.com [mailto:metafoo@gmail.com]
<b>On Behalf Of </b>Richard Smith<br>
<b>Sent:</b> Monday, June 26, 2017 2:25 PM<br>
<b>To:</b> Keane, Erich <erich.keane@intel.com><br>
<b>Cc:</b> cfe-commits <cfe-commits@lists.llvm.org><br>
<b>Subject:</b> Re: r306149 - Emit warning when throw exception in destruct or dealloc functions which has a<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<div>
<p class="MsoNormal">On 23 June 2017 at 13:22, Erich Keane via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">Author: erichkeane<br>
Date: Fri Jun 23 15:22:19 2017<br>
New Revision: 306149<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=306149&view=rev" target="_blank">
http://llvm.org/viewvc/llvm-project?rev=306149&view=rev</a><br>
Log:<br>
Emit warning when throw exception in destruct or dealloc functions which has a<br>
(possible implicit) noexcept specifier<br>
<br>
Throwing in the destructor is not good (C++11 change try to not allow see below).<br>
But in reality, those codes are exist.<br>
C++11 [class.dtor]p3:<br>
<br>
A declaration of a destructor that does not have an exception-specification is<br>
implicitly considered to have the same exception specification as an implicit<br>
declaration.<br>
<br>
With this change, the application worked before may now run into runtime<br>
termination. My goal here is to emit a warning to provide only possible info to<br>
where the code may need to be changed.<br>
<br>
First there is no way, in compile time to identify the “throw” really throw out<br>
of the function. Things like the call which throw out… To keep this simple,<br>
when “throw” is seen, checking its enclosing function(only destructor and<br>
dealloc functions) with noexcept(true) specifier emit warning.<br>
<br>
Here is implementation detail:<br>
A new member function CheckCXXThrowInNonThrowingFunc is added for class Sema<br>
in Sema.h. It is used in the call to both BuildCXXThrow and<br>
TransformCXXThrowExpr.<br>
<br>
The function basic check if the enclosing function with non-throwing noexcept<br>
specifer, if so emit warning for it.<br>
<br>
The example of warning message like:<br>
k1.cpp:18:3: warning: ''~dependent_warn'' has a (possible implicit) non-throwing<br>
<br>
noexcept specifier. Throwing exception may cause termination.<br>
[-Wthrow-in-dtor]<br>
throw 1;<br>
^<br>
<br>
k1.cpp:43:30: note: in instantiation of member function<br>
<br>
'dependent_warn<noexcept_fun>::~dependent_warn' requested here<br>
<br>
dependent_warn<noexcept_fun> f; // cause warning<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D33333" target="_blank">
https://reviews.llvm.org/D33333</a><br>
<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp<br>
cfe/trunk/test/CXX/except/except.spec/p11.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=306149&r1=306148&r2=306149&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=306149&r1=306148&r2=306149&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Jun 23 15:22:19 2017<br>
@@ -6351,6 +6351,15 @@ def err_exceptions_disabled : Error<<br>
"cannot use '%0' with exceptions disabled">;<br>
def err_objc_exceptions_disabled : Error<<br>
"cannot use '%0' with Objective-C exceptions disabled">;<br>
+def warn_throw_in_noexcept_func<br>
+ : Warning<"%0 has a non-throwing exception specification but can still "<br>
+ "throw, resulting in unexpected program termination">,<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">+ InGroup<Exceptions>;<br>
+def note_throw_in_dtor<br>
+ : Note<"destructor or deallocator has a (possibly implicit) non-throwing "<br>
+ "excepton specification">;<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Also, typo "excepton".<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">+def note_throw_in_function<br>
+ : Note<"non-throwing function declare here">;<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">declare -> declared, but something like "function declared non-throwing here" would be preferable<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal"> def err_seh_try_outside_functions : Error<<br>
"cannot use SEH '__try' in blocks, captured regions, or Obj-C method decls">;<br>
def err_mixing_cxx_try_seh_try : Error<<br>
<br>
Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=306149&r1=306148&r2=306149&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=306149&r1=306148&r2=306149&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)<br>
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Fri Jun 23 15:22:19 2017<br>
@@ -279,6 +279,150 @@ static void checkRecursiveFunction(Sema<br>
}<br>
<br>
//===----------------------------------------------------------------------===//<br>
+// Check for throw in a non-throwing function.<br>
+//===----------------------------------------------------------------------===//<br>
+enum ThrowState {<br>
+ FoundNoPathForThrow,<br>
+ FoundPathForThrow,<br>
+ FoundPathWithNoThrowOutFunction,<br>
+};<br>
+<br>
+static bool isThrowCaught(const CXXThrowExpr *Throw,<br>
+ const CXXCatchStmt *Catch) {<br>
+ const Type *ThrowType = nullptr;<br>
+ if (Throw->getSubExpr())<br>
+ ThrowType = Throw->getSubExpr()->getType().getTypePtrOrNull();<br>
+ if (!ThrowType)<br>
+ return false;<br>
+ const Type *CaughtType = Catch->getCaughtType().getTypePtrOrNull();<br>
+ if (!CaughtType)<br>
+ return true;<br>
+ if (ThrowType->isReferenceType())<br>
+ ThrowType = ThrowType->castAs<ReferenceType>()<br>
+ ->getPointeeType()<br>
+ ->getUnqualifiedDesugaredType();<br>
+ if (CaughtType->isReferenceType())<br>
+ CaughtType = CaughtType->castAs<ReferenceType>()<br>
+ ->getPointeeType()<br>
+ ->getUnqualifiedDesugaredType();<br>
+ if (CaughtType == ThrowType)<br>
+ return true;<br>
+ const CXXRecordDecl *CaughtAsRecordType =<br>
+ CaughtType->getPointeeCXXRecordDecl();<br>
+ const CXXRecordDecl *ThrowTypeAsRecordType = ThrowType->getAsCXXRecordDecl();<br>
+ if (CaughtAsRecordType && ThrowTypeAsRecordType)<br>
+ return ThrowTypeAsRecordType->isDerivedFrom(CaughtAsRecordType);<br>
+ return false;<br>
+}<br>
+<br>
+static bool isThrowCaughtByHandlers(const CXXThrowExpr *CE,<br>
+ const CXXTryStmt *TryStmt) {<br>
+ for (unsigned H = 0, E = TryStmt->getNumHandlers(); H < E; ++H) {<br>
+ if (isThrowCaught(CE, TryStmt->getHandler(H)))<br>
+ return true;<br>
+ }<br>
+ return false;<br>
+}<br>
+<br>
+static bool doesThrowEscapePath(CFGBlock Block, SourceLocation &OpLoc) {<br>
+ for (const auto &B : Block) {<br>
+ if (B.getKind() != CFGElement::Statement)<br>
+ continue;<br>
+ const auto *CE = dyn_cast<CXXThrowExpr>(B.getAs<CFGStmt>()->getStmt());<br>
+ if (!CE)<br>
+ continue;<br>
+<br>
+ OpLoc = CE->getThrowLoc();<br>
+ for (const auto &I : Block.succs()) {<br>
+ if (!I.isReachable())<br>
+ continue;<br>
+ if (const auto *Terminator =<br>
+ dyn_cast_or_null<CXXTryStmt>(I->getTerminator()))<br>
+ if (isThrowCaughtByHandlers(CE, Terminator))<br>
+ return false;<br>
+ }<br>
+ return true;<br>
+ }<br>
+ return false;<br>
+}<br>
+<br>
+static bool hasThrowOutNonThrowingFunc(SourceLocation &OpLoc, CFG *BodyCFG) {<br>
+<br>
+ unsigned ExitID = BodyCFG->getExit().getBlockID();<br>
+<br>
+ SmallVector<ThrowState, 16> States(BodyCFG->getNumBlockIDs(),<br>
+ FoundNoPathForThrow);<br>
+ States[BodyCFG->getEntry().getBlockID()] = FoundPathWithNoThrowOutFunction;<br>
+<br>
+ SmallVector<CFGBlock *, 16> Stack;<br>
+ Stack.push_back(&BodyCFG->getEntry());<br>
+ while (!Stack.empty()) {<br>
+ CFGBlock *CurBlock = Stack.back();<br>
+ Stack.pop_back();<br>
+<br>
+ unsigned ID = CurBlock->getBlockID();<br>
+ ThrowState CurState = States[ID];<br>
+ if (CurState == FoundPathWithNoThrowOutFunction) {<br>
+ if (ExitID == ID)<br>
+ continue;<br>
+<br>
+ if (doesThrowEscapePath(*CurBlock, OpLoc))<br>
+ CurState = FoundPathForThrow;<br>
+ }<br>
+<br>
+ // Loop over successor blocks and add them to the Stack if their state<br>
+ // changes.<br>
+ for (const auto &I : CurBlock->succs())<br>
+ if (I.isReachable()) {<br>
+ unsigned NextID = I->getBlockID();<br>
+ if (NextID == ExitID && CurState == FoundPathForThrow) {<br>
+ States[NextID] = CurState;<br>
+ } else if (States[NextID] < CurState) {<br>
+ States[NextID] = CurState;<br>
+ Stack.push_back(I);<br>
+ }<br>
+ }<br>
+ }<br>
+ // Return true if the exit node is reachable, and only reachable through<br>
+ // a throw expression.<br>
+ return States[ExitID] == FoundPathForThrow;<br>
+}<br>
+<br>
+static void EmitDiagForCXXThrowInNonThrowingFunc(Sema &S, SourceLocation OpLoc,<br>
+ const FunctionDecl *FD) {<br>
+ if (!S.getSourceManager().isInSystemHeader(OpLoc)) {<br>
+ S.Diag(OpLoc, diag::warn_throw_in_noexcept_func) << FD;<br>
+ if (S.getLangOpts().CPlusPlus11 &&<br>
+ (isa<CXXDestructorDecl>(FD) ||<br>
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Delete ||<br>
+ FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete))<br>
+ S.Diag(FD->getLocation(), diag::note_throw_in_dtor);<br>
+ else<br>
+ S.Diag(FD->getLocation(), diag::note_throw_in_function);<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Please point this diagnostic at the exception specification when its location can be computed (use FD->getExceptionSpecSourceRange()).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">+ }<br>
+}<br>
+<br>
+static void checkThrowInNonThrowingFunc(Sema &S, const FunctionDecl *FD,<br>
+ AnalysisDeclContext &AC) {<br>
+ CFG *BodyCFG = AC.getCFG();<br>
+ if (!BodyCFG)<br>
+ return;<br>
+ if (BodyCFG->getExit().pred_empty())<br>
+ return;<br>
+ SourceLocation OpLoc;<br>
+ if (hasThrowOutNonThrowingFunc(OpLoc, BodyCFG))<br>
+ EmitDiagForCXXThrowInNonThrowingFunc(S, OpLoc, FD);<br>
+}<br>
+<br>
+static bool isNoexcept(const FunctionDecl *FD) {<br>
+ const auto *FPT = FD->getType()->castAs<FunctionProtoType>();<br>
+ if (FPT->getExceptionSpecType() != EST_None &&<br>
+ FPT->isNothrow(FD->getASTContext()))<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Why the EST_None special case here? The isNothrow check would handle that just fine.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">+ return true;<br>
+ return false;<br>
+}<br>
+<br>
+//===----------------------------------------------------------------------===//<br>
// Check for missing return value.<br>
//===----------------------------------------------------------------------===//<br>
<br>
@@ -2127,6 +2271,12 @@ AnalysisBasedWarnings::IssueWarnings(sem<br>
}<br>
}<br>
<br>
+ // Check for throw out of non-throwing function.<br>
+ if (!Diags.isIgnored(diag::warn_throw_in_noexcept_func, D->getLocStart()))<br>
+ if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))<br>
+ if (S.getLangOpts().CPlusPlus && isNoexcept(FD))<br>
+ checkThrowInNonThrowingFunc(S, FD, AC);<br>
+<br>
// If none of the previous checks caused a CFG build, trigger one here<br>
// for -Wtautological-overlap-compare<br>
if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,<br>
<br>
Modified: cfe/trunk/test/CXX/except/except.spec/p11.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/except/except.spec/p11.cpp?rev=306149&r1=306148&r2=306149&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/except/except.spec/p11.cpp?rev=306149&r1=306148&r2=306149&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CXX/except/except.spec/p11.cpp (original)<br>
+++ cfe/trunk/test/CXX/except/except.spec/p11.cpp Fri Jun 23 15:22:19 2017<br>
@@ -1,12 +1,11 @@<br>
// RUN: %clang_cc1 -std=c++11 -fexceptions -fcxx-exceptions -fsyntax-only -verify %s<br>
-// expected-no-diagnostics<br>
<br>
// This is the "let the user shoot themselves in the foot" clause.<br>
-void f() noexcept {<br>
- throw 0; // no-error<br>
+void f() noexcept { // expected-note {{non-throwing function declare here}}<br>
+ throw 0; // expected-warning {{has a non-throwing exception specification but}}<br>
}<br>
-void g() throw() {<br>
- throw 0; // no-error<br>
+void g() throw() { // expected-note {{non-throwing function declare here}}<br>
+ throw 0; // expected-warning {{has a non-throwing exception specification but}}<br>
}<br>
void h() throw(int) {<br>
throw 0.0; // no-error<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</body>
</html>