Skip to content

[clang][Sema] Fix initialization of NonTypeTemplateParmDecl... #121768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Feb 19, 2025
Merged

[clang][Sema] Fix initialization of NonTypeTemplateParmDecl... #121768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
da2bbf9
[clang][Sema] Fix initialization of `NonTypeTemplateParmDecl`...
alejandro-alvarez-sonarsource Nov 18, 2024
fc14083
`BuildDeclRefExpr` should not fail
alejandro-alvarez-sonarsource Jan 6, 2025
ab1616f
Refactor to avoid duplication
alejandro-alvarez-sonarsource Jan 6, 2025
12d918f
Revert `RecoverExpr`
alejandro-alvarez-sonarsource Jan 9, 2025
792fb35
Test only reduced BMI
alejandro-alvarez-sonarsource Jan 9, 2025
869eae8
Merge remote-tracking branch 'upstream/main' into aa/fix_init_nttp
alejandro-alvarez-sonarsource Jan 20, 2025
38d144e
Do not serialize PlaceholderTypeConstraintInitialized if
alejandro-alvarez-sonarsource Jan 20, 2025
5805f98
Merge remote-tracking branch 'upstream/main' into aa/fix_init_nttp
alejandro-alvarez-sonarsource Jan 23, 2025
d343d5e
Merge remote-tracking branch 'upstream/main' into aa/fix_init_nttp
alejandro-alvarez-sonarsource Jan 24, 2025
cfd9d69
Simpler approach
alejandro-alvarez-sonarsource Jan 24, 2025
5bf0d70
Merge branch 'main' into aa/fix_init_nttp
alejandro-alvarez-sonarsource Feb 11, 2025
786d7bd
Change order of const
alejandro-alvarez-sonarsource Feb 12, 2025
b51566b
Update clang/lib/AST/DeclTemplate.cpp
alejandro-alvarez-sonarsource Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 29 additions & 16 deletions clang/lib/AST/DeclTemplate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,12 +786,16 @@ NonTypeTemplateParmDecl *NonTypeTemplateParmDecl::Create(
QualType T, bool ParameterPack, TypeSourceInfo *TInfo) {
AutoType *AT =
C.getLangOpts().CPlusPlus20 ? T->getContainedAutoType() : nullptr;
return new (C, DC,
additionalSizeToAlloc<std::pair<QualType, TypeSourceInfo *>,
Expr *>(0,
AT && AT->isConstrained() ? 1 : 0))
NonTypeTemplateParmDecl(DC, StartLoc, IdLoc, D, P, Id, T, ParameterPack,
TInfo);
const bool HasConstraint = AT && AT->isConstrained();
auto *NTTP =
new (C, DC,
additionalSizeToAlloc<std::pair<QualType, TypeSourceInfo *>, Expr *>(
0, HasConstraint ? 1 : 0))
NonTypeTemplateParmDecl(DC, StartLoc, IdLoc, D, P, Id, T,
ParameterPack, TInfo);
if (HasConstraint)
NTTP->setPlaceholderTypeConstraint(nullptr);
return NTTP;
}

NonTypeTemplateParmDecl *NonTypeTemplateParmDecl::Create(
Expand All @@ -800,23 +804,30 @@ NonTypeTemplateParmDecl *NonTypeTemplateParmDecl::Create(
QualType T, TypeSourceInfo *TInfo, ArrayRef<QualType> ExpandedTypes,
ArrayRef<TypeSourceInfo *> ExpandedTInfos) {
AutoType *AT = TInfo->getType()->getContainedAutoType();
return new (C, DC,
additionalSizeToAlloc<std::pair<QualType, TypeSourceInfo *>,
Expr *>(
ExpandedTypes.size(), AT && AT->isConstrained() ? 1 : 0))
NonTypeTemplateParmDecl(DC, StartLoc, IdLoc, D, P, Id, T, TInfo,
ExpandedTypes, ExpandedTInfos);
const bool HasConstraint = AT && AT->isConstrained();
auto *NTTP =
new (C, DC,
additionalSizeToAlloc<std::pair<QualType, TypeSourceInfo *>, Expr *>(
ExpandedTypes.size(), HasConstraint ? 1 : 0))
NonTypeTemplateParmDecl(DC, StartLoc, IdLoc, D, P, Id, T, TInfo,
ExpandedTypes, ExpandedTInfos);
if (HasConstraint)
NTTP->setPlaceholderTypeConstraint(nullptr);
return NTTP;
}

NonTypeTemplateParmDecl *
NonTypeTemplateParmDecl::CreateDeserialized(ASTContext &C, GlobalDeclID ID,
bool HasTypeConstraint) {
return new (C, ID, additionalSizeToAlloc<std::pair<QualType,
TypeSourceInfo *>,
Expr *>(0,
HasTypeConstraint ? 1 : 0))
auto *NTTP =
new (C, ID,
additionalSizeToAlloc<std::pair<QualType, TypeSourceInfo *>, Expr *>(
0, HasTypeConstraint ? 1 : 0))
NonTypeTemplateParmDecl(nullptr, SourceLocation(), SourceLocation(),
0, 0, nullptr, QualType(), false, nullptr);
if (HasTypeConstraint)
NTTP->setPlaceholderTypeConstraint(nullptr);
return NTTP;
}

NonTypeTemplateParmDecl *
Expand All @@ -830,6 +841,8 @@ NonTypeTemplateParmDecl::CreateDeserialized(ASTContext &C, GlobalDeclID ID,
NonTypeTemplateParmDecl(nullptr, SourceLocation(), SourceLocation(),
0, 0, nullptr, QualType(), nullptr, {}, {});
NTTP->NumExpandedTypes = NumExpandedTypes;
if (HasTypeConstraint)
NTTP->setPlaceholderTypeConstraint(nullptr);
return NTTP;
}

Expand Down
8 changes: 4 additions & 4 deletions clang/lib/Serialization/ASTWriterDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2016,17 +2016,17 @@ void ASTDeclWriter::VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
// For an expanded parameter pack, record the number of expansion types here
// so that it's easier for deserialization to allocate the right amount of
// memory.
Expr *TypeConstraint = D->getPlaceholderTypeConstraint();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes here are all NFC, right? Or am I missing a purpose to this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference: D->hasPlaceholderTypeConstraint() may be true, but D->getPlaceholderTypeConstraint may return nullptr.
IIUC, before this patch there was a underlying assumption that the former is true if and only if the latter is not nullptr.

This is not aligned when there are parsing errors.

See my comment here.

Record.push_back(!!TypeConstraint);
Record.push_back(D->hasPlaceholderTypeConstraint());
if (D->isExpandedParameterPack())
Record.push_back(D->getNumExpansionTypes());

VisitDeclaratorDecl(D);
// TemplateParmPosition.
Record.push_back(D->getDepth());
Record.push_back(D->getPosition());
if (TypeConstraint)
Record.AddStmt(TypeConstraint);

if (D->hasPlaceholderTypeConstraint())
Record.AddStmt(D->getPlaceholderTypeConstraint());

if (D->isExpandedParameterPack()) {
for (unsigned I = 0, N = D->getNumExpansionTypes(); I != N; ++I) {
Expand Down
55 changes: 55 additions & 0 deletions clang/test/Modules/malformed-constraint-template-non-type-parm-decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// RUN: rm -rf %t
// RUN: split-file %s %t
// RUN: cd %t

// RUN: %clang_cc1 -std=c++20 mod.cppm -emit-module-interface -o mod.pcm -fallow-pcm-with-compiler-errors -verify
// RUN: %clang_cc1 -std=c++20 main.cpp -fmodule-file=mod=mod.pcm -verify -fallow-pcm-with-compiler-errors -fsyntax-only -ast-dump-all | FileCheck %s
Comment on lines +5 to +6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this also manifests with just pch serialization - it would always be great if we can simplify the test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't even need serialization. IIRC it can be triggered with an ast dump, but only if the uninitialized value happens not to be a 0, which is the tricky part to trigger (not using sanitizers, that is).


// RUN: %clang_cc1 -std=c++20 mod.cppm -emit-reduced-module-interface -o mod.pcm -fallow-pcm-with-compiler-errors -verify
// RUN: %clang_cc1 -std=c++20 main.cpp -fmodule-file=mod=mod.pcm -verify -fallow-pcm-with-compiler-errors -fsyntax-only -ast-dump-all | FileCheck %s

//--- mod.cppm
export module mod;

template <typename T, auto Q>
concept ReferenceOf = Q;

// expected-error@+2 {{unknown type name 'AngleIsInvalidNow'}}
// expected-error@+1 {{constexpr variable 'angle' must be initialized by a constant expression}}
constexpr struct angle {AngleIsInvalidNow e;} angle;

// expected-error@+1 {{non-type template argument is not a constant expression}}
template<ReferenceOf<angle> auto R, typename Rep> requires requires(Rep v) {cos(v);}
auto cos(const Rep& q);

// expected-error@+1 {{non-type template argument is not a constant expression}}
template<ReferenceOf<angle> auto R, typename Rep> requires requires(Rep v) {tan(v);}
auto tan(const Rep& q);

//--- main.cpp
// expected-no-diagnostics
import mod;

// CHECK: |-FunctionTemplateDecl {{.*}} <line:11:1, line:12:22> col:6 imported in mod hidden invalid cos
// CHECK-NEXT: | |-NonTypeTemplateParmDecl {{.*}} <line:11:10, col:34> col:34 imported in mod hidden referenced invalid 'ReferenceOf<angle> auto' depth 0 index 0 R
// CHECK-NEXT: | |-TemplateTypeParmDecl {{.*}} <col:37, col:46> col:46 imported in mod hidden referenced typename depth 0 index 1 Rep
// CHECK-NEXT: | |-RequiresExpr {{.*}} <col:60, col:84> 'bool'
// CHECK-NEXT: | | |-ParmVarDecl {{.*}} <col:69, col:73> col:73 imported in mod hidden referenced v 'Rep'
// CHECK-NEXT: | | `-SimpleRequirement {{.*}} dependent
// CHECK-NEXT: | | `-CallExpr {{.*}} <col:77, col:82> '<dependent type>'
// CHECK-NEXT: | | |-UnresolvedLookupExpr {{.*}} <col:77> '<overloaded function type>' lvalue (ADL) = 'cos' empty
// CHECK-NEXT: | | `-DeclRefExpr {{.*}} <col:81> 'Rep' lvalue ParmVar {{.*}} 'v' 'Rep' non_odr_use_unevaluated
// CHECK-NEXT: | `-FunctionDecl {{.*}} <line:12:1, col:22> col:6 imported in mod hidden cos 'auto (const Rep &)'
// CHECK-NEXT: | `-ParmVarDecl {{.*}} <col:10, col:21> col:21 imported in mod hidden q 'const Rep &'

// CHECK: |-FunctionTemplateDecl {{.*}} <line:15:1, line:16:22> col:6 imported in mod hidden invalid tan
// CHECK-NEXT: | |-NonTypeTemplateParmDecl {{.*}} <line:15:10, col:34> col:34 imported in mod hidden referenced invalid 'ReferenceOf<angle> auto' depth 0 index 0 R
// CHECK-NEXT: | |-TemplateTypeParmDecl {{.*}} <col:37, col:46> col:46 imported in mod hidden referenced typename depth 0 index 1 Rep
// CHECK-NEXT: | |-RequiresExpr {{.*}} <col:60, col:84> 'bool'
// CHECK-NEXT: | | |-ParmVarDecl {{.*}} <col:69, col:73> col:73 imported in mod hidden referenced v 'Rep'
// CHECK-NEXT: | | `-SimpleRequirement {{.*}} dependent
// CHECK-NEXT: | | `-CallExpr {{.*}} <col:77, col:82> '<dependent type>'
// CHECK-NEXT: | | |-UnresolvedLookupExpr {{.*}} <col:77> '<overloaded function type>' lvalue (ADL) = 'tan' empty
// CHECK-NEXT: | | `-DeclRefExpr {{.*}} <col:81> 'Rep' lvalue ParmVar {{.*}} 'v' 'Rep' non_odr_use_unevaluated
// CHECK-NEXT: | `-FunctionDecl {{.*}} <line:16:1, col:22> col:6 imported in mod hidden tan 'auto (const Rep &)'
// CHECK-NEXT: | `-ParmVarDecl {{.*}} <col:10, col:21> col:21 imported in mod hidden q 'const Rep &'
Comment on lines +33 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please filter out line numbers on the AST dump match, otherwise this makes these tests hard to update.