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 1 commit
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
18 changes: 16 additions & 2 deletions clang/lib/Sema/SemaTemplate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,15 +1235,24 @@ bool Sema::AttachTypeConstraint(AutoTypeLoc TL,
<< NewConstrainedParm->getTypeSourceInfo()
->getTypeLoc()
.getSourceRange();
NewConstrainedParm->setPlaceholderTypeConstraint(
RecoveryExpr::Create(Context, OrigConstrainedParm->getType(),
OrigConstrainedParm->getBeginLoc(),
OrigConstrainedParm->getEndLoc(), {}));
return true;
}
// FIXME: Concepts: This should be the type of the placeholder, but this is
// unclear in the wording right now.
DeclRefExpr *Ref =
BuildDeclRefExpr(OrigConstrainedParm, OrigConstrainedParm->getType(),
VK_PRValue, OrigConstrainedParm->getLocation());
if (!Ref)
if (!Ref) {
Copy link
Contributor

@cor3ntin cor3ntin Jan 6, 2025

Choose a reason for hiding this comment

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

Can that actually fail? I wonder if we should not just assert

Copy link
Contributor Author

@alejandro-alvarez-sonarsource alejandro-alvarez-sonarsource Jan 6, 2025

Choose a reason for hiding this comment

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

I took the existing if, but looking at BuildDeclRefExpr it doesn't seem like it could return nullptr.

I have dropped the condition and added an assert instead.

Copy link
Contributor

@zyn0217 zyn0217 Jan 6, 2025

Choose a reason for hiding this comment

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

+1, BuildDeclRefExpr wouldn't fail with a nullptr

NewConstrainedParm->setPlaceholderTypeConstraint(
RecoveryExpr::Create(Context, OrigConstrainedParm->getType(),
OrigConstrainedParm->getBeginLoc(),
OrigConstrainedParm->getEndLoc(), {}));
Copy link
Contributor

@cor3ntin cor3ntin Jan 6, 2025

Choose a reason for hiding this comment

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

I think just setting it to nullptr would be sufficient? (maybe with a comment)

Copy link
Contributor Author

@alejandro-alvarez-sonarsource alejandro-alvarez-sonarsource Jan 6, 2025

Choose a reason for hiding this comment

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

To avoid the crash it should. For our use case we prefer a RecoveryExpr because it is obvious from the AST that something went amiss, so if it is not a blocker, I'd prefer to keep it.

Copy link
Contributor

@cor3ntin cor3ntin Jan 6, 2025

Choose a reason for hiding this comment

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

After chatting with @erichkeane, the recovery expression is probably fine.
However maybe we could find a way (lambda?) to avoid the repetition here and line 1263

Copy link
Contributor Author

@alejandro-alvarez-sonarsource alejandro-alvarez-sonarsource Jan 6, 2025

Choose a reason for hiding this comment

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

Refactored into a lambda so there is only one place to handle failures and introduce the RecoveryExpr.

return true;
}
ExprResult ImmediatelyDeclaredConstraint = formImmediatelyDeclaredConstraint(
*this, TL.getNestedNameSpecifierLoc(), TL.getConceptNameInfo(),
TL.getNamedConcept(), /*FoundDecl=*/TL.getFoundDecl(), TL.getLAngleLoc(),
Expand All @@ -1255,8 +1264,13 @@ bool Sema::AttachTypeConstraint(AutoTypeLoc TL,
},
EllipsisLoc);
if (ImmediatelyDeclaredConstraint.isInvalid() ||
!ImmediatelyDeclaredConstraint.isUsable())
!ImmediatelyDeclaredConstraint.isUsable()) {
NewConstrainedParm->setPlaceholderTypeConstraint(
RecoveryExpr::Create(Context, OrigConstrainedParm->getType(),
Copy link
Contributor

@mizvekov mizvekov Jan 7, 2025

Choose a reason for hiding this comment

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

It's not immediately clear to me, nor explained in the commit message, what this RecoveryExpr is expected to buy, versus always initializing to nullptr, which is the status quo (most of the time, by accident).

Copy link
Collaborator

@erichkeane erichkeane Jan 7, 2025

Choose a reason for hiding this comment

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

I'll take a stab at it:

In general, the point of a RecoveryExpr is to make it 'easier' for things after this to recover/handle this. It encodes the state of "something is here, potentially with a type". This is useful sometimes for diagnostics, though we don't do a great job of using them for that.

However, downstream "tooling" (and since the author is at SonarSource, I imagine that is his motivation) can often use this for better SA and diagnostics. This was the original 'motivation' I think, and it is a "it would be great if we always did this...", though we are super bad at it at the moment.

Copy link
Contributor

@mizvekov mizvekov Jan 7, 2025

Choose a reason for hiding this comment

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

Right, but the original bug was about fixing a crash-on-invalid, which was found during reduction of an unrelated bug. It seems this 'improvement' is being preempted, without an actual motivating case.
The RecoveryExpr doesn't even always have the correct type the expression would have, so it's not clear what kind of compromise would be achieved here.

I mostly say this because always initializing the placeholder pointer to null would have been a much simpler fix, which would potentially cover other instances of a similar bug.

Copy link
Contributor Author

@alejandro-alvarez-sonarsource alejandro-alvarez-sonarsource Jan 8, 2025

Choose a reason for hiding this comment

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

The reasons for the RecoveryExpr is, indeed, like @erichkeane said, to be able to identify on the AST places where things were missing during parsing.

For this particular use case, the frontend (an analyzers) can easily tell apart

  1. There is no type constraint (nullptr)
  2. There should be one but we could not parse it (RecoveryExpr)

The second point, IIUC the documentation, is one of the reasons to use RecoveryExpr. Option 1 fixes the crash but loses information that can be used for better diagnosis.

which was found during reduction of an unrelated bug

Not really, and I apologize if I have given this impression.
We have had few crashes and assertions when parsing mp-units with missing headers. This was not accidentally found because of #118288 (which is my other PR based on errors from this project).

I mostly say this because always initializing the placeholder pointer to null would have been a much simpler fix, which would potentially cover other instances of a similar bug.

I agree with you here, and the trailing object should probably be initialized to null on the constructor of NonTypeTemplateParmDecl to catch any other accidental uninitialized value. I am happy to do that too if no one objects.

Copy link
Contributor

@zyn0217 zyn0217 Jan 8, 2025

Choose a reason for hiding this comment

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

and the trailing object should probably be initialized to null on the constructor of NonTypeTemplateParmDecl to catch any other accidental uninitialized value.

We have a bit flag TypeConstraintInitialized in TemplateTypeParmDecl to represent a state of "having constraint attached but uninitialized (because of invalid constraint expression)". I think it'd be better to do the same thing in NonTypeTemplateParmDecl, or the reverse for consistency.

Copy link
Contributor Author

@alejandro-alvarez-sonarsource alejandro-alvarez-sonarsource Jan 9, 2025

Choose a reason for hiding this comment

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

Ok, I have followed a similar approach to #113182. There is no RecoveryExpr but invalid is set, which should work to easily identify something went mising.

Note that I had to patch AstWriterDecl and AstReaderDecl too: there is an extra boolean to flag the initialization of the underlying pointer.

I also changed

  Expr *TypeConstraint = D->getPlaceholderTypeConstraint();
  Record.push_back(!!TypeConstraint);

to

Record.push_back(D->hasPlaceholderTypeConstraint());

Which I think it is the intended meaning from looking at the reader

    bool HasTypeConstraint = Record.readInt();
    D = NonTypeTemplateParmDecl::CreateDeserialized(Context, ID,
                                                    HasTypeConstraint);
    break;

Otherwise there will be no space for the pointer on the trailing object, but hasPlaceholderTypeConstraint would be returning true and that looks inconsistent to me, even if (I imagine) it is unlikely setPlaceholderTypeConstraint would be called on a deserialized NonTypeTemplateParmDecl

OrigConstrainedParm->getBeginLoc(),
OrigConstrainedParm->getEndLoc(), {}));
Copy link
Contributor

@cor3ntin cor3ntin Jan 6, 2025

Choose a reason for hiding this comment

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

Same here

return true;
}

NewConstrainedParm->setPlaceholderTypeConstraint(
ImmediatelyDeclaredConstraint.get());
Expand Down
54 changes: 54 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,54 @@
// 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

@zyn0217 zyn0217 Jan 6, 2025

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

@alejandro-alvarez-sonarsource alejandro-alvarez-sonarsource Jan 6, 2025

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).


//--- 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: | | `-RecoveryExpr {{.*}} <col:10, col:34> 'ReferenceOf<angle> auto' contains-errors lvalue
// 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: | | `-RecoveryExpr {{.*}} <col:10, col:34> 'ReferenceOf<angle> auto' contains-errors lvalue
// 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 &'
Loading