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<FunctionProtoType>(); auto ESI = FPT->getExtProtoInfo().ExceptionSpec; - if (auto *Listener = getContext().getASTMutationListener()) - Listener->ResolvedExceptionSpec(cast<FunctionDecl>(Update.second)); - for (auto *Redecl : Update.second->redecls()) - getContext().adjustExceptionSpec(cast<FunctionDecl>(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<FunctionDecl>(Update.second)); + 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);
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
