<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 13/01/15 23:16, Richard Smith wrote:<br>
</div>
<blockquote
cite="mid:CAOfiQqnm2AByZWxk94r_0WVqZY07v7A9+QyC9+o1_ig-Eu=YaA@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">On Thu, Nov 27, 2014 at 2:50 PM,
Vassil Vassilev <span dir="ltr"><<a
moz-do-not-send="true" href="mailto:vvasilev@cern.ch"
target="_blank">vvasilev@cern.ch</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span class="">
<div>On 25/11/14 00:52, Richard Smith wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><span>+ } + else if
(!isUnresolvedExceptionSpec(FPT->getExceptionSpecType())
&&</span>
<div><span><br>
</span></div>
Per the LLVM coding style, put the close brace and
the 'else' on the same line.
<div><br>
<div><span>+
isUnresolvedExceptionSpec(PrevFPT->getExceptionSpecType()))
{ + // Happens in cases where module A
contains only a fwd decl and module B + //
contains the definition.</span></div>
<div><span><br>
</span></div>
There are other conditions here; I don't think
this comment is particularly helpful. Just
remove it?</div>
<div><br>
<div><span>+ FunctionProtoType::ExtProtoInfo EPI
= FPT->getExtProtoInfo(); + while
(PrevFD) { +
Reader.Context.adjustExceptionSpec(PrevFD,
EPI.ExceptionSpec); + PrevFD =
PrevFD->getPreviousDecl();</span></div>
<div><span><br>
</span></div>
Please use a separate variable for this loop
rather than reusing PrevFD. It's only by
coincidence that this code is at the end of the
function; it shouldn't be changing
function-scope state like this.</div>
</div>
</blockquote>
</span> All good points, thanks!<span class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>It seems like it should be possible to
produce a testcase for this. You'd need
something like:</div>
<div><br>
</div>
<div>A.h:</div>
<div><br>
</div>
<div>struct A { A(); } b{}, c(b); // computed
exception spec for copy ctor and move ctor<br>
</div>
<div><br>
</div>
<div>B.h:</div>
<div><br>
</div>
<div>
<div>struct A { A(); } a{}; // EST_Unevaluated
for copy ctor and move ctor</div>
<div><br>
</div>
</div>
<div>... then import A and B, and do something
that assumes that every declaration has an
exception specification if any declaration does.</div>
</div>
</blockquote>
</span> Thanks for the pointer. I managed to reproduce
the behavior, i.e unevaluated exception spec as a
first/canonical redecl (see the new attached patch).
However this test doesn't trigger the original issue
(and thus not testing anything :( ) <a
moz-do-not-send="true"
href="http://llvm.org/bugs/show_bug.cgi?id=21687"
target="_blank">http://llvm.org/bugs/show_bug.cgi?id=21687</a><br>
There the setup is more difficult, because I need to
generate a unevaluated exception spec dtor as a first
redecl and go through
clang::FunctionProtoType::isNothrow to get the
assertion. To make things worse, this base dtor must be
emitted as an alias. Could you help me out?<br>
</div>
</blockquote>
<div><br>
</div>
<div>Ugh, getting good modules testcases is hard. I'm not
sure I follow what you said above, though: does the
testcase in your patch fail without the code change and
pass with it? If so, I think that's enough. (Or does it
only fail with a previous revision of the patch and pass
with both trunk and the patched code?)</div>
</div>
</div>
</div>
</blockquote>
Sorry for the delay. The attached test contains all preconditions
caused the crash in bugid=21687. The problem is that I am missing a
statement, which forces clang to call
clang::FunctionProtoType::isNothrow and die with an assert. One way
to do it is via: clang::CodeGen::CodeGenModule::EmitGlobalDefinition
-> clang::CodeGen::CodeGenModule::codegenCXXStructor->
clang::CodeGen::CodeGenModule::getAddrOfCXXStructor ->
clang::CodeGen::CodeGenModule::GetOrCreateLLVMFunction ->
clang::CodeGen::CodeGenModule::SetFunctionAttributes ->
clang::CodeGen::CodeGenModule::SetLLVMFunctionAttributes ->
clang::CodeGen::CodeGenModule::ConstructAttributeList call chain.<br>
<br>
However I don't know how to make this happen for the dtors in the
test. I'd be happy to discuss that in IRC if you need more feedback.<br>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<blockquote
cite="mid:CAOfiQqnm2AByZWxk94r_0WVqZY07v7A9+QyC9+o1_ig-Eu=YaA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div><span style="color:rgb(0,0,0);font-family:'Courier
New',Courier,monospace;font-size:14px;white-space:pre-wrap">+
FunctionDecl* PrevFDToUpdate = PrevFD;
+ while (PrevFDToUpdate) {
+ Reader.Context.adjustExceptionSpec(PrevFDToUpdate,
EPI.ExceptionSpec);
+ PrevFDToUpdate = PrevFDToUpdate->getPreviousDecl();
</span></div>
<div><span style="color:rgb(0,0,0);font-family:'Courier
New',Courier,monospace;font-size:14px;white-space:pre-wrap"><br>
</span></div>
<div>The "*" should go on the right, and this would (to me)
read better as a for loop:<br>
</div>
<div><br>
</div>
<div>for (FunctionDecl *PrevFDToUpdate = PrevFD;
PrevFDToUpdate;</div>
<div> PrevFDToUpdate =
PrevFDToUpdate->getPreviousDecl())</div>
<div> Reader.Context.adjustExceptionSpec(PrevFDToUpdate,
EPI.ExceptionSpec);</div>
</div>
</div>
</div>
</blockquote>
I agree it looks much nicer.<br>
Vassil<br>
<blockquote
cite="mid:CAOfiQqnm2AByZWxk94r_0WVqZY07v7A9+QyC9+o1_ig-Eu=YaA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>Other than that, LGTM.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
<div bgcolor="#FFFFFF" text="#000000"> Vassil<span
class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div>
<div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, Nov 24,
2014 at 11:09 AM, Vassil Vassilev <span
dir="ltr"><<a moz-do-not-send="true"
href="mailto:vvasilev@cern.ch"
target="_blank">vvasilev@cern.ch</a>></span>
wrote:<br>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>Sorry for the delay. Attaching
the new version.<span><font
color="#888888"><br>
Vassil</font></span>
<div>
<div><br>
On 14/11/14 02:47, Richard Smith
wrote:<br>
</div>
</div>
</div>
<div>
<div>
<blockquote type="cite">
<div dir="ltr">
<div>+ }</div>
<div>+ else { //
FPT->getExceptionSpecType()
is resolved and the other is
not</div>
<div><br>
</div>
<div>You're not checking for
this condition; the code
here is getting run even if
both or neither are
unresolved.</div>
<div><br>
</div>
<div>The patch needs rebasing
(we have a new helper
function in ASTContext to
update the exception
specification of a
declaration), but looks like
the right direction.</div>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On
Thu, Nov 6, 2014 at 4:23 AM,
Vassil Vassilev <span
dir="ltr"><<a
moz-do-not-send="true"
href="mailto:vasil.georgiev.vasilev@cern.ch"
target="_blank">vasil.georgiev.vasilev@cern.ch</a>></span>
wrote:<br>
<blockquote
class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi
Richard,<br>
I am attaching the patch
we discussed at the dev
meeting. Still haven't
found small reproducer...<br>
The issue appears to
stem from the fact that
module A contains only a
forward declaration of a
function and it exception
spec cannot be computed.
In module B is the
definition with computed
exception spec, which gets
deserialized after the one
in module A. This patch
teaches the ASTDeclReader
to update all the
exception specs of the
previous decls to the
current one.<br>
<br>
Could you review,
please?<br>
Many thanks,<br>
Vassil</blockquote>
</div>
</div>
</blockquote>
<br>
</div>
</div>
</div>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a moz-do-not-send="true"
href="mailto:cfe-commits@cs.uiuc.edu"
target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a moz-do-not-send="true"
href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits"
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</div>
</blockquote>
</span><br>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>