[clang] [Clang] Implement the 'counted_by' attribute (PR #76348)
Bill Wendling via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 27 06:52:25 PST 2023
bwendling wrote:
Some (hopefully) helpful comments on this admittedly very large patch.
General Comment:
I pulled back on allowing the `count` to be in a non-anonymous struct that doesn't contain the flexible array. So this is no longer allowed:
```
struct count_not_in_substruct {
unsigned long flags;
unsigned char count; // expected-note {{field 'count' declared here}}
struct A {
int dummy;
int array[] __counted_by(count); // expected-error {{'counted_by' field 'count' isn't within the same struct as the flexible array}}
} a;
};
```
This should align with @rapidsna's bounds checking work and get rid of the "different behaviors for a pointer and non-pointer" that we had in the previous PR (https://github.com/llvm/llvm-project/pull/73730#discussion_r1430672564).
SEMA:
The SEMA code is a bit different from the previous PR. Most notably is that I'm not using the SEMA "lookup" utility the same way. There's no clear way to prevent lookup from climbing from the current scope up through the top-level scope. This leads to false negatives, duplicate warnings, and other horrors. Also, the `Scope` for each sub-structure is deleted by the time the entire structure's parsing is completed, making checking the entire struct after parsing more difficult. And the `isAnonymousStructOrUnion` flag isn't set during SEMA.
All of these combined to make the use of the "lookup" utility very frustrating. There are probably many improvements that could be made to the lookup, but I'd prefer to do those in a follow-up patch.
CodeGen:
One observation: the `getNonTransparentContext()` (sp?) method doesn't appear to work. In particular, if called on an anonymous struct, it will return that struct and not the enclosing "non-transparent" one. I may be misunderstanding what's meant by non-transparent in this case.
There are two ways that the `counted_by` attribute are used during code generation: for sanitizer checks, and with `__builtin_dynamic_object_size()`.
* Sanitizer check: This uses @rjmccall's suggestion (https://github.com/llvm/llvm-project/pull/73730#discussion_r1427627732) of calculating a byte offset from the flexible array to the "count." It works very well (barring some issues with negative offsets being represented by an unsigned `~1U` value). The only bit of ugliness was determining the offsets of fields that aren't in the top-level of a `RecordDecl`. (I'm curious why this hasn't come up before, but I digress.) I created `getFieldOffsetInBits()` to do this. It's not quite a hack, but could probably be a bit more elegant. (For instance, there could be a `RecordLayout` method indicating if a `FieldDecl` exists in the `RecordDecl`, but that wouldn't take sub-`RecordDecls` into account, and so on, and so on...)
* `__builtin_dynamic_object_size()`: Side-effects aren't allowed with this builtin. So we can work only with pointers that have either already been emitted or that have no side-effects. The "visitor" tries to find the "base" of the expression. If it's a `DeclRefExpr`, we should be able to emit a load of that and continue. If not, we look at `LocalDeclMap` to see if the base is already available. If not, we check to see if it has side-effects and emit it if it doesn't. Assuming one of those three happen, we grab the indices to the `count` and build a series of `GEPs` to them (which seems to be how Clang generates GEPs to nested fields). Again, there didn't appear to be an existing method to get these indices. (I assume that the general case isn't as easy as when working with C structures.)
So that's basically it. I have a program on my machine that compiles most of the examples in the `test/CodeGen/attr-counted-by.c` and it works well. I don't know if there's a unittest like place that runs examples such as these, but I'd be happy to include that test there.
Thank you all for taking the time to review this!
Share and enjoy!
https://github.com/llvm/llvm-project/pull/76348
More information about the cfe-commits
mailing list