Skip to content

[Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null #93231

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.

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 3 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,15 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

If the pointer is null the size is effectively 0. ``sized_by_or_null`` is needed to properly
annotate allocator functions like ``malloc`` that return a buffer of a given byte size, but can
also return null.

- The ``guarded_by``, ``pt_guarded_by``, ``acquired_after``, ``acquired_before``
attributes now support referencing struct members in C. The arguments are also
now late parsed when ``-fexperimental-late-parse-attributes`` is passed like
Expand Down
30 changes: 30 additions & 0 deletions clang/include/clang/Basic/Attr.td
Original file line number Diff line number Diff line change
Expand Up @@ -2292,6 +2292,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".
Expand Down
30 changes: 15 additions & 15 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -6567,28 +6567,28 @@ def warn_superclass_variable_sized_type_not_at_end : Warning<
"field %0 can overwrite instance variable %1 with variable sized type %2"
" 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">;
def err_counted_by_attr_not_on_ptr_or_flexible_array_member : Error<
"'counted_by' only applies to pointers or C99 flexible array members">;
def err_count_attr_param_not_in_same_struct : Error<
"'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}1' field %0 isn't within the same struct as the annotated %select{pointer|flexible array}2">;
def err_count_attr_not_on_ptr_or_flexible_array_member : Error<
"'%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%select{|; did you mean to use 'counted_by'?}1">;
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">;
def err_counted_by_attr_argument_not_integer : Error<
"'counted_by' 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">;
def err_counted_by_attr_in_union : Error<
"'counted_by' 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">;
def err_count_attr_must_be_in_structure : Error<
"field %0 in '%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}1' not inside structure">;
def err_count_attr_argument_not_integer : Error<
"'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' requires a non-boolean integer type argument">;
def err_count_attr_only_support_simple_decl_reference : Error<
"'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' argument must be a simple declaration reference">;
def err_count_attr_in_union : Error<
"'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' cannot be applied to a union member">;
def err_count_attr_refer_to_union : Error<
"'%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' %select{cannot|should not}3 be applied to %select{"
"'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}4' %select{cannot|should not}3 be applied to %select{"
"a pointer with pointee|" // pointer
"an array with element}0" // array
" of unknown size because %1 is %select{"
Expand Down
4 changes: 3 additions & 1 deletion clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -14603,7 +14603,9 @@ class Sema final : public SemaBase {
SourceLocation AttrLoc);

QualType BuildCountAttributedArrayOrPointerType(QualType WrappedTy,
Expr *CountExpr);
Expr *CountExpr,
bool CountInBytes,
bool OrNull);

/// BuildAddressSpaceAttr - Builds a DependentAddressSpaceType if an
/// expression is uninstantiated. If instantiated it will apply the
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/AST/TypePrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1934,6 +1934,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:
Expand Down
10 changes: 6 additions & 4 deletions clang/lib/Parse/ParseDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,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;
Expand Down Expand Up @@ -4866,9 +4869,8 @@ static void DiagnoseCountAttributedTypeInUnnamedAnon(ParsingDeclSpec &DS,

for (const auto &DD : CAT->dependent_decls()) {
if (!RD->containsDecl(DD.getDecl())) {
P.Diag(VD->getBeginLoc(),
diag::err_flexible_array_count_not_in_same_struct)
<< DD.getDecl();
P.Diag(VD->getBeginLoc(), diag::err_count_attr_param_not_in_same_struct)
<< DD.getDecl() << CAT->getKind() << CAT->isArrayType();
P.Diag(DD.getDecl()->getBeginLoc(),
diag::note_flexible_array_counted_by_attr_field)
<< DD.getDecl();
Expand Down
91 changes: 67 additions & 24 deletions clang/lib/Sema/SemaDeclAttr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5868,6 +5868,15 @@ static const RecordDecl *GetEnclosingNamedOrTopAnonRecord(const FieldDecl *FD) {
return RD;
}

static CountAttributedType::DynamicCountPointerKind
getCountAttrKind(bool CountInBytes, bool OrNull) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. Should this be a class method on CountAttributedType?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (CountInBytes)
return OrNull ? CountAttributedType::SizedByOrNull
: CountAttributedType::SizedBy;
return OrNull ? CountAttributedType::CountedByOrNull
: CountAttributedType::CountedBy;
}

enum class CountedByInvalidPointeeTypeKind {
INCOMPLETE,
SIZELESS,
Expand All @@ -5876,22 +5885,31 @@ 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 = getCountAttrKind(CountInBytes, OrNull);

if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
<< FD->getSourceRange();
S.Diag(FD->getBeginLoc(), diag::err_count_attr_in_union)
<< Kind << FD->getSourceRange();
return true;
}

const auto FieldTy = FD->getType();
if (FieldTy->isArrayType() && (CountInBytes || OrNull)) {
S.Diag(FD->getBeginLoc(),
diag::err_count_attr_not_on_ptr_or_flexible_array_member)
<< Kind << FD->getLocation() << /* suggest counted_by */ 1;
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();
diag::err_count_attr_not_on_ptr_or_flexible_array_member)
<< Kind << FD->getLocation() << /* do not suggest counted_by */ 0;
return true;
}

Expand All @@ -5902,7 +5920,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;
}

Expand All @@ -5923,7 +5941,7 @@ static bool CheckCountedByAttrOnField(
// only `PointeeTy->isStructureTypeWithFlexibleArrayMember()` is reachable
// when `FieldTy->isArrayType()`.
bool ShouldWarn = false;
if (PointeeTy->isIncompleteType()) {
if (PointeeTy->isIncompleteType() && !CountInBytes) {
InvalidTypeKind = CountedByInvalidPointeeTypeKind::INCOMPLETE;
} else if (PointeeTy->isSizelessType()) {
InvalidTypeKind = CountedByInvalidPointeeTypeKind::SIZELESS;
Expand All @@ -5948,23 +5966,23 @@ static bool CheckCountedByAttrOnField(
: diag::err_counted_by_attr_pointee_unknown_size;
S.Diag(FD->getBeginLoc(), DiagID)
<< SelectPtrOrArr << PointeeTy << (int)InvalidTypeKind
<< (ShouldWarn ? 1 : 0) << FD->getSourceRange();
<< (ShouldWarn ? 1 : 0) << Kind << FD->getSourceRange();
return true;
}

// Check the expression

if (!E->getType()->isIntegerType() || E->getType()->isBooleanType()) {
S.Diag(E->getBeginLoc(), diag::err_counted_by_attr_argument_not_integer)
<< E->getSourceRange();
S.Diag(E->getBeginLoc(), diag::err_count_attr_argument_not_integer)
<< Kind << E->getSourceRange();
return true;
}

auto *DRE = dyn_cast<DeclRefExpr>(E);
if (!DRE) {
S.Diag(E->getBeginLoc(),
diag::err_counted_by_attr_only_support_simple_decl_reference)
<< E->getSourceRange();
diag::err_count_attr_only_support_simple_decl_reference)
<< Kind << E->getSourceRange();
return true;
}

Expand All @@ -5974,8 +5992,8 @@ static bool CheckCountedByAttrOnField(
CountFD = IFD->getAnonField();
}
if (!CountFD) {
S.Diag(E->getBeginLoc(), diag::err_counted_by_must_be_in_structure)
<< CountDecl << E->getSourceRange();
S.Diag(E->getBeginLoc(), diag::err_count_attr_must_be_in_structure)
<< CountDecl << Kind << E->getSourceRange();

S.Diag(CountDecl->getBeginLoc(),
diag::note_flexible_array_counted_by_attr_field)
Expand All @@ -5985,8 +6003,8 @@ 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();
S.Diag(CountFD->getBeginLoc(), diag::err_count_attr_refer_to_union)
<< Kind << CountFD->getSourceRange();
return true;
}
// Whether CountRD is an anonymous struct is not determined at this
Expand All @@ -5996,9 +6014,8 @@ static bool CheckCountedByAttrOnField(
auto *CountRD = GetEnclosingNamedOrTopAnonRecord(CountFD);

if (RD != CountRD) {
S.Diag(E->getBeginLoc(),
diag::err_flexible_array_count_not_in_same_struct)
<< CountFD << E->getSourceRange();
S.Diag(E->getBeginLoc(), diag::err_count_attr_param_not_in_same_struct)
<< CountFD << Kind << FieldTy->isArrayType() << E->getSourceRange();
S.Diag(CountFD->getBeginLoc(),
diag::note_flexible_array_counted_by_attr_field)
<< CountFD << CountFD->getSourceRange();
Expand All @@ -6018,12 +6035,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);
}

Expand Down Expand Up @@ -6971,6 +7011,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;

Expand Down
8 changes: 5 additions & 3 deletions clang/lib/Sema/SemaType.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9324,15 +9324,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
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Sema/TreeTransform.h
Original file line number Diff line number Diff line change
Expand Up @@ -7397,7 +7397,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);
Expand Down
Loading
Loading