[PATCH] D106614: [Clang] add btf_tag attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1835
+  let Args = [StringArgument<"BTFTag">];
+  let Subjects = SubjectList<[Var, Function, Record, Field], ErrorDiag>;
+  let Documentation = [BTFTagDocs];
----------------
yonghong-song wrote:
> aaron.ballman wrote:
> > ObjC method declarations?
> > 
> > Also, can this apply to *any* kind of variable declaration? e.g., does it do something useful on a `constexpr` variable that never gets emitted at runtime?
> The attribute named btf_tag and it is supposed to be used for bpf programs and kernel, and currently all our use cases are C, so hence ObjC is not considered and I marked it as COnly.
> 
> constexpr is not our use case so it is okay the variable (and possible attricute) is gone.
Great, thank you!


================
Comment at: clang/test/Sema/attr-btf_tag.c:15
+  return arg->a;
+}
----------------
yonghong-song wrote:
> aaron.ballman wrote:
> > There are quite a few test cases that are missing. Can you please add tests for applying the attribute to something invalid (like an enumeration), not given any arguments, given too many arguments.
> > 
> > Also missing are test cases for how this attribute merges on redeclarations, especially with conflicting arguments. e.g.,
> > ```
> > void __attribute__((btf_tag("tag"))) bar();
> > void __attribute__((btf_tag("derp"))) bar() {} // Which argument "wins" or should this be an error?
> > ```
> > Should it be valid to apply this to an incomplete structure declaration? e.g.,
> > ```
> > struct __attribute__((btf_tag("tag"))) S; // Should this be valid or invalid?
> > ```
> Thanks. I will add more test cases about redeclaration. For redeclaration case, my current thinking is they should match *exactly*. Otherwise, we should fail the compilation. I guess some implementation may be needed here.
> 
> For forward declaration, the current intention is not allowed. the btf_tag should just appear in the actual type definition.
> 
> For other types like enum, we didn't find a use case for that yet and that is why is not supported. I will add more tests to cover such invalid cases.
> Thanks. I will add more test cases about redeclaration. For redeclaration case, my current thinking is they should match *exactly*. Otherwise, we should fail the compilation. I guess some implementation may be needed here.

That sounds reasonable to me.  In terms of the implementation work, you're going to want to use the "attribute merge" pattern, which has a number of good examples to follow such as: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L2634.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106614