[Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null #93231
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hnrklssn
requested review from
delcypher,
kees,
AaronBallman,
bwendling,
erichkeane,
rapidsna,
patrykstefanski and
apple-fcloutier
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Henrik G. Olsson (hnrklssn) ChangesThe attributes rdar://125400354 Patch is 91.46 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/93231.diff 30 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2f83f5c6d54e9..841ef07f2e99b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -425,6 +425,12 @@ Attribute Changes in Clang
size_t count;
};
+- The attributes ``sized_by``, ``counted_by_or_null`` and ``sized_by_or_null```
+ have been added as variants on ``counted_by``, each with slightly different semantics.
+ ``sized_by`` takes a byte size parameter instead of an element count, allowing pointees
+ with unknown size. The ``counted_by_or_null`` and ``sized_by_or_null`` variants are equivalent
+ to their base variants, except the pointer can be null regardless of count/size value.
+ If the pointer is null the size is effectively 0.
Improvements to Clang's diagnostics
-----------------------------------
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 7a7721239a28f..70877f0dd2ba8 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2236,6 +2236,36 @@ def CountedBy : DeclOrTypeAttr {
let LangOpts = [COnly];
}
+def CountedByOrNull : DeclOrTypeAttr {
+ let Spellings = [Clang<"counted_by_or_null">];
+ let Subjects = SubjectList<[Field], ErrorDiag>;
+ let Args = [ExprArgument<"Count">, IntArgument<"NestedLevel", 1>];
+ let LateParsed = LateAttrParseExperimentalExt;
+ let ParseArgumentsAsUnevaluated = 1;
+ let Documentation = [CountedByDocs];
+ let LangOpts = [COnly];
+}
+
+def SizedBy : DeclOrTypeAttr {
+ let Spellings = [Clang<"sized_by">];
+ let Subjects = SubjectList<[Field], ErrorDiag>;
+ let Args = [ExprArgument<"Size">, IntArgument<"NestedLevel", 1>];
+ let LateParsed = LateAttrParseExperimentalExt;
+ let ParseArgumentsAsUnevaluated = 1;
+ let Documentation = [CountedByDocs];
+ let LangOpts = [COnly];
+}
+
+def SizedByOrNull : DeclOrTypeAttr {
+ let Spellings = [Clang<"sized_by_or_null">];
+ let Subjects = SubjectList<[Field], ErrorDiag>;
+ let Args = [ExprArgument<"Size">, IntArgument<"NestedLevel", 1>];
+ let LateParsed = LateAttrParseExperimentalExt;
+ let ParseArgumentsAsUnevaluated = 1;
+ let Documentation = [CountedByDocs];
+ let LangOpts = [COnly];
+}
+
// This is a marker used to indicate that an __unsafe_unretained qualifier was
// ignored because ARC is not enabled. The usual representation for this
// qualifier is as an ObjCOwnership attribute with Kind == "none".
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8e6596410c5d0..93672ccfd59d9 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6532,27 +6532,27 @@ def warn_superclass_variable_sized_type_not_at_end : Warning<
" in superclass %3">, InGroup<ObjCFlexibleArray>;
def err_flexible_array_count_not_in_same_struct : Error<
- "'counted_by' field %0 isn't within the same struct as the flexible array">;
+ "'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}1' field %0 isn't within the same struct as the flexible array">;
def err_counted_by_attr_not_on_ptr_or_flexible_array_member : Error<
- "'counted_by' only applies to pointers or C99 flexible array members">;
+ "'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' only applies to pointers%select{ or C99 flexible array members|||}0">;
def err_counted_by_attr_on_array_not_flexible_array_member : Error<
"'counted_by' on arrays only applies to C99 flexible array members">;
def err_counted_by_attr_refer_to_itself : Error<
"'counted_by' cannot refer to the flexible array member %0">;
def err_counted_by_must_be_in_structure : Error<
- "field %0 in 'counted_by' not inside structure">;
+ "field %0 in '%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}1' not inside structure">;
def err_counted_by_attr_argument_not_integer : Error<
- "'counted_by' requires a non-boolean integer type argument">;
+ "'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' requires a non-boolean integer type argument">;
def err_counted_by_attr_only_support_simple_decl_reference : Error<
- "'counted_by' argument must be a simple declaration reference">;
+ "'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' argument must be a simple declaration reference">;
def err_counted_by_attr_in_union : Error<
- "'counted_by' cannot be applied to a union member">;
+ "'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' cannot be applied to a union member">;
def err_counted_by_attr_refer_to_union : Error<
- "'counted_by' argument cannot refer to a union member">;
+ "'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' argument cannot refer to a union member">;
def note_flexible_array_counted_by_attr_field : Note<
"field %0 declared here">;
def err_counted_by_attr_pointee_unknown_size : Error<
- "'counted_by' cannot be applied to %select{"
+ "'%select{counted_by|counted_by_or_null}3' cannot be applied to %select{"
"a pointer with pointee|" // pointer
"an array with element}0" // array
" of unknown size because %1 is %select{"
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index cd06bd9c230df..2ea7eb71a8f2e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11397,7 +11397,9 @@ class Sema final : public SemaBase {
SourceLocation AttrLoc);
QualType BuildCountAttributedArrayOrPointerType(QualType WrappedTy,
- Expr *CountExpr);
+ Expr *CountExpr,
+ bool CountInBytes,
+ bool OrNull);
QualType BuildAddressSpaceAttr(QualType &T, LangAS ASIdx, Expr *AddrSpace,
SourceLocation AttrLoc);
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index 87f0a8728d850..6f346ca894f60 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -1905,6 +1905,9 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
break;
case attr::CountedBy:
+ case attr::CountedByOrNull:
+ case attr::SizedBy:
+ case attr::SizedByOrNull:
case attr::LifetimeBound:
case attr::TypeNonNull:
case attr::TypeNullable:
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 8405b44685ae4..1dc307484204c 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -662,7 +662,10 @@ void Parser::ParseGNUAttributeArgs(
ParseAttributeWithTypeArg(*AttrName, AttrNameLoc, Attrs, ScopeName,
ScopeLoc, Form);
return;
- } else if (AttrKind == ParsedAttr::AT_CountedBy) {
+ } else if (AttrKind == ParsedAttr::AT_CountedBy ||
+ AttrKind == ParsedAttr::AT_CountedByOrNull ||
+ AttrKind == ParsedAttr::AT_SizedBy ||
+ AttrKind == ParsedAttr::AT_SizedByOrNull) {
ParseBoundsAttribute(*AttrName, AttrNameLoc, Attrs, ScopeName, ScopeLoc,
Form);
return;
@@ -4806,7 +4809,7 @@ static void DiagnoseCountAttributedTypeInUnnamedAnon(ParsingDeclSpec &DS,
if (!RD->containsDecl(DD.getDecl())) {
P.Diag(VD->getBeginLoc(),
diag::err_flexible_array_count_not_in_same_struct)
- << DD.getDecl();
+ << DD.getDecl() << CAT->getKind();
P.Diag(DD.getDecl()->getBeginLoc(),
diag::note_flexible_array_counted_by_attr_field)
<< DD.getDecl();
@@ -4964,6 +4967,9 @@ void Parser::ParseLexedCAttribute(LateParsedAttribute &LA,
/*SyntaxUsed=*/ParsedForm.getSyntax());
switch (AttrKind) {
case ParsedAttr::Kind::AT_CountedBy:
+ case ParsedAttr::Kind::AT_CountedByOrNull:
+ case ParsedAttr::Kind::AT_SizedBy:
+ case ParsedAttr::Kind::AT_SizedByOrNull:
ParseBoundsAttribute(LA.AttrName, LA.AttrNameLoc, Attrs,
/*ScopeName=*/ScopeName, SourceLocation(),
/*Form=*/ParsedForm);
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index c8b71631076ba..ebce77d368e50 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -8641,22 +8641,33 @@ enum class CountedByInvalidPointeeTypeKind {
VALID,
};
-static bool CheckCountedByAttrOnField(
- Sema &S, FieldDecl *FD, Expr *E,
- llvm::SmallVectorImpl<TypeCoupledDeclRefInfo> &Decls) {
+static bool
+CheckCountedByAttrOnField(Sema &S, FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl<TypeCoupledDeclRefInfo> &Decls,
+ bool CountInBytes, bool OrNull) {
// Check the context the attribute is used in
+ unsigned Kind = CountInBytes;
+ if (OrNull)
+ Kind += 2;
+
if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
- << FD->getSourceRange();
+ << Kind << FD->getSourceRange();
return true;
}
const auto FieldTy = FD->getType();
+ if (FieldTy->isArrayType() && (CountInBytes || OrNull)) {
+ S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
+ << Kind << FD->getLocation();
+ return true;
+ }
if (!FieldTy->isArrayType() && !FieldTy->isPointerType()) {
S.Diag(FD->getBeginLoc(),
diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
- << FD->getLocation();
+ << Kind << FD->getLocation();
return true;
}
@@ -8667,7 +8678,7 @@ static bool CheckCountedByAttrOnField(
StrictFlexArraysLevel, true)) {
S.Diag(FD->getBeginLoc(),
diag::err_counted_by_attr_on_array_not_flexible_array_member)
- << FD->getLocation();
+ << Kind << FD->getLocation();
return true;
}
@@ -8697,9 +8708,10 @@ static bool CheckCountedByAttrOnField(
InvalidTypeKind = CountedByInvalidPointeeTypeKind::FLEXIBLE_ARRAY_MEMBER;
}
- if (InvalidTypeKind != CountedByInvalidPointeeTypeKind::VALID) {
+ if (InvalidTypeKind != CountedByInvalidPointeeTypeKind::VALID &&
+ !CountInBytes) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_pointee_unknown_size)
- << SelectPtrOrArr << PointeeTy << (int)InvalidTypeKind
+ << SelectPtrOrArr << PointeeTy << (int)InvalidTypeKind << OrNull
<< FD->getSourceRange();
return true;
}
@@ -8708,7 +8720,7 @@ static bool CheckCountedByAttrOnField(
if (!E->getType()->isIntegerType() || E->getType()->isBooleanType()) {
S.Diag(E->getBeginLoc(), diag::err_counted_by_attr_argument_not_integer)
- << E->getSourceRange();
+ << Kind << E->getSourceRange();
return true;
}
@@ -8716,7 +8728,7 @@ static bool CheckCountedByAttrOnField(
if (!DRE) {
S.Diag(E->getBeginLoc(),
diag::err_counted_by_attr_only_support_simple_decl_reference)
- << E->getSourceRange();
+ << Kind << E->getSourceRange();
return true;
}
@@ -8727,7 +8739,7 @@ static bool CheckCountedByAttrOnField(
}
if (!CountFD) {
S.Diag(E->getBeginLoc(), diag::err_counted_by_must_be_in_structure)
- << CountDecl << E->getSourceRange();
+ << CountDecl << Kind << E->getSourceRange();
S.Diag(CountDecl->getBeginLoc(),
diag::note_flexible_array_counted_by_attr_field)
@@ -8738,7 +8750,7 @@ static bool CheckCountedByAttrOnField(
if (FD->getParent() != CountFD->getParent()) {
if (CountFD->getParent()->isUnion()) {
S.Diag(CountFD->getBeginLoc(), diag::err_counted_by_attr_refer_to_union)
- << CountFD->getSourceRange();
+ << Kind << CountFD->getSourceRange();
return true;
}
// Whether CountRD is an anonymous struct is not determined at this
@@ -8750,7 +8762,7 @@ static bool CheckCountedByAttrOnField(
if (RD != CountRD) {
S.Diag(E->getBeginLoc(),
diag::err_flexible_array_count_not_in_same_struct)
- << CountFD << E->getSourceRange();
+ << CountFD << Kind << E->getSourceRange();
S.Diag(CountFD->getBeginLoc(),
diag::note_flexible_array_counted_by_attr_field)
<< CountFD << CountFD->getSourceRange();
@@ -8770,12 +8782,35 @@ static void handleCountedByAttrField(Sema &S, Decl *D, const ParsedAttr &AL) {
if (!CountExpr)
return;
+ bool CountInBytes;
+ bool OrNull;
+ switch (AL.getKind()) {
+ case ParsedAttr::AT_CountedBy:
+ CountInBytes = false;
+ OrNull = false;
+ break;
+ case ParsedAttr::AT_CountedByOrNull:
+ CountInBytes = false;
+ OrNull = true;
+ break;
+ case ParsedAttr::AT_SizedBy:
+ CountInBytes = true;
+ OrNull = false;
+ break;
+ case ParsedAttr::AT_SizedByOrNull:
+ CountInBytes = true;
+ OrNull = true;
+ break;
+ default:
+ llvm_unreachable("unexpected counted_by family attribute");
+ }
+
llvm::SmallVector<TypeCoupledDeclRefInfo, 1> Decls;
- if (CheckCountedByAttrOnField(S, FD, CountExpr, Decls))
+ if (CheckCountedByAttrOnField(S, FD, CountExpr, Decls, CountInBytes, OrNull))
return;
- QualType CAT =
- S.BuildCountAttributedArrayOrPointerType(FD->getType(), CountExpr);
+ QualType CAT = S.BuildCountAttributedArrayOrPointerType(
+ FD->getType(), CountExpr, CountInBytes, OrNull);
FD->setType(CAT);
}
@@ -9802,6 +9837,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
break;
case ParsedAttr::AT_CountedBy:
+ case ParsedAttr::AT_CountedByOrNull:
+ case ParsedAttr::AT_SizedBy:
+ case ParsedAttr::AT_SizedByOrNull:
handleCountedByAttrField(S, D, AL);
break;
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index ef0b6b701a52c..832a30992f24a 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9346,15 +9346,17 @@ BuildTypeCoupledDecls(Expr *E,
}
QualType Sema::BuildCountAttributedArrayOrPointerType(QualType WrappedTy,
- Expr *CountExpr) {
+ Expr *CountExpr,
+ bool CountInBytes,
+ bool OrNull) {
assert(WrappedTy->isIncompleteArrayType() || WrappedTy->isPointerType());
llvm::SmallVector<TypeCoupledDeclRefInfo, 1> Decls;
BuildTypeCoupledDecls(CountExpr, Decls);
/// When the resulting expression is invalid, we still create the AST using
/// the original count expression for the sake of AST dump.
- return Context.getCountAttributedType(
- WrappedTy, CountExpr, /*CountInBytes*/ false, /*OrNull*/ false, Decls);
+ return Context.getCountAttributedType(WrappedTy, CountExpr, CountInBytes,
+ OrNull, Decls);
}
/// getDecltypeForExpr - Given an expr, will return the decltype for
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 29444f0edc2ae..cf96c9e2af68f 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -7344,7 +7344,8 @@ QualType TreeTransform<Derived>::TransformCountAttributedType(
if (getDerived().AlwaysRebuild() || InnerTy != OldTy->desugar() ||
OldCount != NewCount) {
// Currently, CountAttributedType can only wrap incomplete array types.
- Result = SemaRef.BuildCountAttributedArrayOrPointerType(InnerTy, NewCount);
+ Result = SemaRef.BuildCountAttributedArrayOrPointerType(
+ InnerTy, NewCount, OldTy->isCountInBytes(), OldTy->isOrNull());
}
TLB.push<CountAttributedTypeLoc>(Result);
diff --git a/clang/test/AST/attr-counted-by-or-null-late-parsed-struct-ptrs.c b/clang/test/AST/attr-counted-by-or-null-late-parsed-struct-ptrs.c
new file mode 100644
index 0000000000000..975c0a0231943
--- /dev/null
+++ b/clang/test/AST/attr-counted-by-or-null-late-parsed-struct-ptrs.c
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -fexperimental-late-parse-attributes %s -ast-dump | FileCheck %s
+
+#define __counted_by_or_null(f) __attribute__((counted_by_or_null(f)))
+
+struct size_known {
+ int field;
+};
+
+//==============================================================================
+// __counted_by_or_null on struct member pointer in decl attribute position
+//==============================================================================
+
+struct on_member_pointer_complete_ty {
+ struct size_known *buf __counted_by_or_null(count);
+ int count;
+};
+// CHECK-LABEL: struct on_member_pointer_complete_ty definition
+// CHECK-NEXT: |-FieldDecl {{.*}} buf 'struct size_known * __counted_by_or_null(count)':'struct size_known *'
+// CHECK-NEXT: `-FieldDecl {{.*}} referenced count 'int'
+
+struct on_pointer_anon_count {
+ struct size_known *buf __counted_by_or_null(count);
+ struct {
+ int count;
+ };
+};
+
+// CHECK-LABEL: struct on_pointer_anon_count definition
+// CHECK-NEXT: |-FieldDecl {{.*}} buf 'struct size_known * __counted_by_or_null(count)':'struct size_known *'
+// CHECK-NEXT: |-RecordDecl {{.*}} struct definition
+// CHECK-NEXT: | `-FieldDecl {{.*}} count 'int'
+// CHECK-NEXT: |-FieldDecl {{.*}} implicit 'struct on_pointer_anon_count::(anonymous at {{.*}})'
+// CHECK-NEXT: `-IndirectFieldDecl {{.*}} implicit referenced count 'int'
+// CHECK-NEXT: |-Field {{.*}} '' 'struct on_pointer_anon_count::(anonymous at {{.*}})'
+// CHECK-NEXT: `-Field {{.*}} 'count' 'int'
+
+//==============================================================================
+// __counted_by_or_null on struct member pointer in type attribute position
+//==============================================================================
+// TODO: Correctly parse counted_by_or_null as a type attribute. Currently it is parsed
+// as a declaration attribute and is **not** late parsed resulting in the `count`
+// field being unavailable.
+//
+// See `clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c` for test
+// cases.
diff --git a/clang/test/AST/attr-counted-by-or-null-struct-ptrs.c b/clang/test/AST/attr-counted-by-or-null-struct-ptrs.c
new file mode 100644
index 0000000000000..cedb3f1192eda
--- /dev/null
+++ b/clang/test/AST/attr-counted-by-or-null-struct-ptrs.c
@@ -0,0 +1,117 @@
+// RUN: %clang_cc1 %s -ast-dump | FileCheck %s
+
+#define __counted_by_or_null(f) __attribute__((counted_by_or_null(f)))
+
+struct size_unknown;
+struct size_known {
+ int field;
+};
+
+//==============================================================================
+// __counted_by_or_null on struct member pointer in decl attribute position
+//==============================================================================
+
+// CHECK-LABEL: RecordDecl {{.+}} struct on_member_pointer_complete_ty definition
+// CHECK-NEXT: |-FieldDecl {{.+}} referenced count 'int'
+// CHECK-NEXT: `-FieldDecl {{.+}} buf 'struct size_known * __counted_by_or_null(count)':'struct size_known *'
+struct on_member_pointer_complete_ty {
+ int count;
+ struct size_known * buf __counted_by_or_null(count);
+};
+
+// CHECK-LABEL: RecordDecl {{.+}} struct on_pointer_anon_buf definition
+// CHECK-NEXT: |-FieldDecl {{.+}} referenced count 'int'
+// CHECK-NEXT: |-RecordDecl {{.+}} struct definition
+// CHECK-NEXT: | `-FieldDecl {{.+}} buf 'struct size_known * __counted_by_or_null(count)':'struct size_known *'
+// CHECK-NEXT: |-FieldDecl {{.+}} implicit 'struct on_pointer_anon_buf::(anonymous at [[ANON_STRUCT_PATH:.+]])'
+// CHECK-NEXT: `-IndirectFieldDecl {{.+}} implicit buf 'struct size_known * __counted_by_or_null(count)':'struct size_known *'
+// CHECK-NEXT: |-Field {{.+}} '' 'struct on_pointer_anon_buf::(anonymous at [[ANON_STRUCT_PATH]])'
+// CHECK-NEXT: `-Field {{.+}} 'buf' 'struct size_known * __counted_by_or_null(count)':'struct size_known *'
+struct on_pointer_anon_buf {
+ int count;
+ struct {
+ struct size_known *buf __counted_by_or_null(count);
+ };
+};
+
+struct on_pointer_anon_count {
+ struct {
+ int count;
+ };
+ struct size_known *buf __counted_by_or_null(count);
+};
+
+//==============================================================================
+// __counted_by_or_null on struct member pointer in type attribute position
+//===============...
[truncated]
|
|
Blocked by pointer support for |
delcypher
requested changes
May 24, 2024
| have been added as variants on ``counted_by``, each with slightly different semantics. | ||
| ``sized_by`` takes a byte size parameter instead of an element count, allowing pointees | ||
| with unknown size. The ``counted_by_or_null`` and ``sized_by_or_null`` variants are equivalent | ||
| to their base variants, except the pointer can be null regardless of count/size value. |
There was a problem hiding this comment.
Nit: The description of the behavior of counted_by with a nullptr is the -fbound-safety behavior. I'm not sure if that restriction exists for counted_by outside that mode.
There was a problem hiding this comment.
Hmm yeah there's nothing preventing you from assigning null I guess, but I don't believe __bdos CG has any support for returning 0 when the pointer is null.
There was a problem hiding this comment.
It would be nice to add a suggested use case for sized_by_or_null. e.g., an allocator that returns either a buffer of size or nullptr.
There was a problem hiding this comment.
Nit: Maybe it's worth calling out (because it's really not obvious) that the only valid count for __sized_by when the pointer is NULL is 0, __sized_by_or_null allows any count value when the pointer is NULL. We could also note that this isn't currently enforced.
AaronBallman
approved these changes
May 31, 2024
There was a problem hiding this comment.
There are a few minor suggestions from @delcypher that should be addressed, but overall the changes LGTM (assuming there are no precommit CI surprises after fixing the merge conflicts).
rapidsna
approved these changes
May 31, 2024
hnrklssn
force-pushed
the
eng/PR-125400354
branch
from
fa82a97 to
5c5a284
Compare
|
Rebased on main after @delcypher's patch was relanded. Addressing comments now. |
delcypher
added
the
clang:bounds-safety
AaronBallman
approved these changes
Jun 18, 2024
There was a problem hiding this comment.
Continues to LGTM, @delcypher are you happy with the changes?
|
@delcypher @rapidsna updated __sized_by(_or_null) to only allow types with unknown size if incomplete |
|
Still LGTM |
delcypher
approved these changes
Jul 8, 2024
| have been added as variants on ``counted_by``, each with slightly different semantics. | ||
| ``sized_by`` takes a byte size parameter instead of an element count, allowing pointees | ||
| with unknown size. The ``counted_by_or_null`` and ``sized_by_or_null`` variants are equivalent | ||
| to their base variants, except the pointer can be null regardless of count/size value. |
There was a problem hiding this comment.
Nit: Maybe it's worth calling out (because it's really not obvious) that the only valid count for __sized_by when the pointer is NULL is 0, __sized_by_or_null allows any count value when the pointer is NULL. We could also note that this isn't currently enforced.
| @@ -8320,6 +8320,15 @@ static const RecordDecl *GetEnclosingNamedOrTopAnonRecord(const FieldDecl *FD) { | |||
| return RD; | |||
| } | |||
|
|
|||
| static CountAttributedType::DynamicCountPointerKind | |||
| getCountAttrKind(bool CountInBytes, bool OrNull) { | |||
There was a problem hiding this comment.
Nit. Should this be a class method on CountAttributedType?
There was a problem hiding this comment.
I guess it could be a static method. It can't be an instance method since it's used before the object is constructed. Since it's only used in one place I thought it'd improve readability to keep it close to the call site.
| unsigned char count; // expected-note {{'count' declared here}} | ||
| struct A { | ||
| int dummy; | ||
| int * ptr __counted_by_or_null(count); // expected-error {{'counted_by_or_null' field 'count' isn't within the same struct as the annotated pointer}} |
There was a problem hiding this comment.
Nit: This wording is a little odd. I would expect something like.
'counted_by_or_null' references field 'count' which isn't within the same struct as the annotated pointer
There was a problem hiding this comment.
@hnrklssn Looks like you just modified the existing wording. I definitely think it could be improved but we can do that in a separate patch if you prefer.
The attributes `sized_by`, `counted_by_or_null` and `sized_by_or_null` have been added as variants on `counted_by`, each with slightly different semantics. `sized_by` takes a byte size parameter instead of an element count, allowing pointees with unknown size. The `counted_by_or_null` and `sized_by_or_null` variants are equivalent to their base variants, except the pointer can be null regardless of count/size value. If the pointer is null the size is effectively 0. rdar://125400354
- use enum to index into diagnostics - rename err_counted_by* diagnostics to err_count_attr* when the diagnostic applies to other attributes than only counted_by (sized_by, counted_by_or_null, sized_by_or_null) - suggest using counted_by when arrays are annotated with sized_by, counted_by_or_null or sized_by_or_null - update release notes with example use case for sized_by_or_null
Emit an error when sized_by or sized_by_or_null is used on sizeless types, function pointers and pointers to structs with flexible array members. Although we *could* track their size, it is not useful and indexing would not work.
hnrklssn
force-pushed
the
eng/PR-125400354
branch
from
6ca0ef2 to
9d2b87d
Compare
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
…lvm#93231) The attributes `sized_by`, `counted_by_or_null` and `sized_by_or_null` have been added as variants on `counted_by`, each with slightly different semantics. `sized_by` takes a byte size parameter instead of an element count, allowing pointees with unknown size. The `counted_by_or_null` and `sized_by_or_null` variants are equivalent to their base variants, except the pointer can be null regardless of count/size value. If the pointer is null the size is effectively 0. rdar://125400354
|
I really like the test coverage of this feature, but I started to wonder if we should start splitting up some sema tests into subfolders. I think the bounds-safety related tests would be a good candidate to have their own subfolder :) |
|
@Xazax-hun This is something I'd like too. Unfortunately in previous discussions we were asked to not do this. As the number of tests grow I think it will become more justfiable to put Bounds Safety tests in their own folder. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The attributes
sized_by,counted_by_or_nullandsized_by_or_nullhave been added as variants oncounted_by, each with slightly different semantics.sized_bytakes a byte size parameter instead of an element count, allowing pointees with unknown size. Thecounted_by_or_nullandsized_by_or_nullvariants are equivalent to their base variants, except the pointer can be null regardless of count/size value. If the pointer is null the size is effectively 0.rdar://125400354