llvm / llvm/llvm-project

[Clang] Add -Wcounted-by-addrof to warn when &fam silently bypasses __counted_by

Open
#206,536 1 comment 0 reactions 0 assignees View on GitHub
clang:bounds-safety
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

### Summary

When taking the address of a flexible array member annotated with
`__counted_by`, `__builtin_dynamic_object_size(&fam, 1)` falls through
to layout-derived `llvm.objectsize` and silently discards the
`__counted_by` annotation. For statically allocated bases the layout
answer is in fact the truth -- `__counted_by` cannot override a fixed
allocation -- but for pointer bases the layout fallback gives `-1`
("unknown"), while `__counted_by` would have given a usable runtime
bound. The annotation is silently disregarded in either case.

Under `-fbounds-safety` `&fam` on a `__counted_by`-annotated FAM is
already an error. I'd like to propose a parallel diagnostic for non-
bounds-safety mode -- likely a warning `-Wcounted-by-addrof` -- so
users outside bounds-safety can opt into the same check.

### Example

```c
typedef __SIZE_TYPE__ size_t;

struct annotated_flex {
size_t count;
char induce_padding;
char fam[] __attribute__((counted_by(count)));
};

struct annotated_flex gaf = { .fam = "i am very long", .count = 10 };

size_t global_addrof(void) {
// returns 15 (initializer length). count=10 cannot grow the static
// storage; layout-derived is the real bound.
return __builtin_dynamic_object_size(&gaf.fam, 1);
}

size_t local_addrof(size_t n) {
// returns 7 (trailing struct padding), regardless of n. Same story:
// count cannot extend the static allocation.
struct annotated_flex af = { .count = n };
return __builtin_dynamic_object_size(&af.fam, 1);
}

size_t ptr_addrof(struct annotated_flex *p) {
// returns SIZE_MAX (-1). Here __counted_by would have produced a
// useful bound; layout has nothing to say.
return __builtin_dynamic_object_size(&p->fam, 1);
}
```

The decayed forms (`gaf.fam`, `&gaf.fam[idx]`, `p->fam`, etc.)
consistently consult `__counted_by` and report the count-derived
bound -- which is what the attribute promises. For the pointer case
this is the genuinely useful answer the address-of form throws away.
For the static cases the count-derived bound can still *disagree*
with the physical allocation (e.g. `gaf.fam` reports `count=10`
while the storage holds 15 bytes; `af.fam` reports `n` while the
storage is fixed at 7), but that mismatch is a property of the
declaration, not of `&fam`, and is tracked separately in #206541.

### Proposed behavior

- New warning `-Wcounted-by-addrof`.
- Fires when the operand of unary `&` has `CountAttributedType` and is
an incomplete array (the FAM-as-a-whole shape that bypasses
`emitCountedBySize` in CGBuiltin).
- Fix-it: remove the `&` to get the decayed pointer-to-element form,
which honors the count. **Only offered when the base's allocation
isn't statically known** (pointer bases, function parameters,
opaque returns, etc.). For automatic/static-storage bases, the
decay form's count-derived bound can disagree with the physical
allocation -- often the layout-derived answer the user already has
is the more truthful one -- so the diagnostic fires without a
fix-it. See #206541 for the underlying static-storage problem.
- The bounds-safety error already uses similar wording; the new
warning should stay close to it for consistency, modulo the
conditional fix-it above.

The strongest case for the warning is the pointer-base shape, where
`__counted_by` would have produced a bound and the layout fallback
cannot. The local/global cases are weaker on their own (the layout
answer is technically correct relative to the static allocation), but
flagging them is still useful because `&fam` discards the count in
shapes where the user almost certainly didn't expect that.

### Default state

Off by default, opt-in via `-W` (or `-Wall`) as a conservative
starting point. I'd be open to flipping later once we have feedback.

### Related

The static-allocation cases above (`local_addrof`, `global_addrof`)
hint at a separate latent issue: a `__counted_by` FAM in static
storage is structurally suspect, because the count cannot constrain a
fixed allocation, and the count-derived bound the decay form returns
can disagree with the actual storage in either direction. A companion
warning at the *declaration* site -- not the address-of site -- would
catch the root cause for those shapes, filed as #206541.

Contributor guide

Open the contributing guide

Research direction

Start at the CGBuiltin handling for __builtin_dynamic_object_size and compare it with the existing bounds-safety diagnostic for taking the address of a counted-by flexible array member. Trace the pointer-base and static-storage examples in the issue, then add diagnostic coverage for the warning, its default state, and the conditional fix-it; done means the proposed cases produce the expected warning behavior without changing decayed forms.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
compilers
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.