[modules][PATCH] Teach the ASTDeclReader to update the exception specs of the deserialized decl's redecl chain.
Vassil Vassilev
vvasilev at cern.ch
Tue Mar 10 00:27:37 PDT 2015
On 10/03/15 03:03, Richard Smith wrote:
> On Mon, Mar 9, 2015 at 5:22 AM, Vassil Vassilev <vvasilev at cern.ch
> <mailto: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 <mailto: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!
Wow, great job!
Many thanks,
Vassil
>
>
>> + 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 <mailto: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
>>>> <mailto: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 <mailto: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/20150310/ed3fdd42/attachment.html>
More information about the cfe-commits
mailing list