[clang] [Clang][Sema] Fix exception specification comparison for functions with different template depths (PR #111561)

Krystian Stasiowski via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 22 09:22:37 PDT 2024


https://github.com/sdkrystian updated https://github.com/llvm/llvm-project/pull/111561

>From d41114109ebfb98570152fe8fdc2854863f23397 Mon Sep 17 00:00:00 2001
From: Krystian Stasiowski <sdkrystian at gmail.com>
Date: Tue, 8 Oct 2024 12:54:26 -0400
Subject: [PATCH 1/4] [Clang][Sema] Fix exception specification comparison for
 functions with different template depths

---
 clang/include/clang/Sema/Sema.h         |   5 ++
 clang/lib/Sema/SemaExceptionSpec.cpp    | 105 +++++++++++++++++++++++-
 clang/test/CXX/basic/basic.link/p11.cpp |  37 +++++++++
 3 files changed, 146 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/CXX/basic/basic.link/p11.cpp

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index bc9c422ed4c477..b295db62bf016d 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5026,6 +5026,11 @@ class Sema final : public SemaBase {
   /// special member function.
   void EvaluateImplicitExceptionSpec(SourceLocation Loc, FunctionDecl *FD);
 
+  bool AreExceptionSpecsEqual(const NamedDecl *Old,
+                              const Expr *OldExceptionSpec,
+                              const NamedDecl *New,
+                              const Expr *NewExceptionSpec);
+
   /// Check the given exception-specification and update the
   /// exception specification information with the results.
   void checkExceptionSpecification(bool IsTopLevel,
diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp
index dbddd6c370aa07..c74686073df228 100644
--- a/clang/lib/Sema/SemaExceptionSpec.cpp
+++ b/clang/lib/Sema/SemaExceptionSpec.cpp
@@ -10,7 +10,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/Expr.h"
@@ -19,6 +18,9 @@
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Sema/EnterExpressionEvaluationContext.h"
+#include "clang/Sema/SemaInternal.h"
+#include "clang/Sema/Template.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include <optional>
@@ -314,6 +316,22 @@ bool Sema::CheckEquivalentExceptionSpec(FunctionDecl *Old, FunctionDecl *New) {
     return false;
   }
 
+  if (Old->getExceptionSpecType() == EST_DependentNoexcept &&
+      New->getExceptionSpecType() == EST_DependentNoexcept) {
+    const auto *OldType = Old->getType()->getAs<FunctionProtoType>();
+    const auto *NewType = New->getType()->getAs<FunctionProtoType>();
+    OldType = ResolveExceptionSpec(New->getLocation(), OldType);
+    if (!OldType)
+      return false;
+    NewType = ResolveExceptionSpec(New->getLocation(), NewType);
+    if (!NewType)
+      return false;
+
+    if (AreExceptionSpecsEqual(Old, OldType->getNoexceptExpr(), New,
+                               NewType->getNoexceptExpr()))
+      return false;
+  }
+
   // Check the types as written: they must match before any exception
   // specification adjustment is applied.
   if (!CheckEquivalentExceptionSpecImpl(
@@ -501,6 +519,89 @@ bool Sema::CheckEquivalentExceptionSpec(
   return Result;
 }
 
+static const Expr *SubstituteExceptionSpecWithoutEvaluation(
+    Sema &S, const Sema::TemplateCompareNewDeclInfo &DeclInfo,
+    const Expr *ExceptionSpec) {
+  MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
+      DeclInfo.getDecl(), DeclInfo.getLexicalDeclContext(),
+      /*Final=*/false, /*Innermost=*/std::nullopt,
+      /*RelativeToPrimary=*/true, /*ForConstraintInstantiation=*/true);
+
+  if (!MLTAL.getNumSubstitutedLevels())
+    return ExceptionSpec;
+
+  Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/false);
+
+  Sema::InstantiatingTemplate Inst(
+      S, DeclInfo.getLocation(),
+      const_cast<FunctionDecl *>(DeclInfo.getDecl()->getAsFunction()),
+      Sema::InstantiatingTemplate::ExceptionSpecification());
+  if (Inst.isInvalid())
+    return nullptr;
+
+  // Set up a dummy 'instantiation' scope in the case of reference to function
+  // parameters that the surrounding function hasn't been instantiated yet. Note
+  // this may happen while we're comparing two templates' constraint
+  // equivalence.
+  LocalInstantiationScope ScopeForParameters(S);
+  if (auto *FD = DeclInfo.getDecl()->getAsFunction())
+    for (auto *PVD : FD->parameters())
+      ScopeForParameters.InstantiatedLocal(PVD, PVD);
+
+  std::optional<Sema::CXXThisScopeRAII> ThisScope;
+
+  // See TreeTransform::RebuildTemplateSpecializationType. A context scope is
+  // essential for having an injected class as the canonical type for a template
+  // specialization type at the rebuilding stage. This guarantees that, for
+  // out-of-line definitions, injected class name types and their equivalent
+  // template specializations can be profiled to the same value, which makes it
+  // possible that e.g. constraints involving C<Class<T>> and C<Class> are
+  // perceived identical.
+  std::optional<Sema::ContextRAII> ContextScope;
+  if (auto *RD = dyn_cast<CXXRecordDecl>(DeclInfo.getDeclContext())) {
+    ThisScope.emplace(S, const_cast<CXXRecordDecl *>(RD), Qualifiers());
+    ContextScope.emplace(S, const_cast<DeclContext *>(cast<DeclContext>(RD)),
+                         /*NewThisContext=*/false);
+  }
+
+  EnterExpressionEvaluationContext ConstantEvaluated(
+      S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+
+  ExprResult SubstExceptionSpec =
+      S.SubstExpr(const_cast<clang::Expr *>(ExceptionSpec), MLTAL);
+  if (SFINAE.hasErrorOccurred() || !SubstExceptionSpec.isUsable())
+    return nullptr;
+  return SubstExceptionSpec.get();
+}
+
+bool Sema::AreExceptionSpecsEqual(const NamedDecl *Old,
+                                  const Expr *OldExceptionSpec,
+                                  const NamedDecl *New,
+                                  const Expr *NewExceptionSpec) {
+  if (OldExceptionSpec == NewExceptionSpec)
+    return true;
+  if (Old && New &&
+      Old->getLexicalDeclContext() != New->getLexicalDeclContext()) {
+    if (const Expr *SubstExceptionSpec =
+            SubstituteExceptionSpecWithoutEvaluation(*this, Old,
+                                                     OldExceptionSpec))
+      OldExceptionSpec = SubstExceptionSpec;
+    else
+      return false;
+    if (const Expr *SubstExceptionSpec =
+            SubstituteExceptionSpecWithoutEvaluation(*this, New,
+                                                     NewExceptionSpec))
+      NewExceptionSpec = SubstExceptionSpec;
+    else
+      return false;
+  }
+
+  llvm::FoldingSetNodeID ID1, ID2;
+  OldExceptionSpec->Profile(ID1, Context, /*Canonical=*/true);
+  NewExceptionSpec->Profile(ID2, Context, /*Canonical=*/true);
+  return ID1 == ID2;
+}
+
 /// CheckEquivalentExceptionSpec - Check if the two types have compatible
 /// exception specifications. See C++ [except.spec]p3.
 ///
@@ -574,6 +675,7 @@ static bool CheckEquivalentExceptionSpecImpl(
     }
   }
 
+#if 0
   // C++14 [except.spec]p3:
   //   Two exception-specifications are compatible if [...] both have the form
   //   noexcept(constant-expression) and the constant-expressions are equivalent
@@ -584,6 +686,7 @@ static bool CheckEquivalentExceptionSpecImpl(
     if (OldFSN == NewFSN)
       return false;
   }
+#endif
 
   // Dynamic exception specifications with the same set of adjusted types
   // are compatible.
diff --git a/clang/test/CXX/basic/basic.link/p11.cpp b/clang/test/CXX/basic/basic.link/p11.cpp
new file mode 100644
index 00000000000000..e244336417fd67
--- /dev/null
+++ b/clang/test/CXX/basic/basic.link/p11.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
+
+namespace MemberSpecialization {
+  template<typename T>
+  struct A {
+    template<bool B>
+    void f() noexcept(B);
+
+    template<bool B>
+    void g() noexcept(B); // expected-note {{previous declaration is here}}
+  };
+
+  template<>
+  template<bool B>
+  void A<int>::f() noexcept(B);
+
+  template<>
+  template<bool B>
+  void A<int>::g() noexcept(!B); // expected-error {{exception specification in declaration does not match previous declaration}}
+}
+
+namespace Friend {
+  template<bool B>
+  void f() noexcept(B);
+
+  template<bool B>
+  void g() noexcept(B); // expected-note {{previous declaration is here}}
+
+  template<typename T>
+  struct A {
+    template<bool B>
+    friend void f() noexcept(B);
+
+    template<bool B>
+    friend void g() noexcept(!B); // expected-error {{exception specification in declaration does not match previous declaration}}
+  };
+}

>From 7446198d879c4aabec3ae4b2d23ab3a50560c7cc Mon Sep 17 00:00:00 2001
From: Krystian Stasiowski <sdkrystian at gmail.com>
Date: Tue, 8 Oct 2024 14:13:59 -0400
Subject: [PATCH 2/4] [FOLD] remove dead code

---
 clang/lib/Sema/SemaExceptionSpec.cpp | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp
index c74686073df228..9ef096ca13e6b4 100644
--- a/clang/lib/Sema/SemaExceptionSpec.cpp
+++ b/clang/lib/Sema/SemaExceptionSpec.cpp
@@ -675,19 +675,6 @@ static bool CheckEquivalentExceptionSpecImpl(
     }
   }
 
-#if 0
-  // C++14 [except.spec]p3:
-  //   Two exception-specifications are compatible if [...] both have the form
-  //   noexcept(constant-expression) and the constant-expressions are equivalent
-  if (OldEST == EST_DependentNoexcept && NewEST == EST_DependentNoexcept) {
-    llvm::FoldingSetNodeID OldFSN, NewFSN;
-    Old->getNoexceptExpr()->Profile(OldFSN, S.Context, true);
-    New->getNoexceptExpr()->Profile(NewFSN, S.Context, true);
-    if (OldFSN == NewFSN)
-      return false;
-  }
-#endif
-
   // Dynamic exception specifications with the same set of adjusted types
   // are compatible.
   if (OldEST == EST_Dynamic && NewEST == EST_Dynamic) {

>From 1be01ebe5051b1e34a5a15936ce7e7e39a3b2761 Mon Sep 17 00:00:00 2001
From: Krystian Stasiowski <sdkrystian at gmail.com>
Date: Fri, 18 Oct 2024 11:36:00 -0400
Subject: [PATCH 3/4] [FOLD] simplify SubstituteExceptionSpecWithoutEvaluation

---
 clang/lib/Sema/SemaExceptionSpec.cpp | 53 +++++++++++++++++++---------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp
index 9ef096ca13e6b4..0f000b4da5f660 100644
--- a/clang/lib/Sema/SemaExceptionSpec.cpp
+++ b/clang/lib/Sema/SemaExceptionSpec.cpp
@@ -523,18 +523,19 @@ static const Expr *SubstituteExceptionSpecWithoutEvaluation(
     Sema &S, const Sema::TemplateCompareNewDeclInfo &DeclInfo,
     const Expr *ExceptionSpec) {
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
-      DeclInfo.getDecl(), DeclInfo.getLexicalDeclContext(),
-      /*Final=*/false, /*Innermost=*/std::nullopt,
-      /*RelativeToPrimary=*/true, /*ForConstraintInstantiation=*/true);
+      DeclInfo.getDecl(), DeclInfo.getLexicalDeclContext(), /*Final=*/false,
+      /*Innermost=*/std::nullopt,
+      /*RelativeToPrimary=*/true,
+      /*ForConstraintInstantiation=*/true);
 
-  if (!MLTAL.getNumSubstitutedLevels())
+  if (MLTAL.getNumSubstitutedLevels() == 0)
     return ExceptionSpec;
 
   Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/false);
 
+  auto *FD = const_cast<FunctionDecl *>(DeclInfo.getDecl()->getAsFunction());
   Sema::InstantiatingTemplate Inst(
-      S, DeclInfo.getLocation(),
-      const_cast<FunctionDecl *>(DeclInfo.getDecl()->getAsFunction()),
+      S, DeclInfo.getLocation(), FD,
       Sema::InstantiatingTemplate::ExceptionSpecification());
   if (Inst.isInvalid())
     return nullptr;
@@ -544,11 +545,31 @@ static const Expr *SubstituteExceptionSpecWithoutEvaluation(
   // this may happen while we're comparing two templates' constraint
   // equivalence.
   LocalInstantiationScope ScopeForParameters(S);
-  if (auto *FD = DeclInfo.getDecl()->getAsFunction())
-    for (auto *PVD : FD->parameters())
-      ScopeForParameters.InstantiatedLocal(PVD, PVD);
 
-  std::optional<Sema::CXXThisScopeRAII> ThisScope;
+  for (auto *PVD : FD->parameters()) {
+    if (!PVD->isParameterPack()) {
+      ScopeForParameters.InstantiatedLocal(PVD, PVD);
+      continue;
+    }
+    // This is hacky: we're mapping the parameter pack to a size-of-1 argument
+    // to avoid building SubstTemplateTypeParmPackTypes for
+    // PackExpansionTypes. The SubstTemplateTypeParmPackType node would
+    // otherwise reference the AssociatedDecl of the template arguments, which
+    // is, in this case, the template declaration.
+    //
+    // However, as we are in the process of comparing potential
+    // re-declarations, the canonical declaration is the declaration itself at
+    // this point. So if we didn't expand these packs, we would end up with an
+    // incorrect profile difference because we will be profiling the
+    // canonical types!
+    //
+    // FIXME: Improve the "no-transform" machinery in FindInstantiatedDecl so
+    // that we can eliminate the Scope in the cases where the declarations are
+    // not necessarily instantiated. It would also benefit the noexcept
+    // specifier comparison.
+    ScopeForParameters.MakeInstantiatedLocalArgPack(PVD);
+    ScopeForParameters.InstantiatedLocalPackArg(PVD, PVD);
+  }
 
   // See TreeTransform::RebuildTemplateSpecializationType. A context scope is
   // essential for having an injected class as the canonical type for a template
@@ -557,12 +578,12 @@ static const Expr *SubstituteExceptionSpecWithoutEvaluation(
   // template specializations can be profiled to the same value, which makes it
   // possible that e.g. constraints involving C<Class<T>> and C<Class> are
   // perceived identical.
-  std::optional<Sema::ContextRAII> ContextScope;
-  if (auto *RD = dyn_cast<CXXRecordDecl>(DeclInfo.getDeclContext())) {
-    ThisScope.emplace(S, const_cast<CXXRecordDecl *>(RD), Qualifiers());
-    ContextScope.emplace(S, const_cast<DeclContext *>(cast<DeclContext>(RD)),
-                         /*NewThisContext=*/false);
-  }
+  Sema::ContextRAII ContextScope(S, FD);
+
+  auto *MD = dyn_cast<CXXMethodDecl>(FD);
+  Sema::CXXThisScopeRAII ThisScope(
+      S, MD ? MD->getParent() : nullptr,
+      MD ? MD->getMethodQualifiers() : Qualifiers{}, MD != nullptr);
 
   EnterExpressionEvaluationContext ConstantEvaluated(
       S, Sema::ExpressionEvaluationContext::ConstantEvaluated);

>From 7df326227711c3247ca2b52c1d86ceb33eac8629 Mon Sep 17 00:00:00 2001
From: Krystian Stasiowski <sdkrystian at gmail.com>
Date: Fri, 18 Oct 2024 13:18:49 -0400
Subject: [PATCH 4/4] [FOLD] fix typo

---
 clang/lib/Sema/SemaExceptionSpec.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp
index 0f000b4da5f660..a532c0a2c84e0c 100644
--- a/clang/lib/Sema/SemaExceptionSpec.cpp
+++ b/clang/lib/Sema/SemaExceptionSpec.cpp
@@ -320,7 +320,7 @@ bool Sema::CheckEquivalentExceptionSpec(FunctionDecl *Old, FunctionDecl *New) {
       New->getExceptionSpecType() == EST_DependentNoexcept) {
     const auto *OldType = Old->getType()->getAs<FunctionProtoType>();
     const auto *NewType = New->getType()->getAs<FunctionProtoType>();
-    OldType = ResolveExceptionSpec(New->getLocation(), OldType);
+    OldType = ResolveExceptionSpec(Old->getLocation(), OldType);
     if (!OldType)
       return false;
     NewType = ResolveExceptionSpec(New->getLocation(), NewType);



More information about the cfe-commits mailing list