[modules][PATCH] Teach the ASTDeclReader to update the exception specs of the deserialized decl's redecl chain.

[Vassil Vassilev]

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?
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
>
>


-- 
--------------------------------------------
Q: Why is this email five sentences or less?
A: http://five.sentenc.es

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141127/91d8e6e9/attachment.html>
-------------- next part --------------
Index: lib/Serialization/ASTReaderDecl.cpp
===================================================================
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2779,11 +2779,20 @@ 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();
+      FunctionDecl* PrevFDToUpdate = PrevFD;
+      while (PrevFDToUpdate) {
+        Reader.Context.adjustExceptionSpec(PrevFDToUpdate, EPI.ExceptionSpec);
+        PrevFDToUpdate = PrevFDToUpdate->getPreviousDecl();
+      }
+    }
   }
 }
 }
Index: test/Modules/Inputs/PR21687/A.h
new file mode 100644
===================================================================
--- /dev/null
+++ 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
Index: test/Modules/Inputs/PR21687/B.h
new file mode 100644
===================================================================
--- /dev/null
+++ test/Modules/Inputs/PR21687/B.h
@@ -0,0 +1,4 @@
+struct A {
+  A(){}
+  virtual ~A() {}
+} a{};  // EST_Unevaluated for copy ctor and move ctor
Index: test/Modules/Inputs/PR21687/module.modulemap
new file mode 100644
===================================================================
--- /dev/null
+++ test/Modules/Inputs/PR21687/module.modulemap
@@ -0,0 +1,10 @@
+module modA {
+  header "A.h"
+  export *
+}
+
+module modB {
+  header "B.h"
+  export *
+}
+
Index: test/Modules/pr21687.cpp
new file mode 100644
===================================================================
--- /dev/null
+++ 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;
+}