Skip to content

Commit 56dfef9

Browse files
hnrklssnhnrklssn
committed
address PR comments
1 parent f680d86 commit 56dfef9

File tree

2 files changed

+119
-66
lines changed

2 files changed

+119
-66
lines changed

‎clang/lib/Sema/SemaDeclAttr.cpp

Lines changed: 68 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "clang/AST/ExprCXX.h"
2525
#include "clang/AST/Mangle.h"
2626
#include "clang/AST/RecursiveASTVisitor.h"
27+
#include "clang/AST/StmtVisitor.h"
2728
#include "clang/AST/Type.h"
2829
#include "clang/AST/TypeVisitor.h"
2930
#include "clang/Basic/CharInfo.h"
@@ -6160,13 +6161,12 @@ getBoundsAttrKind(AttributeCommonInfo::Kind Kind) {
61606161
}
61616162

61626163
class EarlyLifetimeAndScopeCheck
6163-
: public RecursiveASTVisitor<EarlyLifetimeAndScopeCheck> {
6164+
: public StmtVisitor<EarlyLifetimeAndScopeCheck, bool> {
61646165
Sema &SemaRef;
61656166
bool ScopeCheck;
61666167
Sema::LifetimeCheckKind LifetimeCheck;
61676168
BoundsAttributedType::BoundsAttrKind AttrKind;
61686169
bool IsArrayType;
6169-
bool HadError = false;
61706170
llvm::SmallPtrSet<Decl *, 16> Visited;
61716171

61726172
public:
@@ -6175,30 +6175,32 @@ class EarlyLifetimeAndScopeCheck
61756175
: SemaRef(S), ScopeCheck(SC), LifetimeCheck(LC),
61766176
AttrKind(getBoundsAttrKind(AK)), IsArrayType(IsArray) {}
61776177

6178+
bool VisitStmt(Stmt *S) {
6179+
bool HadError = false;
6180+
for (auto Child : S->children())
6181+
HadError |= Visit(Child);
6182+
return HadError;
6183+
}
6184+
61786185
bool VisitDeclRefExpr(DeclRefExpr *E) {
61796186
ValueDecl *VD = E->getDecl();
61806187
bool IsNewVD = Visited.insert(VD).second;
6181-
if (IsNewVD) {
6182-
HadError |=
6183-
CheckArgLifetimeAndScope(SemaRef, VD, E->getExprLoc(), ScopeCheck,
6184-
LifetimeCheck, AttrKind, IsArrayType);
6185-
}
6186-
return true;
6188+
return IsNewVD &&
6189+
CheckArgLifetimeAndScope(SemaRef, VD, E->getExprLoc(), ScopeCheck,
6190+
LifetimeCheck, AttrKind, IsArrayType);
61876191
}
61886192

61896193
bool VisitMemberExpr(MemberExpr *E) {
6190-
TraverseStmt(E->getBase());
6194+
bool HadError = Visit(E->getBase());
61916195
ValueDecl *VD = E->getMemberDecl();
61926196
bool IsNewVD = Visited.insert(VD).second;
61936197
if (IsNewVD) {
61946198
HadError |=
61956199
CheckArgLifetimeAndScope(SemaRef, VD, E->getExprLoc(), ScopeCheck,
61966200
LifetimeCheck, AttrKind, IsArrayType);
61976201
}
6198-
return true;
6202+
return HadError;
61996203
}
6200-
6201-
bool hadError() { return HadError; }
62026204
};
62036205

62046206
static bool diagnoseBoundsAttrLifetimeAndScope(
@@ -6443,13 +6445,24 @@ diagnoseRangeDependentDecls(Sema &S, const ValueDecl *TheDepender,
64436445
return HadError;
64446446
}
64456447

6448+
namespace {
64466449
class DynamicBoundsAttrInfo {
64476450
public:
6451+
TypedefNameDecl *TND;
6452+
ValueDecl *VD;
6453+
VarDecl *Var;
6454+
QualType DeclTy;
64486455
QualType Ty;
64496456
unsigned EffectiveLevel;
64506457
bool IsFPtr;
6458+
Sema::LifetimeCheckKind LifetimeCheck = Sema::LifetimeCheckKind::None;
6459+
bool ScopeCheck;
64516460

6452-
DynamicBoundsAttrInfo(QualType DeclTy, unsigned Level) {
6461+
DynamicBoundsAttrInfo(Decl *D, unsigned Level) {
6462+
TND = dyn_cast<TypedefNameDecl>(D);
6463+
VD = dyn_cast<ValueDecl>(D);
6464+
Var = dyn_cast<VarDecl>(D);
6465+
DeclTy = TND ? TND->getUnderlyingType() : VD->getType();
64536466
IsFPtr = false;
64546467
EffectiveLevel = Level;
64556468
Ty = DeclTy;
@@ -6463,8 +6476,12 @@ class DynamicBoundsAttrInfo {
64636476
break;
64646477
}
64656478
}
6479+
if (!IsFPtr)
6480+
LifetimeCheck = Sema::getLifetimeCheckKind(Var);
6481+
ScopeCheck = (Var && Var->isLocalVarDecl()) || IsFPtr;
64666482
}
64676483
};
6484+
} // namespace
64686485

64696486
void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
64706487
AttributeCommonInfo::Kind Kind,
@@ -6480,15 +6497,10 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
64806497
if (D->isInvalidDecl())
64816498
return;
64826499

6483-
auto *TND = dyn_cast<TypedefNameDecl>(D);
6484-
auto *VD = dyn_cast<ValueDecl>(D);
6485-
auto *Var = dyn_cast<VarDecl>(D);
6486-
QualType DeclTy = TND ? TND->getUnderlyingType() : VD->getType();
6487-
6488-
DynamicBoundsAttrInfo Info(DeclTy, Level);
6500+
DynamicBoundsAttrInfo Info(D, Level);
64896501

64906502
// Don't allow typedefs with __counted_by on non-function types.
6491-
if (TND && (!DeclTy->isFunctionType() && !Info.IsFPtr)) {
6503+
if (Info.TND && (!Info.DeclTy->isFunctionType() && !Info.IsFPtr)) {
64926504
Diag(Loc, diag::err_bounds_safety_typedef_dynamic_bound) << DiagName;
64936505
return;
64946506
}
@@ -6519,7 +6531,8 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
65196531
if (!IsEndedBy) {
65206532
// Nullability as indicated by _Nonnull or _Nullable. Does not impact
65216533
// semantics, only warnings.
6522-
std::optional<NullabilityKind> AttrNullability = DeclTy->getNullability();
6534+
std::optional<NullabilityKind> AttrNullability =
6535+
Info.DeclTy->getNullability();
65236536
if (OrNull) {
65246537
// Function parameter/return value attribute that *does* impact semantics,
65256538
// letting the compiler elide null checks. This could remove bounds safety
@@ -6554,16 +6567,16 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
65546567
}
65556568
}
65566569

6557-
if (VD) {
6558-
const auto *FD = dyn_cast<FieldDecl>(VD);
6570+
if (Info.VD) {
6571+
const auto *FD = dyn_cast<FieldDecl>(Info.VD);
65596572
if (FD && FD->getParent()->isUnion()) {
65606573
Diag(Loc, diag::err_invalid_decl_kind_bounds_safety_union_count)
65616574
<< DiagName;
65626575
return;
65636576
}
65646577

65656578
if (Info.EffectiveLevel != 0 &&
6566-
(!isa<ParmVarDecl>(VD) || DeclTy->isBoundsAttributedType())) {
6579+
(!isa<ParmVarDecl>(Info.VD) || Info.DeclTy->isBoundsAttributedType())) {
65676580
Diag(Loc, diag::err_bounds_safety_nested_dynamic_bound) << DiagName;
65686581
return;
65696582
}
@@ -6574,7 +6587,7 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
65746587
// case of ConstructCountAttributedType, which complains that the type
65756588
// has two count attributes. See if we can produce a better diagnostic here
65766589
// instead.
6577-
if (const auto *PVD = dyn_cast_or_null<ParmVarDecl>(Var)) {
6590+
if (const auto *PVD = dyn_cast_or_null<ParmVarDecl>(Info.Var)) {
65786591
QualType TSITy = PVD->getTypeSourceInfo()->getType();
65796592
if (IsEndedBy) {
65806593
if (Level == 0 && TSITy->isArrayType()) {
@@ -6592,7 +6605,8 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
65926605
}
65936606

65946607
if (Info.Ty->isArrayType() && OrNull &&
6595-
(FD || Info.EffectiveLevel > 0 || (Var && Var->hasExternalStorage()))) {
6608+
(FD || Info.EffectiveLevel > 0 ||
6609+
(Info.Var && Info.Var->hasExternalStorage()))) {
65966610
auto ErrDiag = Diag(Loc, diag::err_bounds_safety_nullable_fam);
65976611
// Pointers to dynamic count types are only allowed for parameters, so any
65986612
// FieldDecl containing a dynamic count type is a FAM. I.e. a struct field
@@ -6622,7 +6636,6 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
66226636
}
66236637

66246638
QualType NewDeclTy{};
6625-
bool ScopeCheck = (Var && Var->isLocalVarDecl()) || Info.IsFPtr;
66266639
const BoundsAttributedType *ConstructedType = nullptr;
66276640

66286641
bool HadAtomicError = false;
@@ -6641,22 +6654,22 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
66416654
// Make started_by() pointers if VD is a field or variable. We don't want to
66426655
// create started_by(X) pointers where X is a function etc.
66436656
std::optional<TypeCoupledDeclRefInfo> StartPtrInfo;
6644-
if (VD && (isa<FieldDecl>(VD) || isa<VarDecl>(VD))) {
6657+
if (Info.VD && (isa<FieldDecl>(Info.VD) || isa<VarDecl>(Info.VD))) {
66456658
assert(Level <= 1);
6646-
StartPtrInfo = TypeCoupledDeclRefInfo(VD, /*Deref=*/Level != 0);
6659+
StartPtrInfo = TypeCoupledDeclRefInfo(Info.VD, /*Deref=*/Level != 0);
66476660
}
66486661

66496662
auto TypeConstructor = ConstructDynamicRangePointerType(
6650-
*this, Level, DiagName, AttrArg, Loc, OriginatesInAPINotes, ScopeCheck,
6651-
StartPtrInfo);
6652-
NewDeclTy = TypeConstructor.Visit(DeclTy);
6663+
*this, Level, DiagName, AttrArg, Loc, OriginatesInAPINotes,
6664+
Info.ScopeCheck, StartPtrInfo);
6665+
NewDeclTy = TypeConstructor.Visit(Info.DeclTy);
66536666
HadAtomicError = TypeConstructor.hadAtomicError();
66546667
ConstructedType = TypeConstructor.getConstructedType();
66556668
} else {
66566669
auto TypeConstructor = ConstructCountAttributedType(
66576670
*this, Level, DiagName, AttrArg, Loc, CountInBytes, OrNull,
6658-
OriginatesInAPINotes, ScopeCheck);
6659-
NewDeclTy = TypeConstructor.Visit(DeclTy);
6671+
OriginatesInAPINotes, Info.ScopeCheck);
6672+
NewDeclTy = TypeConstructor.Visit(Info.DeclTy);
66606673
HadAtomicError = TypeConstructor.hadAtomicError();
66616674
ConstructedType = TypeConstructor.getConstructedType();
66626675
}
@@ -6670,44 +6683,43 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
66706683
// nested type, so `ConstructedType` can be different from `NewDeclTy`.
66716684
assert(ConstructedType);
66726685

6673-
auto LifetimeCheck = Info.IsFPtr ? Sema::LifetimeCheckKind::None
6674-
: Sema::getLifetimeCheckKind(Var);
66756686
// Scope information is not available after template instantiation, so this
66766687
// check has been performed earlier if this is a template instantiation.
66776688
if (!InInstantiatedTemplate &&
6678-
diagnoseBoundsAttrLifetimeAndScope(*this, ConstructedType, ScopeCheck,
6679-
LifetimeCheck))
6689+
diagnoseBoundsAttrLifetimeAndScope(*this, ConstructedType,
6690+
Info.ScopeCheck, Info.LifetimeCheck))
66806691
return;
66816692

6682-
if (VD && !isa<FunctionDecl>(VD) && !HadAtomicError) {
6693+
if (Info.VD && !isa<FunctionDecl>(Info.VD) && !HadAtomicError) {
66836694
if (const auto *BDTy = dyn_cast<CountAttributedType>(ConstructedType)) {
6684-
if (!diagnoseCountDependentDecls(*this, VD, BDTy, Info.EffectiveLevel,
6685-
Info.IsFPtr))
6686-
AttachDependerDeclsAttr(VD, BDTy, Info.EffectiveLevel);
6695+
if (!diagnoseCountDependentDecls(*this, Info.VD, BDTy,
6696+
Info.EffectiveLevel, Info.IsFPtr))
6697+
AttachDependerDeclsAttr(Info.VD, BDTy, Info.EffectiveLevel);
66876698
} else if (const auto *BDTy =
66886699
dyn_cast<DynamicRangePointerType>(ConstructedType)) {
6689-
diagnoseRangeDependentDecls(*this, VD, BDTy, Info.EffectiveLevel,
6700+
diagnoseRangeDependentDecls(*this, Info.VD, BDTy, Info.EffectiveLevel,
66906701
Info.IsFPtr);
66916702
} else {
66926703
llvm_unreachable("Unexpected bounds attributed type");
66936704
}
66946705
}
66956706

6696-
if (TND) {
6707+
if (Info.TND) {
66976708
// We need to create a TypeSourceInfo, as TypedefNameDecl is not a ValueDecl
66986709
// and thus doesn't have setType() method.
6699-
SourceLocation Loc = TND->getTypeSourceInfo()->getTypeLoc().getBeginLoc();
6700-
TypeSourceInfo *Info = Context.getTrivialTypeSourceInfo(NewDeclTy, Loc);
6701-
TND->setTypeSourceInfo(Info);
6710+
SourceLocation Loc =
6711+
Info.TND->getTypeSourceInfo()->getTypeLoc().getBeginLoc();
6712+
TypeSourceInfo *TSI = Context.getTrivialTypeSourceInfo(NewDeclTy, Loc);
6713+
Info.TND->setTypeSourceInfo(TSI);
67026714
} else {
6703-
VD->setType(NewDeclTy);
6715+
Info.VD->setType(NewDeclTy);
67046716
// Reconstruct implicit cast for initializer after variable type change.
6705-
if (Var && Var->hasInit()) {
6706-
Expr *Init = Var->getInit();
6717+
if (Info.Var && Info.Var->hasInit()) {
6718+
Expr *Init = Info.Var->getInit();
67076719
ExprResult Res = removeUnusedOVEs(*this, Init, Init->IgnoreImpCasts());
67086720
if (Res.isInvalid())
67096721
return;
6710-
AddInitializerToDecl(Var, Res.get(), Var->isDirectInit());
6722+
AddInitializerToDecl(Info.Var, Res.get(), Info.Var->isDirectInit());
67116723
}
67126724
}
67136725
}
@@ -6740,21 +6752,14 @@ static void handlePtrCountedByEndedByAttr(Sema &S, Decl *D,
67406752
return;
67416753

67426754
if (D->getDescribedTemplate() || S.CurContext->isDependentContext()) {
6743-
auto *TND = dyn_cast<TypedefNameDecl>(D);
6744-
auto *VD = dyn_cast<ValueDecl>(D);
6745-
auto *Var = dyn_cast<VarDecl>(D);
6746-
QualType DeclTy = TND ? TND->getUnderlyingType() : VD->getType();
6747-
DynamicBoundsAttrInfo Info(DeclTy, Level);
6755+
DynamicBoundsAttrInfo Info(D, Level);
67486756
// Scope information will be invalid by the time we instantiate the
67496757
// template, so perform these checks now and delay the rest of the
67506758
// processing until the point of instantiation.
6751-
auto LifetimeCheck = Info.IsFPtr ? Sema::LifetimeCheckKind::None
6752-
: Sema::getLifetimeCheckKind(Var);
6753-
bool ScopeCheck = (Var && Var->isLocalVarDecl()) || Info.IsFPtr;
6754-
EarlyLifetimeAndScopeCheck EarlyCheck(S, ScopeCheck, LifetimeCheck,
6755-
AL.getKind(), Info.Ty->isArrayType());
6756-
EarlyCheck.TraverseStmt(AL.getArgAsExpr(0));
6757-
if (EarlyCheck.hadError())
6759+
EarlyLifetimeAndScopeCheck EarlyCheck(S, Info.ScopeCheck,
6760+
Info.LifetimeCheck, AL.getKind(),
6761+
Info.Ty->isArrayType());
6762+
if (EarlyCheck.Visit(AL.getArgAsExpr(0)))
67586763
return;
67596764
attachLateInstantiatedCountedByEndedByAttr(S, D, AL, Level);
67606765
return;

‎clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-argument.cpp

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ void cb_cchar_42(const char *__counted_by(42) s);
7171
// expected-note@+1 19{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'count' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
7272
void cb_int(int *__counted_by(count) p, size_t count);
7373

74-
// expected-note@+1 11{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'count' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
74+
// expected-note@+1 12{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'count' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
7575
void cb_cint(const int *__counted_by(count) p, size_t count);
7676

7777
// expected-note@+1 10{{consider using 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
@@ -80,7 +80,7 @@ void cb_cint_42(const int *__counted_by(42) p);
8080
// expected-note@+1 6{{consider using 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
8181
void cb_cint_multi(const int *__counted_by((a + b) * (c - d)) p, int a, int b, int c, int d);
8282

83-
// expected-note@+1 11{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'size' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
83+
// expected-note@+1 13{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'size' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
8484
void sb_cvoid(const void *__sized_by(size) p, size_t size);
8585

8686
// expected-note@+1 {{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'size' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
@@ -89,7 +89,7 @@ void sb_cchar(const char *__sized_by(size) p, size_t size);
8989
// expected-note@+1 5{{consider using 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
9090
void sb_cvoid_42(const void *__sized_by(42) p);
9191

92-
// expected-note@+1 5{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'size' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
92+
// expected-note@+1 6{{consider using a safe container and passing '.data()' to the parameter 'p' and '.size()' to its dependent parameter 'size' or 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
9393
void sb_cint(const int *__sized_by(size) p, size_t size);
9494

9595
// expected-note@+1 3{{consider using 'std::span' and passing '.first(...).data()' to the parameter 'p'}}
@@ -161,6 +161,54 @@ void pointers_of_diff_type(std::array<int, 42> &a, std::string &s,
161161
sb_cvoid(sp_char.data(), sp_char.size_bytes());
162162
}
163163

164+
template <typename IntT, typename CharT>
165+
void pointers_of_diff_type_uninstantiated(std::array<IntT, 42> &a, std::string &s,
166+
std::span<IntT> sp_int, std::span<CharT> sp_char) {
167+
cb_cint(a.data(), a.size());
168+
sb_cvoid(a.data(), a.size());
169+
170+
cb_cchar(s.data(), s.size());
171+
sb_cvoid(s.data(), s.size());
172+
173+
cb_cint(sp_int.data(), sp_int.size());
174+
cb_cint(sp_int.data(), sp_int.size_bytes());
175+
sb_cvoid(sp_int.data(), sp_int.size());
176+
sb_cvoid(sp_int.data(), sp_int.size_bytes());
177+
sb_cint(sp_int.data(), sp_int.size());
178+
sb_cint(sp_int.data(), sp_int.size_bytes());
179+
180+
cb_cchar(sp_char.data(), sp_char.size());
181+
cb_cchar(sp_char.data(), sp_char.size_bytes());
182+
sb_cvoid(sp_char.data(), sp_char.size());
183+
sb_cvoid(sp_char.data(), sp_char.size_bytes());
184+
}
185+
186+
template <typename IntT, typename CharT>
187+
void pointers_of_diff_type_instantiated(std::array<IntT, 42> &a, std::string &s,
188+
std::span<IntT> sp_int, std::span<CharT> sp_char) {
189+
cb_cint(a.data(), a.size());
190+
sb_cvoid(a.data(), a.size()); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
191+
192+
cb_cchar(s.data(), s.size());
193+
sb_cvoid(s.data(), s.size());
194+
195+
cb_cint(sp_int.data(), sp_int.size());
196+
cb_cint(sp_int.data(), sp_int.size_bytes()); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
197+
sb_cvoid(sp_int.data(), sp_int.size()); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
198+
sb_cvoid(sp_int.data(), sp_int.size_bytes());
199+
sb_cint(sp_int.data(), sp_int.size()); // expected-warning{{unsafe assignment to function parameter of count-attributed type}}
200+
sb_cint(sp_int.data(), sp_int.size_bytes());
201+
202+
cb_cchar(sp_char.data(), sp_char.size());
203+
cb_cchar(sp_char.data(), sp_char.size_bytes());
204+
sb_cvoid(sp_char.data(), sp_char.size());
205+
sb_cvoid(sp_char.data(), sp_char.size_bytes());
206+
}
207+
208+
// explicitly instantiate the templated function to trigger processing of bounds safety attributes
209+
template void pointers_of_diff_type_instantiated<int, char>(std::array<int, 42> &a, std::string &s,
210+
std::span<int> sp_int, std::span<char> sp_char);
211+
164212
// Check if passing to __counted_by(const) uses a subview.
165213

166214
void cb_const_subview(std::string &s, std::span<int> sp) {

0 commit comments

Comments
 (0)