elsteveogrande created this revision.
elsteveogrande added reviewers: rsmith, dblaikie.
Herald added a subscriber: cfe-commits.
(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).
Repository:
rC Clang
https://reviews.llvm.org/D51608
Files:
lib/Serialization/ASTReader.cpp
test/Modules/Inputs/PR38627/a.h
test/Modules/Inputs/PR38627/module.modulemap
test/Modules/pr38627.cpp
Index: test/Modules/pr38627.cpp
===
--- /dev/null
+++ test/Modules/pr38627.cpp
@@ -0,0 +1,47 @@
+// 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
Index: test/Modules/Inputs/PR38627/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/PR38627/module.modulemap
@@ -0,0 +1,3 @@
+module a {
+ header "a.h"
+}
Index: test/Modules/Inputs/PR38627/a.h
===
--- /dev/null
+++ test/Modules/Inputs/PR38627/a.h
@@ -0,0 +1,17 @@
+#pragma once
+
+class A {
+ public:
+ virtual ~A() = default;
+};
+class B {
+ public:
+ virtual ~B() {
+c.func();
+ }
+ struct C {
+void func() {}
+friend B::~B();
+ };
+ C c;
+};
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -11551,10 +11551,17 @@
ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
auto *FPT = Update.second->getType()->castAs();
auto ESI = FPT->getExtProtoInfo().ExceptionSpec;
-if (auto *Listener = getContext().getASTMutationListener())
- Listener->ResolvedExceptionSpec(cast(Update.second));
-for (auto *Redecl : Update.second->redecls())
- getContext().adjustExceptionSpec(cast(Redecl), ESI);
+// 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(Update.second));
+ for (auto *Redecl : Update.second->redecls()) {
+// FIXME: If the exception spec is already set, ensure it matches.
+getContext().adjustExceptionSpec(cast(Redecl), ESI);
+ }
+}
}
auto DTUpdates = std::move(PendingDeducedTypeUpdates);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
