On Mon, Mar 9, 2015 at 5:22 AM, Vassil Vassilev <[email protected]> wrote:
> On 13/01/15 23:16, Richard Smith wrote: > > On Thu, Nov 27, 2014 at 2:50 PM, Vassil Vassilev <[email protected]> > 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 <[email protected]> >> 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 < >>> [email protected]> 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 >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
