<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 10/03/15 03:03, Richard Smith wrote:<br>
</div>
<blockquote
cite="mid:CAOfiQqmYsCpppBoPiaKbAZ2j64_-4Xm3JByL0m9gRDkFfFg0nA@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 Mon, Mar 9, 2015 at 5:22 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 text="#000000" bgcolor="#FFFFFF"><span class="">
<div>On 13/01/15 23:16, Richard Smith wrote:<br>
</div>
<blockquote type="cite">
<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>
<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><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>
</span> 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.</div>
</blockquote>
<div><br>
</div>
<div>I did some fuzzing and found a testcase that triggers
the assertion; I checked in your patch with that testcase
and minor tweaks in r231738. Thanks!</div>
</div>
</div>
</div>
</blockquote>
Wow, great job!<br>
Many thanks,<br>
Vassil<br>
<blockquote
cite="mid:CAOfiQqmYsCpppBoPiaKbAZ2j64_-4Xm3JByL0m9gRDkFfFg0nA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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">
<div text="#000000" bgcolor="#FFFFFF"><span class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><span>+ FunctionDecl* PrevFDToUpdate =
PrevFD; + while (PrevFDToUpdate) { +
Reader.Context.adjustExceptionSpec(PrevFDToUpdate,
EPI.ExceptionSpec); + PrevFDToUpdate =
PrevFDToUpdate->getPreviousDecl(); </span></div>
<div><span><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>
</span> I agree it looks much nicer.<span class=""><font
color="#888888"><br>
Vassil</font></span><span class=""><br>
<blockquote 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><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>
</span></div>
</blockquote>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>