Skip to content

Commit b17b93d

Browse files
committed
[BoundsSafety][Sema] Allow counted_by and counted_by_or_null on pointers where the pointee type is incomplete but potentially completable
Previously using the `counted_by` or `counted_by_or_null` attribute on a pointer with an incomplete pointee type was forbidden. Unfortunately this prevented a situation like the following from being allowed. Header file: ``` struct EltTy; // Incomplete type struct Buffer { size_t count; struct EltTy* __counted_by(count) buffer; // error before this patch }; ``` Implementation file: ``` struct EltTy { // definition }; void allocBuffer(struct Buffer* b) { b->buffer = malloc(sizeof(EltTy)* b->count); } ``` To allow code like the above but still enforce that the pointee type size is known in locations where `-fbounds-safety` needs to emit bounds checks the following scheme is used. * For incomplete pointee types that can never be completed (e.g. `void`) these are treated as error where the attribute is written (just like before this patch). * For incomplete pointee types that might be completable later on (struct, union, and enum forward declarations) in the translation unit, writing the attribute on the incomplete pointee type is allowed on the FieldDecl declaration but "uses" of the declared pointer are forbidden if at the point of "use" the pointee type is still incomplete. For this patch a "use" of a FieldDecl covers: * Explicit and Implicit initialization (note see **Tentative Definition Initialization** for an exception to this) * Assignment * Conversion to an lvalue (e.g. for use in an expression) In the swift lang fork of Clang the `counted_by` and `counted_by_or_null` attribute are allowed in many more contexts. That isn't the case for upstream Clang so the "use" checks for the attribute on VarDecl, ParamVarDecl, and function return type have been omitted from this patch because they can't be tested. However, the `BoundsSafetyCheckAssignmentToCountAttrPtrWithIncompletePointeeTy` and `BoundsSafetyCheckUseOfCountAttrPtrWithIncompletePointeeTy` functions retain the ability to emit diagnostics for these other contexts to avoid unnecessary divergence between upstream Clang and Apple's internal fork. Support for checking "uses" will be upstreamed when upstream Clang allows the `counted_by` and `counted_by_or_null` attribute in additional contexts. This patch has a few limitations: ** 1. Tentative Defition Initialization ** This patch currently allows something like: ``` struct IncompleteTy; struct Buffer { int count; struct IncompleteTy* __counted_by(count) buf; }; // Tentative definition struct Buffer GlobalBuf; ``` The Tentative definition in this example becomes an actual definition whose initialization **should be checked** but it currently isn't. Addressing this problem will be done in a subseqent patch. ** 2. When the incomplete pointee type is a typedef diagnostics are slightly misleading ** For this situation: ``` struct IncompleteTy; typedef struct IncompleteTy Incomplete_t; struct Buffer { int count; struct IncompleteTy* __counted_by(count) buf; }; void use(struct Buffer b) { b.buf = 0x0; } ``` This code emits `note: forward declaration of 'Incomplete_t' (aka 'struct IncompleteTy')` but the location is on the `struct IncompleteTy;` forward declaration. This is misleading because `Incomplete_t` isn't actually forward declared there (instead the underlying type is). This could be resolved by additional diagnostics that walk the chain of typedefs and explain each step of the walk. However, that would be very verbose and didn't seem like a direction worth pursuing. rdar://133600117
1 parent d7354e3 commit b17b93d

14 files changed

+1029
-27
lines changed

clang/include/clang/AST/Type.h

+22
Original file line numberDiff line numberDiff line change
@@ -2433,6 +2433,26 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
24332433
return !isFunctionType();
24342434
}
24352435

2436+
/// \returns True if the type is incomplete and it is also a type that
2437+
/// cannot be completed by a later type definition.
2438+
///
2439+
/// E.g. For `void` this is true but for `struct ForwardDecl;` this is false
2440+
/// because a definition for `ForwardDecl` could be provided later on in the
2441+
/// translation unit.
2442+
///
2443+
/// Note even for types that this function returns true for it is still
2444+
/// possible for the declarations that contain this type to later have a
2445+
/// complete type in a translation unit. E.g.:
2446+
///
2447+
/// \code{.c}
2448+
/// // This decl has type 'char[]' which is incomplete and cannot be later
2449+
/// // completed by another by another type declaration.
2450+
/// extern char foo[];
2451+
/// // This decl now has complete type 'char[5]'.
2452+
/// char foo[5]; // foo has a complete type
2453+
/// \endcode
2454+
bool isAlwaysIncompleteType() const;
2455+
24362456
/// Determine whether this type is an object type.
24372457
bool isObjectType() const {
24382458
// C++ [basic.types]p8:
@@ -3349,6 +3369,8 @@ class CountAttributedType final
33493369
static bool classof(const Type *T) {
33503370
return T->getTypeClass() == CountAttributed;
33513371
}
3372+
3373+
StringRef getAttributeName(bool WithMacroPrefix) const;
33523374
};
33533375

33543376
/// Represents a type which was implicitly adjusted by the semantic

clang/include/clang/Basic/DiagnosticSemaKinds.td

+37
Original file line numberDiff line numberDiff line change
@@ -4445,6 +4445,7 @@ def err_mismatched_visibility: Error<"visibility does not match previous declara
44454445
def note_previous_attribute : Note<"previous attribute is here">;
44464446
def note_conflicting_attribute : Note<"conflicting attribute is here">;
44474447
def note_attribute : Note<"attribute is here">;
4448+
def note_named_attribute : Note<"%0 attribute is here">;
44484449
def err_mismatched_ms_inheritance : Error<
44494450
"inheritance model does not match %select{definition|previous declaration}0">;
44504451
def warn_ignored_ms_inheritance : Warning<
@@ -6710,6 +6711,42 @@ def err_counted_by_attr_pointee_unknown_size : Error<
67106711
"%select{|. This will be an error in a future compiler version}3"
67116712
""
67126713
"}2">;
6714+
def err_counted_by_on_incomplete_type_on_assign : Error <
6715+
"cannot %select{"
6716+
"assign to %select{object|'%1'}2 that has|" // AA_Assigning,
6717+
"pass argument to %select{parameter|parameter '%1'}2 that has|" // AA_Passing,
6718+
"return|" // AA_Returning,
6719+
"convert to|" // AA_Converting (UNUSED)
6720+
"%select{|implicitly }3initialize %select{object|'%1'}2 that has|" // AA_Initializing,
6721+
"pass argument to parameter that has|" // AA_Sending (UNUSED)
6722+
"cast to|" // AA_Casting (UNUSED)
6723+
"pass argument to parameter that has" // AA_Passing_CFAudited (UNUSED)
6724+
"}0 type %4 because the pointee type %6 is incomplete and the '%5' attribute "
6725+
"requires the pointee type be complete when %select{"
6726+
"assigning|" // AA_Assigning,
6727+
"passing|" // AA_Passing,
6728+
"returning|" // AA_Returning,
6729+
"converting|" // AA_Converting (UNUSED)
6730+
"initializing|" // AA_Initializing,
6731+
"passing|" // AA_Sending (UNUSED)
6732+
"casting|" // AA_Casting (UNUSED)
6733+
"passing" // AA_Passing_CFAudited (UNUSED)
6734+
"}0; "
6735+
"consider providing a complete definition for %6 or using the "
6736+
"'__sized_by%select{|_or_null}7' attribute"
6737+
>;
6738+
def err_counted_by_on_incomplete_type_on_use : Error <
6739+
"cannot %select{"
6740+
"use '%1'|" // Generic expr
6741+
"call '%1'" // CallExpr
6742+
"}0 with%select{"
6743+
"|" // Generic expr
6744+
" return" // CallExpr
6745+
"}0 type %2 because the pointee type %3 is incomplete "
6746+
"and the '%4' attribute requires the pointee type be complete in this context; "
6747+
"consider providing a complete definition for %3 or using the "
6748+
"'__sized_by%select{|_or_null}5' attribute"
6749+
>;
67136750

67146751
def warn_counted_by_attr_elt_type_unknown_size :
67156752
Warning<err_counted_by_attr_pointee_unknown_size.Summary>,

clang/include/clang/Sema/Sema.h

+44
Original file line numberDiff line numberDiff line change
@@ -2098,6 +2098,50 @@ class Sema final : public SemaBase {
20982098
bool CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, bool CountInBytes,
20992099
bool OrNull);
21002100

2101+
/// Perform Bounds Safety Semantic checks for assigning to a `__counted_by` or
2102+
/// `__counted_by_or_null` pointer type \param LHSTy.
2103+
///
2104+
/// \param LHSTy The type being assigned to. Checks will only be performed if
2105+
/// the type is a `counted_by` or `counted_by_or_null ` pointer.
2106+
/// \param RHSExpr The expression being assigned from.
2107+
/// \param Action The type assignment being performed
2108+
/// \param Loc The SourceLocation to use for error diagnostics
2109+
/// \param Assignee The ValueDecl being assigned. This is used to compute
2110+
/// the name of the assignee. If the assignee isn't known this can
2111+
/// be set to nullptr.
2112+
/// \param ShowFullyQualifiedAssigneeName If set to true when using \p
2113+
/// Assignee to compute the name of the assignee use the fully
2114+
/// qualified name, otherwise use the unqualified name.
2115+
///
2116+
/// \returns True iff no diagnostic where emitted, false otherwise.
2117+
bool BoundsSafetyCheckAssignmentToCountAttrPtr(
2118+
QualType LHSTy, Expr *RHSExpr, AssignmentAction Action,
2119+
SourceLocation Loc, const ValueDecl *Assignee,
2120+
bool ShowFullyQualifiedAssigneeName);
2121+
2122+
/// Perform Bounds Safety Semantic checks for initializing a Bounds Safety
2123+
/// pointer.
2124+
///
2125+
/// \param Entity The entity being initialized
2126+
/// \param Kind The kind of initialization being performed
2127+
/// \param Action The type assignment being performed
2128+
/// \param LHSTy The type being assigned to. Checks will only be performed if
2129+
/// the type is a `counted_by` or `counted_by_or_null ` pointer.
2130+
/// \param RHSExpr The expression being used for initialization.
2131+
///
2132+
/// \returns True iff no diagnostic where emitted, false otherwise.
2133+
bool BoundsSafetyCheckInitialization(const InitializedEntity &Entity,
2134+
const InitializationKind &Kind,
2135+
AssignmentAction Action,
2136+
QualType LHSType, Expr *RHSExpr);
2137+
2138+
/// Perform Bounds Safety semantic checks for uses of invalid uses counted_by
2139+
/// or counted_by_or_null pointers in \param E.
2140+
///
2141+
/// \param E the expression to check
2142+
///
2143+
/// \returns True iff no diagnostic where emitted, false otherwise.
2144+
bool BoundsSafetyCheckUseOfCountAttrPtr(const Expr *E);
21012145
///@}
21022146

21032147
//

clang/lib/AST/Type.cpp

+41
Original file line numberDiff line numberDiff line change
@@ -2490,6 +2490,22 @@ bool Type::isIncompleteType(NamedDecl **Def) const {
24902490
}
24912491
}
24922492

2493+
bool Type::isAlwaysIncompleteType() const {
2494+
if (!isIncompleteType())
2495+
return false;
2496+
2497+
// Forward declarations of structs, classes, enums, and unions could be later
2498+
// completed in a compilation unit by providing a type definition.
2499+
if (getAsTagDecl())
2500+
return false;
2501+
2502+
// Other types are incompletable.
2503+
//
2504+
// E.g. `char[]` and `void`. The type is incomplete and no future
2505+
// type declarations can make the type complete.
2506+
return true;
2507+
}
2508+
24932509
bool Type::isSizelessBuiltinType() const {
24942510
if (isSizelessVectorType())
24952511
return true;
@@ -3931,6 +3947,31 @@ CountAttributedType::CountAttributedType(
39313947
DeclSlot[i] = CoupledDecls[i];
39323948
}
39333949

3950+
StringRef CountAttributedType::getAttributeName(bool WithMacroPrefix) const {
3951+
// TODO: This method isn't really ideal because it doesn't return the spelling
3952+
// of the attribute that was used in the user's code. This method is used for
3953+
// diagnostics so the fact it doesn't use the spelling of the attribute in
3954+
// the user's code could be confusing (#113585).
3955+
#define ENUMERATE_ATTRS(PREFIX) \
3956+
do { \
3957+
if (isCountInBytes()) { \
3958+
if (isOrNull()) \
3959+
return PREFIX "sized_by_or_null"; \
3960+
return PREFIX "sized_by"; \
3961+
} \
3962+
if (isOrNull()) \
3963+
return PREFIX "counted_by_or_null"; \
3964+
return PREFIX "counted_by"; \
3965+
} while (0)
3966+
3967+
if (WithMacroPrefix)
3968+
ENUMERATE_ATTRS("__");
3969+
else
3970+
ENUMERATE_ATTRS("");
3971+
3972+
#undef ENUMERATE_ATTRS
3973+
}
3974+
39343975
TypedefType::TypedefType(TypeClass tc, const TypedefNameDecl *D,
39353976
QualType Underlying, QualType can)
39363977
: Type(tc, can, toSemanticDependence(can->getDependence())),

0 commit comments

Comments
 (0)