[clang][Sema] Fix initialization of NonTypeTemplateParmDecl...
#121768
Conversation
...when there are invalid constraints. When attaching a `TypeConstraint`, in case of error, the trailing pointer that is supposed to point to the constraint is left uninitialized. Sometimes the uninitialized value will be a `nullptr`, but at other times it will not. If we traverse the AST (for instance, dumping it, or when writing the BMI), we may get a crash depending on the value that was left. The serialization may also contain a bogus value. With this commit, we always initialize this trailing pointer, using a `RecoveryExpr` in case of failure to parse.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource) Changes...when there are invalid constraints. When attaching a With this commit, we always initialize this trailing pointer, using a This does not affect only modules, but it causes a segfault more consistently when they are involved. The test case was reduced from mp-units. Full diff: https://github.com/llvm/llvm-project/pull/121768.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 20ec2fbeaa6a8b..9b51f973fb2bb8 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -1235,6 +1235,10 @@ 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
@@ -1242,8 +1246,13 @@ bool Sema::AttachTypeConstraint(AutoTypeLoc TL,
DeclRefExpr *Ref =
BuildDeclRefExpr(OrigConstrainedParm, OrigConstrainedParm->getType(),
VK_PRValue, OrigConstrainedParm->getLocation());
- if (!Ref)
+ if (!Ref) {
+ NewConstrainedParm->setPlaceholderTypeConstraint(
+ RecoveryExpr::Create(Context, OrigConstrainedParm->getType(),
+ OrigConstrainedParm->getBeginLoc(),
+ OrigConstrainedParm->getEndLoc(), {}));
return true;
+ }
ExprResult ImmediatelyDeclaredConstraint = formImmediatelyDeclaredConstraint(
*this, TL.getNestedNameSpecifierLoc(), TL.getConceptNameInfo(),
TL.getNamedConcept(), /*FoundDecl=*/TL.getFoundDecl(), TL.getLAngleLoc(),
@@ -1255,8 +1264,13 @@ bool Sema::AttachTypeConstraint(AutoTypeLoc TL,
},
EllipsisLoc);
if (ImmediatelyDeclaredConstraint.isInvalid() ||
- !ImmediatelyDeclaredConstraint.isUsable())
+ !ImmediatelyDeclaredConstraint.isUsable()) {
+ NewConstrainedParm->setPlaceholderTypeConstraint(
+ RecoveryExpr::Create(Context, OrigConstrainedParm->getType(),
+ OrigConstrainedParm->getBeginLoc(),
+ OrigConstrainedParm->getEndLoc(), {}));
return true;
+ }
NewConstrainedParm->setPlaceholderTypeConstraint(
ImmediatelyDeclaredConstraint.get());
diff --git a/clang/test/Modules/malformed-constraint-template-non-type-parm-decl.cpp b/clang/test/Modules/malformed-constraint-template-non-type-parm-decl.cpp
new file mode 100644
index 00000000000000..cea6404bbebd28
--- /dev/null
+++ b/clang/test/Modules/malformed-constraint-template-non-type-parm-decl.cpp
@@ -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
+
+//--- 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 &'
|
clang/lib/Sema/SemaTemplate.cpp
Outdated
| 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) { |
There was a problem hiding this comment.
Can that actually fail? I wonder if we should not just assert
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
+1, BuildDeclRefExpr wouldn't fail with a nullptr
clang/lib/Sema/SemaTemplate.cpp
Outdated
| NewConstrainedParm->setPlaceholderTypeConstraint( | ||
| RecoveryExpr::Create(Context, OrigConstrainedParm->getType(), | ||
| OrigConstrainedParm->getBeginLoc(), | ||
| OrigConstrainedParm->getEndLoc(), {})); |
There was a problem hiding this comment.
I think just setting it to nullptr would be sufficient? (maybe with a comment)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Refactored into a lambda so there is only one place to handle failures and introduce the RecoveryExpr.
clang/lib/Sema/SemaTemplate.cpp
Outdated
| !ImmediatelyDeclaredConstraint.isUsable()) { | ||
| NewConstrainedParm->setPlaceholderTypeConstraint( | ||
| RecoveryExpr::Create(Context, OrigConstrainedParm->getType(), | ||
| OrigConstrainedParm->getBeginLoc(), | ||
| OrigConstrainedParm->getEndLoc(), {})); |
| // 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 |
There was a problem hiding this comment.
I'm wondering if this also manifests with just pch serialization - it would always be great if we can simplify the test
There was a problem hiding this comment.
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).
clang/lib/Sema/SemaTemplate.cpp
Outdated
| 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) { |
There was a problem hiding this comment.
+1, BuildDeclRefExpr wouldn't fail with a nullptr
clang/lib/Sema/SemaTemplate.cpp
Outdated
| !ImmediatelyDeclaredConstraint.isUsable()) | ||
| !ImmediatelyDeclaredConstraint.isUsable()) { | ||
| NewConstrainedParm->setPlaceholderTypeConstraint( | ||
| RecoveryExpr::Create(Context, OrigConstrainedParm->getType(), |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
- There is no type constraint (
nullptr) - 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Ping? |
| Expr *TypeConstraint = D->getPlaceholderTypeConstraint(); | ||
| Record.push_back(/*PlaceholderTypeConstraintInitialized=*/TypeConstraint != | ||
| nullptr); |
There was a problem hiding this comment.
Can we only serialize PlaceholderTypeConstraintInitialized only when D->hasPlaceholderTypeConstraint() so that we can save up a bit for a number of cases?
hasPlaceholderTypeConstraint is false
| } | ||
|
|
||
| void setPlaceholderTypeConstraint(Expr *E) { | ||
| *getTrailingObjects<Expr *>() = E; |
There was a problem hiding this comment.
While I like the assert-safety that we get from the PlaceholderTypeConstraintInitialized, I'm not sure it is worth the rigamarole. Why not just have the ctor do setPlaceholderTypeConstraint?
There was a problem hiding this comment.
Both inside Sema and ASTReaderDecl the NonTypeTemplateParmDecl is created relatively far from the parsing / deserialization of the constraint.
There was a problem hiding this comment.
I'm not complaining about the 'set' method, just that the concern is that it is uninitialized. I'm saying, why not just initialize it to nullptr? Or am I misunderstanding the concern.
There was a problem hiding this comment.
The flag would allow to discern between "was not initialized" and "was explicitly initialized to null".
Now, I have to admit I do not know if the latter is meaningful for a constrained NTTP.
There was a problem hiding this comment.
Ok, then we're on the same page. I do not see a difference between "explicitly null" and "not initialized" here. I think that is a differentiation without a cause.
|
Also, can you please update the commit message? it isn't accurate anymore, re |
|
Ping? |
clang/lib/AST/DeclTemplate.cpp
Outdated
| AT && AT->isConstrained() ? 1 : 0)) | ||
| NonTypeTemplateParmDecl(DC, StartLoc, IdLoc, D, P, Id, T, ParameterPack, | ||
| TInfo); | ||
| bool const HasConstraint = AT && AT->isConstrained(); |
There was a problem hiding this comment.
We prefer const bool afaik
There was a problem hiding this comment.
Thanks! Swapped the order of the cost. Would you mind doing the merge?
alejandro-alvarez-sonarsource
requested review from
cyndyishida,
rupprecht,
keith,
aaronmondal,
aaupov and
maksfb
as code owners
alejandro-alvarez-sonarsource
requested review from
rafaelauler,
ayermolo,
dcci,
yota9,
banach-space,
dcaballe,
nicolasvasilache,
Groverkss,
kuhar,
JDevlieghere,
nikic and
a team
as code owners
|
Sorry for the noise, it seems I messed up merging from main. Let me revert the mess. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
alejandro-alvarez-sonarsource
force-pushed
the
aa/fix_init_nttp
branch
from
4892b25 to
786d7bd
Compare
| @@ -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(); | |||
There was a problem hiding this comment.
Changes here are all NFC, right? Or am I missing a purpose to this change?
There was a problem hiding this comment.
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.
Co-authored-by: Erich Keane <[email protected]>
| // 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 &' |
There was a problem hiding this comment.
Please filter out line numbers on the AST dump match, otherwise this makes these tests hard to update.
...when there are invalid constraints.
When attaching a
TypeConstraint, in case of error, the trailing pointer that is supposed to point to the constraint is left uninitialized.Sometimes the uninitialized value will be a
nullptr, but at other times it will not. If we traverse the AST (for instance, dumping it, or when writing the BMI), we may get a crash depending on the value that was left. The serialization may also contain a bogus value.In this commit, we always initialize the
PlaceholderTypeConstraintwithnullptr, to avoid accessing this uninitialized memory.This does not affect only modules, but it causes a segfault more consistently when they are involved.
The test case was reduced from
mp-units.