[PATCH] D106614: [Clang] add btf_tag attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6841
+  if (auto *Rec = dyn_cast<RecordDecl>(D)) {
+    if (!Rec->isCompleteDefinition()) {
+      S.Diag(AL.getLoc(), diag::warn_btftag_attribute_fwd_decl_ignored) << Str;
----------------
yonghong-song wrote:
> aaron.ballman wrote:
> > Does this work for code like:
> > ```
> > struct __attribute__((btf_tag(""))) S {
> >   int x;
> > };
> > ```
> > (I didn't see a test case for the attribute being written in that location.)
> Yes, I missed this one as typically I (and kernel developers) write the attribute at the end of definition. I just checked that if we put attribute before the struct name, when the attribute is handled/merged, it is actually an incomplete definition.
I kind of thought that might be the case. I think you'll want to also test `!Rec->isBeingDefined()`


================
Comment at: clang/test/Sema/attr-btf_tag.c:31
+
+int __tag2 foo(struct t1 *arg); // expected-warning {{attribute 'btf_tag("tag2")' ignored as not in later redeclaration or definition}}
+int __tag foo(struct t1 *arg);
----------------
yonghong-song wrote:
> aaron.ballman wrote:
> > This looks backwards to me -- I think the initial declaration of `foo()` is fine and shouldn't be diagnosed, it's the subsequent declarations of `foo()` with a different `btf_tag` argument that should be diagnosed.
> > 
> > I think additional test cases for these semantics is:
> > ```
> > void bar();
> > void __tag bar(); // (I think I understood that you want this to diagnose.)
> > 
> > void __tag bar();
> > void bar(); // (But that you don't want this to diagnose.)
> > ```
> > 
> There are a little complication here. Maybe you can give some advice.
> We could have code like
>    D1: void __tag1 __tag2 bar();
>    D2: void bar();
> The reason is that we explicitly allow multiple btf_tag's in the same declaration and this is exactly our use case.
> 
> Current merge*Attribute() framework will take one attribute at a time.
> For example, first we will do
>     mergeBTFTagAttribute(D2, __tag1):
>       Here, we add __tag1 to D2, so we have
>        D2: void __tag1 bar();
> we then do
>     mergeBTFTagAttribute(D2, __tag2):
>       but D2 already has a __tag1 which is incompatible with __tag2, and
>       we will issue an error here. But it is incorrect to issue an error
>       since the correct semantics is to inherit both __tag1 and __tag2 for D2.
> 
> Let me take a look at the code how to best handle such cases. Please also let me know if you have any suggestions. thanks!
> 
> A little bit more. The following are possible cases to check:
>    (1)  void bar();
>          void __tag1 __tag2 bar();  // fail
>    (2).  void __tag1 bar();
>          void __tag1 __tag2 bar(); // fail
>    (3)   void __tag1 __tag2 bar();
>          void __tag1 __tag2 bar();  // succeed
>    (4). void __tag1 __tag2 __tag3 bar();
>          void __tag1 __tag2 bar();   // fail
>    (5). void __tag3 bar();
>          void __tag1 __tag2 bar(); // fail
>    (6). void __tag1 __tag2 bar();
>          void bar()  // succeed
> 
> Basically, for two same declaration except attributes, D1, D2,
>    no error/warning only if D2 has no btf_tag attributes, or
>    D1 and D2 have identical btf_tag attributes.
> Do this sound reasonable?
> Let me take a look at the code how to best handle such cases. Please also let me know if you have any suggestions. thanks!

Oh, that's... interesting! You want to accept multiple, potentially separate attribute specifiers on the initial declaration, but not allow any new attributes on redeclarations or the definition. Btw, there are some tests missing from your above comment. You also need to think about:
```
void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void bar(); // succeed

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // succeed

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void __attribute__((btf_tag("one"))) bar(); // fail

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void __attribute__((btf_tag("one"), btf_tag("two"), btf_tag("three"))) bar(); // fail

[[clang::btf_tag("one")]] void bar [[clang::btf_tag("two")]] (); // ok
void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // succeed
```
I don't think we have any attributes that behave like that currently. Can you remind me: what is the concern if the attributes are additive on redeclaration?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106614