[PATCH] D106614: [Clang] add btf_tag attribute

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song 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;
----------------
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.


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106614