[modules][PATCH] Teach the ASTDeclReader to update the exception specs of the deserialized decl's redecl chain.

[Richard Smith]

On Mon, Mar 9, 2015 at 5:22 AM, Vassil Vassilev <vvasilev at cern.ch> wrote:

>  On 13/01/15 23:16, Richard Smith wrote:
>
>  On Thu, Nov 27, 2014 at 2:50 PM, Vassil Vassilev <vvasilev at cern.ch>
> wrote:
>
>>  On 25/11/14 00:52, Richard Smith wrote:
>>
>> + } + else if (!isUnresolvedExceptionSpec(FPT->getExceptionSpecType()) &&
>>
>>  Per the LLVM coding style, put the close brace and the 'else' on the
>> same line.
>>
>> + isUnresolvedExceptionSpec(PrevFPT->getExceptionSpecType())) { + //
>> Happens in cases where module A contains only a fwd decl and module B + //
>> contains the definition.
>>
>>  There are other conditions here; I don't think this comment is
>> particularly helpful. Just remove it?
>>
>> + FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo(); + while
>> (PrevFD) { + Reader.Context.adjustExceptionSpec(PrevFD, EPI.ExceptionSpec);
>> + PrevFD = PrevFD->getPreviousDecl();
>>
>>  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.
>>
>>  All good points, thanks!
>>
>>
>>  It seems like it should be possible to produce a testcase for this.
>> You'd need something like:
>>
>>  A.h:
>>
>>  struct A { A(); } b{}, c(b);  // computed exception spec for copy ctor
>> and move ctor
>>
>>  B.h:
>>
>>  struct A { A(); } a{};  // EST_Unevaluated for copy ctor and move ctor
>>
>>  ... then import A and B, and do something that assumes that every
>> declaration has an exception specification if any declaration does.
>>
>>  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 :( ) http://llvm.org/bugs/show_bug.cgi?id=21687
>> 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?
>>
>
>  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?)
>
> 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.
>
> 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.
>

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!


>
>   + FunctionDecl* PrevFDToUpdate = PrevFD; + while (PrevFDToUpdate) { +
> Reader.Context.adjustExceptionSpec(PrevFDToUpdate, EPI.ExceptionSpec); +
> PrevFDToUpdate = PrevFDToUpdate->getPreviousDecl();
>
>  The "*" should go on the right, and this would (to me) read better as a
> for loop:
>
>  for (FunctionDecl *PrevFDToUpdate = PrevFD; PrevFDToUpdate;
>      PrevFDToUpdate = PrevFDToUpdate->getPreviousDecl())
>   Reader.Context.adjustExceptionSpec(PrevFDToUpdate, EPI.ExceptionSpec);
>
> I agree it looks much nicer.
> Vassil
>
>
>  Other than that, LGTM.
>
>
>>
>>  Vassil
>>
>>
>> On Mon, Nov 24, 2014 at 11:09 AM, Vassil Vassilev <vvasilev at cern.ch>
>> wrote:
>>
>>>  Sorry for the delay. Attaching the new version.
>>> Vassil
>>>
>>> On 14/11/14 02:47, Richard Smith wrote:
>>>
>>>  +    }
>>> +    else { // FPT->getExceptionSpecType() is resolved and the other is
>>> not
>>>
>>>  You're not checking for this condition; the code here is getting run
>>> even if both or neither are unresolved.
>>>
>>>  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.
>>>
>>> On Thu, Nov 6, 2014 at 4:23 AM, Vassil Vassilev <
>>> vasil.georgiev.vasilev at cern.ch> wrote:
>>>
>>>> Hi Richard,
>>>>   I am attaching the patch we discussed at the dev meeting. Still
>>>> haven't found small reproducer...
>>>>   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.
>>>>
>>>>   Could you review, please?
>>>> Many thanks,
>>>> Vassil
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150309/d653f689/attachment.html>