[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