[PATCH] D106614: [Clang] add btf_tag attribute
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 29 05:15:08 PDT 2021
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
More information about the cfe-commits
mailing list