[PATCH] D106614: [Clang] add btf_tag attribute

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added inline comments.


================
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);
----------------
aaron.ballman wrote:
> 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?
>> Can you remind me: what is the concern if the attributes are additive on redeclaration?

I guess my main concern is people may look at one place for attribute availability. They may find it not there but actually it is defined in a different places. But in practice this may not be an issue as people typically will put attributes in the header *unique* declaration and/or the later *unique* definition.

Let me just to use additive approach which will simplify the whole thing a lot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106614