[Clang][Concepts] Normalize SizeOfPackExpr's pack declaration #110238

zyn0217 · 2024-09-27T10:26:42Z

SizeOfPackExpr has a pointer to the referenced pack declaration, which is left as-is during the transformation process.

The situation could be subtle when a friend class template declaration comes into play. The declaration per se would be instantiated into its parent declaration context, and consequently, the template parameter list would have a depth adjustment; however, as we don't evaluate constraints during instantiation, those constraints would still reference the original template parameters, which is fine for constraint evaluation because we have handled friend cases in the template argument collection.

However, things are different when we want to profile the constraint expression with dependent template arguments. The hash algorithm of SizeOfPackExpr takes its pack declaration as a factor, which is the original template parameter that might still have untransformed template depths after the constraint normalization.

This patch transforms the pack declaration when normalizing constraint expressions and pluses a fix in HandleFunctionTemplateDecl() where the associated declaration is incorrect for nested specifiers.

Note that the fix in HandleFunctionTemplateDecl(), as well as the handling logic for NestedNameSpecifier, would be removed once Krystian's refactoring patch lands. But I still want to incorporate it in the patch for the correction purpose, though it hasn't caused any problems so far - I just tripped over that in getFullyPackExpandedSize() when I tried to extract the transformed declarations from the TemplateArgument.

Fixes #93099

This reverts commit 23a765a.

zyn0217 · 2024-09-28T14:47:10Z

The CI failure looks unrelated - it also failed in other PRs e.g.

https://buildkite.com/llvm-project/github-pull-requests/builds/104957#019238e6-fd32-40a1-987a-4cd1c539926a

(Which happens to fail in std::common_type that might have connections with the sizeof... expression)

llvmbot · 2024-09-28T14:56:34Z

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

SizeOfPackExpr has a pointer to the referenced pack declaration, which is left as-is during the transformation process.

The situation could be subtle when a friend class template declaration comes into play. The declaration per se would be instantiated into its parent declaration context, and consequently, the template parameter list would have a depth adjustment; however, as we don't evaluate constraints during instantiation, those constraints would still reference the original template parameters, which is fine for constraint evaluation because we have handled friend cases in the template argument collection.

However, things are different when we want to profile the constraint expression with dependent template arguments. The hash algorithm of SizeOfPackExpr takes its pack declaration as a factor, which is the original template parameter that might still have untransformed template depths after the constraint normalization.

This patch transforms the pack declaration when normalizing constraint expressions and pluses a fix in HandleFunctionTemplateDecl() where the associated declaration is incorrect for nested specifiers.

Note that the fix in HandleFunctionTemplateDecl(), as well as the handling logic for NestedNameSpecifier, would be removed once Krystian's refactoring patch lands. But I still want to incorporate it in the patch for the correction purpose, though it hasn't caused any problems so far - I just tripped over that in getFullyPackExpandedSize() when I tried to extract the transformed declarations from the TemplateArgument.

Fixes #93099

Full diff: https://github.com/llvm/llvm-project/pull/110238.diff

5 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+2)
(modified) clang/include/clang/AST/ExprCXX.h (+2)
(modified) clang/lib/Sema/SemaConcept.cpp (+9-5)
(modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+27-1)
(modified) clang/test/SemaTemplate/concepts-out-of-line-def.cpp (+34)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1fbcac807d0b30..831dab0657fcaf 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -444,6 +444,8 @@ Bug Fixes to C++ Support
 - Fixed an assertion failure in debug mode, and potential crashes in release mode, when
   diagnosing a failed cast caused indirectly by a failed implicit conversion to the type of the constructor parameter.
 - Fixed an assertion failure by adjusting integral to boolean vector conversions (#GH108326)
+- Fixed a bug in constraint expression comparison where the ``sizeof...`` expression was not handled properly
+  in certain friend declarations. (#GH93099)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 975bcdac5069b9..dfcf739ed1614f 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -4326,6 +4326,8 @@ class SizeOfPackExpr final
   /// Retrieve the parameter pack.
   NamedDecl *getPack() const { return Pack; }
 
+  void setPack(NamedDecl *NewPack) { Pack = NewPack; }
+
   /// Retrieve the length of the parameter pack.
   ///
   /// This routine may only be invoked when the expression is not
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 6a1b32598bb4a6..67fc603e9ce1d5 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -975,11 +975,14 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
   // 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())
+  std::optional<LocalInstantiationScope> ScopeForParameters;
+  if (const NamedDecl *ND = DeclInfo.getDecl();
+      ND && ND->isFunctionOrFunctionTemplate()) {
+    ScopeForParameters.emplace(S);
+    const FunctionDecl *FD = ND->getAsFunction();
     for (auto *PVD : FD->parameters()) {
       if (!PVD->isParameterPack()) {
-        ScopeForParameters.InstantiatedLocal(PVD, PVD);
+        ScopeForParameters->InstantiatedLocal(PVD, PVD);
         continue;
       }
       // This is hacky: we're mapping the parameter pack to a size-of-1 argument
@@ -998,9 +1001,10 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
       // 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);
+      ScopeForParameters->MakeInstantiatedLocalArgPack(PVD);
+      ScopeForParameters->InstantiatedLocalPackArg(PVD, PVD);
     }
+  }
 
   std::optional<Sema::CXXThisScopeRAII> ThisScope;
 
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index fd51fa4afcacbf..20215d32539af9 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -371,7 +371,7 @@ Response HandleFunctionTemplateDecl(const FunctionTemplateDecl *FTD,
                   Specialization->getTemplateInstantiationArgs().asArray();
           }
           Result.addOuterTemplateArguments(
-              const_cast<FunctionTemplateDecl *>(FTD), Arguments,
+              TSTy->getTemplateName().getAsTemplateDecl(), Arguments,
               /*Final=*/false);
         }
       }
@@ -1722,6 +1722,32 @@ namespace {
       return inherited::TransformLambdaBody(E, Body);
     }
 
+    ExprResult TransformSizeOfPackExpr(SizeOfPackExpr *E) {
+      ExprResult Result = inherited::TransformSizeOfPackExpr(E);
+
+      if (SemaRef.CodeSynthesisContexts.back().Kind !=
+          Sema::CodeSynthesisContext::ConstraintNormalization)
+        return Result;
+
+      if (!Result.isUsable())
+        return Result;
+
+      SizeOfPackExpr *NewExpr = cast<SizeOfPackExpr>(Result.get());
+#ifndef NDEBUG
+      for (auto *Iter = TemplateArgs.begin(); Iter != TemplateArgs.end();
+           ++Iter)
+        for (const TemplateArgument &TA : Iter->Args)
+          assert(TA.getKind() != TemplateArgument::Pack || TA.pack_size() == 1);
+#endif
+      Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(SemaRef, 0);
+      Decl *NewDecl = TransformDecl(NewExpr->getPackLoc(), NewExpr->getPack());
+      if (!NewDecl)
+        return ExprError();
+
+      NewExpr->setPack(cast<NamedDecl>(NewDecl));
+      return NewExpr;
+    }
+
     ExprResult TransformRequiresExpr(RequiresExpr *E) {
       LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
       ExprResult TransReq = inherited::TransformRequiresExpr(E);
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index 5450d105a6f54a..8ca399a0f729a9 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -666,3 +666,37 @@ int foo() {
 }
 
 } // namespace eve
+
+namespace GH93099 {
+
+// Issues with sizeof...(expr)
+
+template <typename T = int> struct C {
+  template <int... N>
+    requires(sizeof...(N) > 0)
+  friend class NTTP;
+
+  template <class... Tp>
+    requires(sizeof...(Tp) > 0)
+  friend class TP;
+
+  template <template <typename> class... TTp>
+    requires(sizeof...(TTp) > 0)
+  friend class TTP;
+};
+
+template <int... N>
+  requires(sizeof...(N) > 0)
+class NTTP;
+
+template <class... Tp>
+  requires(sizeof...(Tp) > 0)
+class TP;
+
+template <template <typename> class... TTp>
+  requires(sizeof...(TTp) > 0)
+class TTP;
+
+C v;
+
+} // namespace GH93099

clang/lib/Sema/SemaTemplateInstantiate.cpp

mizvekov

LGTM, Thanks!

Co-authored-by: Matheus Izvekov <[email protected]>

github-actions · 2024-10-01T00:43:17Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…10238) SizeOfPackExpr has a pointer to the referenced pack declaration, which is left as-is during the transformation process. The situation could be subtle when a friend class template declaration comes into play. The declaration per se would be instantiated into its parent declaration context, and consequently, the template parameter list would have a depth adjustment; however, as we don't evaluate constraints during instantiation, those constraints would still reference the original template parameters, which is fine for constraint evaluation because we have handled friend cases in the template argument collection. However, things are different when we want to profile the constraint expression with dependent template arguments. The hash algorithm of SizeOfPackExpr takes its pack declaration as a factor, which is the original template parameter that might still have untransformed template depths after the constraint normalization. This patch transforms the pack declaration when normalizing constraint expressions and pluses a fix in HandleFunctionTemplateDecl() where the associated declaration is incorrect for nested specifiers. Note that the fix in HandleFunctionTemplateDecl(), as well as the handling logic for NestedNameSpecifier, would be removed once Krystian's refactoring patch lands. But I still want to incorporate it in the patch for the correction purpose, though it hasn't caused any problems so far - I just tripped over that in getFullyPackExpandedSize() when I tried to extract the transformed declarations from the TemplateArgument. Fixes llvm#93099 --------- Co-authored-by: Matheus Izvekov <[email protected]>

zyn0217 added 5 commits September 27, 2024 18:23

[Clang] GH93099

23a765a

Fix CI - the associated Decls are wrong previously

e36ecb4

Revert "[Clang] GH93099"

a798926

This reverts commit 23a765a.

Only transform SizeOfPackExprs' pack decls when normalizing constraints

9c72edc

Remove unnecessary includes

1e384b5

Add a release note

391696e

zyn0217 changed the title ~~[Clang] GH93099~~ [Clang][Concepts] Normalize SizeOfPackExpr's pack declaration Sep 28, 2024

zyn0217 requested review from cor3ntin, erichkeane, sdkrystian and mizvekov September 28, 2024 14:55

zyn0217 marked this pull request as ready for review September 28, 2024 14:56

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 28, 2024

Use RebuildSizeOfPackExpr instead

3144698

erichkeane approved these changes Sep 30, 2024

View reviewed changes

Merge branch 'main' into issue-93099

91ef893

mizvekov reviewed Oct 1, 2024

View reviewed changes

clang/lib/Sema/SemaTemplateInstantiate.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaTemplateInstantiate.cpp Show resolved Hide resolved

mizvekov approved these changes Oct 1, 2024

View reviewed changes

zyn0217 and others added 2 commits October 1, 2024 08:37

Apply suggestions from mizvekov

7eec00e

Co-authored-by: Matheus Izvekov <[email protected]>

Merge branch 'main' into issue-93099

4a9154a

Address format issue

ed9d3f7

zyn0217 merged commit 463a4f1 into llvm:main Oct 1, 2024
9 checks passed

zyn0217 deleted the issue-93099 branch October 1, 2024 04:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][Concepts] Normalize SizeOfPackExpr's pack declaration #110238

[Clang][Concepts] Normalize SizeOfPackExpr's pack declaration #110238

Uh oh!

zyn0217 commented Sep 27, 2024 •

edited

Loading

Uh oh!

zyn0217 commented Sep 28, 2024

Uh oh!

llvmbot commented Sep 28, 2024

Uh oh!

Uh oh!

Uh oh!

mizvekov left a comment

Uh oh!

github-actions bot commented Oct 1, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

[Clang][Concepts] Normalize SizeOfPackExpr's pack declaration #110238

[Clang][Concepts] Normalize SizeOfPackExpr's pack declaration #110238

Uh oh!

Conversation

zyn0217 commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zyn0217 commented Sep 28, 2024

Uh oh!

llvmbot commented Sep 28, 2024

Uh oh!

Uh oh!

Uh oh!

mizvekov left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zyn0217 commented Sep 27, 2024 •

edited

Loading

github-actions bot commented Oct 1, 2024 •

edited

Loading