[clang] [Clang] Add -Wcounted-by-addrof to warn when &fam discards __counted_by (PR #206760)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> > > > > > I haven't given this a thorough review, but one question (probably for @rapidsna) that comes to mind is: because `__counted_by` is a declaration or type attribute, do we have to drop the information when taking the address? I would assume that would form a pointer to a counted by FAM?
> > > > > 
> > > > > 
> > > > > @AaronBallman Yes, counted_by can be attached to T (*)[]. But that would be misleading when the pointer actually gets used in most cases because the element size is still zero. Pointer arithmetic still has stride 0, and indexing does too. With __bdos it returns -1, which is likely not what the user intended.
> > > > 
> > > > 
> > > > Why is the element size zero? You can't have an array whose element type is incomplete, so I think we should always know the element size?
> > > 
> > > 
> > > Sorry, I was unclear — the "element" of a pointer-to-incomplete-array (i.e. the element for pointer arithmetic) is the incomplete array `T[]`, not `T`. `__counted_by` would be misleading there: if it were respected, the stride would be `sizeof(T) * n`, but in practice the stride stays zero because `sizeof(T[])` is zero.
> > 
> > 
> > I might just be dense, but how do you get into that scenario? https://godbolt.org/z/jeEafnT9n
> > Or are you talking about pointer arithmetic on the structure type which contains the FAM?
> 
> Sorry, we're talking past each other. Concretely, given
> 
> `struct S { int len; char fam[] __counted_by(len); };` the array's element type is char — complete, no issue there. The type I'm calling out is the one produced by `&p->fam`, which is `char (*)[]`. Its pointee is `char[]` — the incomplete array itself.

Phew, I'm following along so far. :-)

> The original issue this PR addresses is a common C idiom: given `char arr[] __counted_by(n)`, programmers often write `&arr` when they meant `arr` or `&arr[0]` (e.g., passing to a `void *` parameter). In plain C that's harmless in practice — nobody actually does sizeof or pointer arithmetic on the resulting `char (*)[]`, so no visible misbehavior. But once bounds enter the picture — via __builtin_[dynamic_]object_size or -fbounds-safety — the upper bound differs meaningfully between &p->fam and p->fam/&p->fam[0]: for the former, the pointee is char[] (incomplete), so the upper bound is unknown; for the latter, it's p->fam + n. E.g., __bdos(&p->fam, 1) returns -1, while __bdos(p->fam, 1) returns the count-derived size. And &p->fam is almost always a mistake, not intent.

Ahhhhh, thank you, this is what I was missing.

> Arguably, __counted_by could be honored on the incomplete-array pointee itself (`char (*)[__counted_by(n)]`) to reconstruct the upper bound. I'd initially thought that would be problematic because I assumed `sizeof(arr[])` and `q+1` silently became stride-0 — but on rechecking, they're rejected outright, so honoring the attribute on the pointee is less fraught than I thought. Still, absent that work, warning users off the idiom is the pragmatic call.

Yeah, this was my thinking as well; basically, `__counted_by` would actually be involved in all of those cases because it's providing more information which the type system should be free to use, so I was expecting us to treat it like `char (*)[__counted_by(n)]`.

If we don't go that route, then I agree the diagnostic in the PR makes sense. So maybe this is a temporary diagnostic while research is done to see whether we can make use of the extra type information?

https://github.com/llvm/llvm-project/pull/206760