[modules][PATCH] Teach the ASTDeclReader to update the exception specs of the deserialized decl's redecl chain.
Vassil Vassilev
vvasilev at cern.ch
Mon Mar 9 05:22:24 PDT 2015
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.
>
> + 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/20150309/bdfe16a2/attachment.html>
-------------- next part --------------
From ec324286a30dc142de21c1c3a0b5d5c3cb6d7e95 Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <vvasilev at cern.ch>
Date: Mon, 24 Nov 2014 19:58:23 +0100
Subject: [PATCH] [modules] Update exception specs of the redecl chain when
deserializing a module.
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.
Test case shows the issue but doesn't reproduce the crash of http://llvm.org/bugs/show_bug.cgi?id=21687
---
lib/Serialization/ASTReaderDecl.cpp | 15 +++++++++++----
test/Modules/Inputs/PR21687/A.h | 4 ++++
test/Modules/Inputs/PR21687/B.h | 4 ++++
test/Modules/Inputs/PR21687/module.modulemap | 10 ++++++++++
test/Modules/pr21687.cpp | 10 ++++++++++
5 files changed, 39 insertions(+), 4 deletions(-)
create mode 100644 test/Modules/Inputs/PR21687/A.h
create mode 100644 test/Modules/Inputs/PR21687/B.h
create mode 100644 test/Modules/Inputs/PR21687/module.modulemap
create mode 100644 test/Modules/pr21687.cpp
diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp
index 5aeb3d1..eae4116 100644
--- a/lib/Serialization/ASTReaderDecl.cpp
+++ b/lib/Serialization/ASTReaderDecl.cpp
@@ -2831,11 +2831,18 @@ void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader,
// specification now.
auto *FPT = FD->getType()->getAs<FunctionProtoType>();
auto *PrevFPT = PrevFD->getType()->getAs<FunctionProtoType>();
- if (FPT && PrevFPT &&
- isUnresolvedExceptionSpec(FPT->getExceptionSpecType()) &&
- !isUnresolvedExceptionSpec(PrevFPT->getExceptionSpecType())) {
- Reader.Context.adjustExceptionSpec(
+ if (FPT && PrevFPT) {
+ if (isUnresolvedExceptionSpec(FPT->getExceptionSpecType()) &&
+ !isUnresolvedExceptionSpec(PrevFPT->getExceptionSpecType())) {
+ Reader.Context.adjustExceptionSpec(
FD, PrevFPT->getExtProtoInfo().ExceptionSpec);
+ } else if (!isUnresolvedExceptionSpec(FPT->getExceptionSpecType()) &&
+ isUnresolvedExceptionSpec(PrevFPT->getExceptionSpecType())) {
+ FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
+ for (FunctionDecl *PrevFDToUpdate = PrevFD; PrevFDToUpdate;
+ PrevFDToUpdate = PrevFDToUpdate->getPreviousDecl())
+ Reader.Context.adjustExceptionSpec(PrevFDToUpdate, EPI.ExceptionSpec);
+ }
}
}
}
diff --git a/test/Modules/Inputs/PR21687/A.h b/test/Modules/Inputs/PR21687/A.h
new file mode 100644
index 0000000..86f2d1e
--- /dev/null
+++ b/test/Modules/Inputs/PR21687/A.h
@@ -0,0 +1,4 @@
+struct A {
+ A(){}
+ virtual ~A() {}
+} b{}, c(b); // computed exception spec for copy ctor and move ctor
diff --git a/test/Modules/Inputs/PR21687/B.h b/test/Modules/Inputs/PR21687/B.h
new file mode 100644
index 0000000..bded111
--- /dev/null
+++ b/test/Modules/Inputs/PR21687/B.h
@@ -0,0 +1,4 @@
+struct A {
+ A(){}
+ virtual ~A() {}
+} a{}; // EST_Unevaluated for copy ctor and move ctor
diff --git a/test/Modules/Inputs/PR21687/module.modulemap b/test/Modules/Inputs/PR21687/module.modulemap
new file mode 100644
index 0000000..5bce4f7
--- /dev/null
+++ b/test/Modules/Inputs/PR21687/module.modulemap
@@ -0,0 +1,10 @@
+module modA {
+ header "A.h"
+ export *
+}
+
+module modB {
+ header "B.h"
+ export *
+}
+
diff --git a/test/Modules/pr21687.cpp b/test/Modules/pr21687.cpp
new file mode 100644
index 0000000..a4fe69c
--- /dev/null
+++ b/test/Modules/pr21687.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -x c++ -std=c++11 %s
+
+#include "Inputs/PR21687/B.h"
+A tmp;
+#include "Inputs/PR21687/A.h"
+
+int main() {
+ A tmp1;
+}
--
1.9.3 (Apple Git-50)
More information about the cfe-commits
mailing list