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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
