[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

[Kees Cook via Phabricator via cfe-commits]

kees added a comment.

In D148381#4645600 <https://reviews.llvm.org/D148381#4645600>, @void wrote:

> Added more error messages. Changed some code around to align with coding practices. Added some more test cases.

Doing a full "allmodconfig" Linux kernel build with my 200ish __counted_by annotations is building cleanly. Yay! :)



================
Comment at: clang/test/Sema/attr-counted-by.c:21
+  struct bar *fam[] __counted_by(non_integer); // expected-note {{counted_by field 'non_integer' declared here}}
+};
----------------
void wrote:
> aaron.ballman wrote:
> > Please add test with the other strange FAM-like members, as well as one that is just a trailing constant array of size 2.
> > 
> > Some more tests or situations to consider:
> > ```
> > struct S {
> >   struct {
> >     int NotInThisStruct;
> >   };
> >   int FAM[] __counted_by(NotInThisStruct); // Should this work?
> > };
> > 
> > int Global;
> > struct T {
> >   int FAM[] __counted_by(Global); // Should fail per your design but is that the behavior you want?
> > };
> > 
> > struct U {
> >   struct {
> >     int Count;
> >   } data;
> >   int FAM[] __counted_by(data.Count); // Thoughts?
> > };
> > 
> > struct V {
> >   int Sizes[2];
> >   int FAM[] __counted_by(Sizes[0]); // Thoughts?
> > };
> > ```
> > (I'm not suggesting any of this needs to be accepted in order to land the patch.)
> > 
> > You should also have tests showing the attribute is diagnosed when applied to something other than a field, given something other than an identifier, is given zero arguments, and is given two arguments.
> I added a "this isn't a flexible array member" error message.
> 
> ```
> struct S {
>   struct {
>     int NotInThisStruct;
>   };
>   int FAM[] __counted_by(NotInThisStruct); // Should this work?
> };
> ```
> 
> Yes, I believe it should. Kees had a similar example:
> 
> ```
> struct S {
>   int x;
>   struct {
>     int count;
>     int FAM[] __counted_by(count);
>   };
> };
> ```
> 
> I made changes to support this. It may be controversial, so please let me know if you or anyone has an issue with it.
> 
> ```
> int Global;
> struct T {
>   int FAM[] __counted_by(Global); // Should fail per your design but is that the behavior you want?
> };
> ```
> 
> Yes, it should fail. I'll add a test case.
> 
> ```
> struct U {
>   struct {
>     int Count;
>   } data;
>   int FAM[] __counted_by(data.Count); // Thoughts?
> };
> ```
> 
> I don't think we should support that at the moment. Referencing arbitrary fields in a nested structure is very complicated and the syntax for it is not at all settled on.
> 
> ```
> struct V {
>   int Sizes[2];
>   int FAM[] __counted_by(Sizes[0]); // Thoughts?
> };
> ```
> 
> Hmm...Not sure. I'll ask the GCC person who's implementing this on her side about this.
> Yes, I believe it should. Kees had a similar example:
> 
> ```
> struct S {
>   int x;
>   struct {
>     int count;
>     int FAM[] __counted_by(count);
>   };
> };
> ```
> 
> I made changes to support this. It may be controversial, so please let me know if you or anyone has an issue with it.

FWIW, the Linux kernel has a LOT of this style (in an anonymous struct), which is why I wanted to make sure it was supported in this first version of the feature.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148381/new/

https://reviews.llvm.org/D148381