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.
+ 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] <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
From ec324286a30dc142de21c1c3a0b5d5c3cb6d7e95 Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <[email protected]>
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)
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
