[PATCH] D106614: [Clang] add btf_tag attribute

[Yonghong Song via Phabricator via cfe-commits]

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


================
Comment at: clang/include/clang/Basic/AttrDocs.td:2019
+targets. This attribute may be attached to a struct/union, struct/union field,
+function or variables declaration. If -g is specified, the ARGUMENT info will
+be preserved in IR and be emitted to dwarf. For BPF target, the ARGUMENT info
----------------
aaron.ballman wrote:
> yonghong-song wrote:
> > anakryiko wrote:
> > > can it be also applied to function's argument?
> > The attribute does apply to function argument. I missed it in the documentation. Will add it.
> 
thanks! Will change.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:2020
+function or variables declaration. If -g is specified, the ARGUMENT info will
+be preserved in IR and be emitted to dwarf. For BPF target, the ARGUMENT info
+will be emitted to .BTF ELF section too.
----------------
aaron.ballman wrote:
> 
Thanks. Will change.


================
Comment at: clang/test/Sema/attr-btf_tag.c:15
+  return arg->a;
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106614