[clang] Represent array refs as `TemplateArgument::Declaration` #80050

bolshakov-a · 2024-01-30T20:03:37Z

This returns (probably temporarily) array-referring NTTP behavior to which was prior to #78041 because ~~I'm fed up~~ have no time to fix regressions.

llvmbot · 2024-01-30T20:04:08Z

@llvm/pr-subscribers-clang

Author: Andrey Ali Khan Bolshakov (bolshakov-a)

Changes

This returns (probably temporarily) array-referring NTTP behavior to which was prior to #78041 because ~~I'm fed up~~ have no time to fix regressions.

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

1 Files Affected:

(modified) clang/lib/Sema/SemaTemplate.cpp (+24-20)

diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 3f33ecb89502e..9e8d058041f04 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -7417,9 +7417,9 @@ ExprResult Sema::CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
     if (ArgResult.isInvalid())
       return ExprError();
 
-    // Prior to C++20, enforce restrictions on possible template argument
-    // values.
-    if (!getLangOpts().CPlusPlus20 && Value.isLValue()) {
+    if (Value.isLValue()) {
+      APValue::LValueBase Base = Value.getLValueBase();
+      auto *VD = const_cast<ValueDecl *>(Base.dyn_cast<const ValueDecl *>());
       //   For a non-type template-parameter of pointer or reference type,
       //   the value of the constant expression shall not refer to
       assert(ParamType->isPointerType() || ParamType->isReferenceType() ||
@@ -7428,8 +7428,6 @@ ExprResult Sema::CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
       // -- a string literal
       // -- the result of a typeid expression, or
       // -- a predefined __func__ variable
-      APValue::LValueBase Base = Value.getLValueBase();
-      auto *VD = const_cast<ValueDecl *>(Base.dyn_cast<const ValueDecl *>());
       if (Base &&
           (!VD ||
            isa<LifetimeExtendedTemporaryDecl, UnnamedGlobalConstantDecl>(VD))) {
@@ -7437,24 +7435,30 @@ ExprResult Sema::CheckTemplateArgument(NonTypeTemplateParmDecl *Param,
             << Arg->getSourceRange();
         return ExprError();
       }
-      // -- a subobject [until C++20]
-      if (Value.hasLValuePath() && Value.getLValuePath().size() == 1 &&
-          VD && VD->getType()->isArrayType() &&
+
+      if (Value.hasLValuePath() && Value.getLValuePath().size() == 1 && VD &&
+          VD->getType()->isArrayType() &&
           Value.getLValuePath()[0].getAsArrayIndex() == 0 &&
           !Value.isLValueOnePastTheEnd() && ParamType->isPointerType()) {
-        // Per defect report (no number yet):
-        //   ... other than a pointer to the first element of a complete array
-        //       object.
-      } else if (!Value.hasLValuePath() || Value.getLValuePath().size() ||
-                 Value.isLValueOnePastTheEnd()) {
-        Diag(StartLoc, diag::err_non_type_template_arg_subobject)
-          << Value.getAsString(Context, ParamType);
-        return ExprError();
+        SugaredConverted = TemplateArgument(VD, ParamType);
+        CanonicalConverted = TemplateArgument(
+            cast<ValueDecl>(VD->getCanonicalDecl()), CanonParamType);
+        return ArgResult.get();
+      }
+
+      // -- a subobject [until C++20]
+      if (!getLangOpts().CPlusPlus20) {
+        if (!Value.hasLValuePath() || Value.getLValuePath().size() ||
+            Value.isLValueOnePastTheEnd()) {
+          Diag(StartLoc, diag::err_non_type_template_arg_subobject)
+              << Value.getAsString(Context, ParamType);
+          return ExprError();
+        }
+        assert((VD || !ParamType->isReferenceType()) &&
+               "null reference should not be a constant expression");
+        assert((!VD || !ParamType->isNullPtrType()) &&
+               "non-null value of type nullptr_t?");
       }
-      assert((VD || !ParamType->isReferenceType()) &&
-             "null reference should not be a constant expression");
-      assert((!VD || !ParamType->isNullPtrType()) &&
-             "non-null value of type nullptr_t?");
     }
 
     if (Value.isAddrLabelDiff())

bolshakov-a · 2024-01-30T20:04:10Z

@erichkeane, @cor3ntin

erichkeane

Are there any release-notes that should be reverted with this? or at least modified, since this is reverting stuff?

Also, can you please add the tests/regressions that were found that have lead to this partial revert?

Also-- We hope you find more time to help with the regressions, your contributions so far have been really appreciated!

cor3ntin · 2024-01-30T21:24:18Z

@AaronBallman @zygoloid
There probably is a missing ODR-use marking somewhere, if you have a clue
#78041 (comment)

Alas we do not seem to have a repro for the chromium-reported issue which make debugging it difficult.

erichkeane · 2024-01-30T21:27:16Z

@AaronBallman @zygoloid There probably is a missing ODR-use marking somewhere, if you have a clue #78041 (comment)

Alas we do not seem to have a repro for the chromium-reported issue which make debugging it difficult.

SMH

bolshakov-a · 2024-01-30T21:39:03Z

Are there any release-notes that should be reverted with this? or at least modified, since this is reverting stuff?

No, it is intended only to fix potential bugs with already-written user's code (I suspect there are more places where OVE->getSourceExpr() should be checked, besides the last reported bug) without reverting the new NTTP feature (which may also contain bugs, but is marked as experimental). Just arrays are switched to the old mechanism.

Also, can you please add the tests/regressions that were found that have lead to this partial revert?

I'd add the test if the repro would be provided. As far, I can revert the changes in code from #79764, and the tests from it would test this change.

We hope you find more time to help with the regressions, your contributions so far have been really appreciated!

Thanks! Probably, on my vacation in summer...

Alas we do not seem to have a repro for the chromium-reported issue which make debugging it difficult.

Do you mean this? I've succeeded in reducing it to that.

erichkeane

I don't have concerns as written. @cor3ntin : feel free to accept if you like it too.

bolshakov-a · 2024-01-30T22:25:28Z

The changes under that discussion could now be reverted as well because they are now useless. Should I make a separate commit inside this PR?

jeremiahar · 2024-01-31T08:05:27Z

This also avoids regressing #79957 (I cherrypicked this commit and compiled the test case locally)

This returns (probably temporarily) array-referring NTTP behavior to which was prior to llvm#78041 to avoid regressions.

bolshakov-a · 2024-01-31T11:49:02Z

@jeremiahar, thanks for reporting! Added a coverage mapping test case.

cor3ntin

I will accept that as it seems to unblock some folks,
but I'd like to understand the root cause and what a proper fix would look like

bolshakov-a · 2024-01-31T13:45:11Z

I think the problem is that the generalized NTTP feature is still raw. It should not break already working features, namely array reference NTTPs, hence they are swithed back to the old TemplateArgument::Declaration, as they worked prior to #78041, from the new StructuralValue.

bolshakov-a · 2024-01-31T13:48:59Z

what a proper fix would look like

Probably, some more OpaqueValueExpr handlers should be rewritten correctly.

bolshakov-a · 2024-01-31T13:49:47Z

@cor3ntin, @erichkeane, could you merge it please?

bolshakov-a · 2024-01-31T15:11:52Z

Thanks! Should I open an issue to backport this to RC?

erichkeane · 2024-01-31T15:12:24Z

Thanks! Should I open an issue to backport this to RC?

Please do!

bolshakov-a · 2024-01-31T15:21:59Z

Please do!

#80150. I don't know, maybe, some more actions should be done. I cannot add "release:backport" label, at least.

erichkeane · 2024-01-31T15:23:10Z

Please do!

#80150. I don't know, maybe, some more actions should be done. I cannot add "release:backport" label, at least.

I think you just have to set hte milestone, which I did.

…#80050) This returns (probably temporarily) array-referring NTTP behavior to which was prior to llvm#78041 because ~~I'm fed up~~ have no time to fix regressions. (cherry picked from commit 9bf4e54)

Introduced in llvm#78041, originally reported as llvm#79957 and fixed partially in llvm#80050. `OpaqueValueExpr` used with `TemplateArgument::StructuralValue` has no corresponding source expression. A test case with subobject-referring NTTP added.

…#85837) Introduced in #78041, originally reported as #79957 and fixed partially in #80050. `OpaqueValueExpr` used with `TemplateArgument::StructuralValue` has no corresponding source expression. A test case with subobject-referring NTTP added.

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

bolshakov-a mentioned this pull request Jan 30, 2024

[c++20] P1907R1: Support for generalized non-type template arguments of scalar type. #78041

Merged

erichkeane reviewed Jan 30, 2024

View reviewed changes

[clang] Represent array refs as TemplateArgument::Declaration

ae4b26c

This returns (probably temporarily) array-referring NTTP behavior to which was prior to llvm#78041 to avoid regressions.

bolshakov-a force-pushed the avoid_regressions branch from 0ccf1c9 to ae4b26c Compare January 31, 2024 11:48

cor3ntin approved these changes Jan 31, 2024

View reviewed changes

erichkeane merged commit 9bf4e54 into llvm:main Jan 31, 2024

bolshakov-a deleted the avoid_regressions branch January 31, 2024 15:12

jeremiahar mentioned this pull request Feb 11, 2024

Clang 18.1.0-rc / trunk crash when generating code with coverage information - `detail::isPresent(Val) && "dyn_cast on a non-existent value"' failed #79957

Closed

bolshakov-a mentioned this pull request Mar 19, 2024

[clang][c++20] Fix code coverage mapping crash with generalized NTTPs #85837

Merged

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Represent array refs as `TemplateArgument::Declaration` #80050

[clang] Represent array refs as `TemplateArgument::Declaration` #80050

bolshakov-a commented Jan 30, 2024

llvmbot commented Jan 30, 2024

bolshakov-a commented Jan 30, 2024

erichkeane left a comment •

edited

Loading

cor3ntin commented Jan 30, 2024

erichkeane commented Jan 30, 2024

bolshakov-a commented Jan 30, 2024

erichkeane left a comment

bolshakov-a commented Jan 30, 2024

jeremiahar commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

cor3ntin left a comment

bolshakov-a commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

erichkeane commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

erichkeane commented Jan 31, 2024

[clang] Represent array refs as TemplateArgument::Declaration #80050

[clang] Represent array refs as TemplateArgument::Declaration #80050

Conversation

bolshakov-a commented Jan 30, 2024

llvmbot commented Jan 30, 2024

bolshakov-a commented Jan 30, 2024

erichkeane left a comment • edited Loading

Choose a reason for hiding this comment

cor3ntin commented Jan 30, 2024

erichkeane commented Jan 30, 2024

bolshakov-a commented Jan 30, 2024

erichkeane left a comment

Choose a reason for hiding this comment

bolshakov-a commented Jan 30, 2024

jeremiahar commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

cor3ntin left a comment

Choose a reason for hiding this comment

bolshakov-a commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

erichkeane commented Jan 31, 2024

bolshakov-a commented Jan 31, 2024

erichkeane commented Jan 31, 2024

[clang] Represent array refs as `TemplateArgument::Declaration` #80050

[clang] Represent array refs as `TemplateArgument::Declaration` #80050

erichkeane left a comment •

edited

Loading