On 10/03/15 03:03, Richard Smith wrote:
On Mon, Mar 9, 2015 at 5:22 AM, Vassil Vassilev <[email protected] <mailto:[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] <mailto:[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!
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. VassilOther than that, LGTM. VassilOn Mon, Nov 24, 2014 at 11:09 AM, Vassil Vassilev <[email protected] <mailto:[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] <mailto:[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] <mailto:[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
