[modules] when deserializing method, ensure it has correct exception spec

[elsteveogrande]

Summary

(PR38627)

After deserializing, the PendingExceptionSpecUpdates table is iterated over to apply exception specs to functions. There may be EST_None-type in this table. However if there is a method in the redecl chain already having some other non-EST_None type, this should not be clobbered with none.

If we see EST_None avoid setting the exception spec. (This is the default anyway, so it's safe to skip in the legit case.)

There may be *better* ways to actually fix this, rather than a simple 1-line defensive check like I did here. It would be more complicated and quite outside my area of experience though.

Test Plan: Passes ninja check-clang, including a new test case.

Andrew Gallagher provided the test case (distilling it from a much more complex instance in our code base).

[rsmith]

I think this is addressing a symptom rather than the cause. The bug appears to be that when parsing a.h, we end up with inconsistent exception specifications along the redeclaration chain. It looks like Sema::AdjustDestructorExceptionSpec doesn't do the right thing when befriending a destructor. Here's a non-modules testcase based on yours that demonstrates either the same problem or at least a closely-related one:

struct B {
  virtual ~B();
  struct C { friend B::~B() noexcept; }; 
};

We produce a bogus error here ("exception specification in declaration does not match previous declaration"). But we think the exception specifications match if class C is moved outside class B.

Generally, we skip checking the exception specification for a destructor until we get to the end of the class. But when the destructor declaration is a friend declaration for a destructor of an enclosing class, we need to instead delay until that enclosing class is complete. I expect the logic to do that is what's missing here.

[elsteveogrande]

Hi @rsmith ! Thanks for taking a look at this. I'd much prefer to fix the underlying problem and not swat at these symptoms, so thanks for the analysis of what's actually going on here. :) Let me take a stab at fixing the real problem in the manner you describe.

[rsmith]

I had a look at fixing this, and it... snowballed into a much larger change than I was expecting. I have a patch on the way.

[rsmith]

Fixed in r341499.

[elsteveogrande]

that's great news, thanks!

lib/Serialization/ASTReader.cpp

if (NumCurrentElementsDeserializing == 0) {
     while (!PendingExceptionSpecUpdates.empty() ||
            !PendingDeducedTypeUpdates.empty()) {
       auto ESUpdates = std::move(PendingExceptionSpecUpdates);
       PendingExceptionSpecUpdates.clear();
       for (auto Update : ESUpdates) {
         ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
         auto *FPT = Update.second->getType()->castAs<FunctionProtoType>();
         auto ESI = FPT->getExtProtoInfo().ExceptionSpec;
+        // Ignore updates with EST_None sorts of exception specifications.
+        // That is the default anyway; also, in the event a FunctionDecl does
+        // have a non-None type, we'll avoid clobbering it.
+        if (ESI.Type != ExceptionSpecificationType::EST_None) {
           if (auto *Listener = getContext().getASTMutationListener())
             Listener->ResolvedExceptionSpec(cast<FunctionDecl>(Update.second));
-        for (auto *Redecl : Update.second->redecls())
+          for (auto *Redecl : Update.second->redecls()) {
+            // FIXME: If the exception spec is already set, ensure it matches.
             getContext().adjustExceptionSpec(cast<FunctionDecl>(Redecl), ESI);
           }
+        }
+      }
 
       auto DTUpdates = std::move(PendingDeducedTypeUpdates);
       PendingDeducedTypeUpdates.clear();
       for (auto Update : DTUpdates) {
         ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
         // FIXME: If the return type is already deduced, check that it matches.
         getContext().adjustDeducedFunctionResultType(Update.first,
                                                      Update.second);

test/Modules/Inputs/PR38627/a.h

+#pragma once
+
+class A {
+ public:
+  virtual ~A() = default;
+};
+class B {
+ public:
+  virtual ~B() {
+    c.func();
+  }
+  struct C {
+    void func() {}
+    friend B::~B();
+  };
+  C c;
+};

test/Modules/Inputs/PR38627/module.modulemap

+module a {
+  header "a.h"
+}

test/Modules/pr38627.cpp

+// RUN: rm -rf %t
+// RUN: %clang -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/PR38627 -c %s
+// expected-no-diagnostics
+
+// PR38627: Exception specifications sometimes go missing / are incorrect
+// after deserializing from a module.
+// https://bugs.llvm.org/show_bug.cgi?id=38627
+
+namespace test_with_unserialized_decls {
+
+// X/Y/Z are the same as A/B/C from: Inputs/PR38627/a.h
+class X {
+ public:
+  virtual ~X() = default;
+};
+class Y {
+ public:
+  virtual ~Y() {
+    z.func();
+  }
+  struct Z {
+    void func() {}
+    friend Y::~Y();
+  };
+  Z z;
+};
+
+// ~Y's exception spec should be calculated to be noexcept.
+static_assert(noexcept(Y().~Y()));
+
+} // end test_with_unserialized_decls
+
+// Same definitions of A and B (but without the namespace) in this module
+#include "a.h"
+
+namespace test_with_deserialized_decls {
+
+static_assert(noexcept(B().~B()), "PR38627");
+
+class D : public A, public B {
+ public:
+  // Error should be gone with bug fix:
+  // "exception specification of overriding function is more lax than base version"
+  virtual ~D() override = default;
+};
+
+} // end test_with_deserialized_decls