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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
