Skip to content

[Cherry-pick][BoundsSafety][Sema] Allow counted_by and counted_by_or_null on pointers where the pointee type is incomplete but potentially completable #10514

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 1 commit into from
Apr 18, 2025

Conversation

delcypher
Copy link


This is a cherry-pick of the change in llvm#106321. It is being cherry-pick now so that the merge conflict is handled upfront to minimize the work when the conflict from the upstream change reaches the automerger.

Conflicts:
clang/include/clang/AST/Type.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/Type.cpp
clang/lib/Sema/SemaBoundsSafety.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaInit.cpp


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

…null on pointers where the pointee type is incomplete but potentially completable

---

This is a cherry-pick of the change in llvm#106321.
It is being cherry-pick now so that the merge conflict is handled
upfront to minimize the work when the conflict from the upstream change
reaches the automerger.

Conflicts:
	clang/include/clang/AST/Type.h
	clang/include/clang/Sema/Sema.h
	clang/lib/AST/Type.cpp
	clang/lib/Sema/SemaBoundsSafety.cpp
	clang/lib/Sema/SemaExpr.cpp
	clang/lib/Sema/SemaInit.cpp

---

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
@delcypher delcypher added the clang:bounds-safety Issue relating to the experimental -fbounds-safety feature in Clang label Apr 18, 2025
@@ -1086,6 +1086,7 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E,
if (!Count.get())
return ExprError();

// FIXME: Hoist this out of if-else blocks
Copy link
Author

Choose a reason for hiding this comment

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

@rapidsna We should probably fix this after I merge this.

@delcypher
Copy link
Author

I'm going to merge this now so I can merge the upstream PR (llvm#106321). We can fix up problems with this afterwards.

@delcypher delcypher merged commit ca8ecf8 into next Apr 18, 2025
@delcypher delcypher deleted the dliew/pre-cherry-pick-133600117 branch April 18, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:bounds-safety Issue relating to the experimental -fbounds-safety feature in Clang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant