[Clang] add btf_tag attribute

[yonghong-song]

Summary

A new attribute btf_tag is added. The syntax looks like

__attribute__((btf_tag(<string>)))

Users may tag a particular structure/member/function/func_parameter/variable
declaration with an arbitrary string and the intention is
that this string is passed to dwarf so it is available for
post-compilation analysis. The string will be also passed
to .BTF section if the target is BPF. For each permitted
declaration, multiple btf_tag's are allowed.
For detailed use cases, please see

https://lists.llvm.org/pipermail/llvm-dev/2021-June/151009.html

In case that there exist redeclarations, the btf_tag attributes
will be accumulated along with different declarations, and the
last declaration will contain all attributes.

[anakryiko]

clang/include/clang/Basic/AttrDocs.td

2019

can it be also applied to function's argument?

clang/test/Sema/attr-btf_tag.c

13

in this case, second __tag, does it apply to function argument, or pointer, or pointer's pointee (struct t1)?

[yonghong-song]

clang/include/clang/Basic/AttrDocs.td

2019

The attribute does apply to function argument. I missed it in the documentation. Will add it.

clang/test/Sema/attr-btf_tag.c

13

apply to function argument. In dwarf, it applies to DW_TAG_member.

[aaron.ballman]

This currently has no codegen, so it only adds the attribute to the AST and does nothing with it. Can you also add the codegen components to this patch?

clang/include/clang/Basic/Attr.td

1835

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?

clang/include/clang/Basic/AttrDocs.td

2019

2020

clang/test/Sema/attr-btf_tag.c

15

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?

[yonghong-song]

This currently has no codegen, so it only adds the attribute to the AST and does nothing with it. Can you also add the codegen components to this patch?

The code patches to generate IR are below:

https://reviews.llvm.org/D106620 (generate btf_tag annotations for func parameters)
https://reviews.llvm.org/D106619 (generate btf_tag annotations for DIGlobalVariable)
https://reviews.llvm.org/D106618 (generate btf_tag annotations for DISubprogram types)
https://reviews.llvm.org/D106616 (generate btf_tag annotations for DIDerived types)
https://reviews.llvm.org/D106615 (generate btf_tag annotations for DIComposite types)

The dwarf generation patch:

https://reviews.llvm.org/D106621 (Support new TAG DW_TAG_LLVM_annotation)

In one of my early PIC patches, David Blaikie suggested to break into manageable pieces
for review and that is why I have multiple patches instead of one giant one. Please let
me know if you have better suggestions.

[yonghong-song]

clang/include/clang/Basic/Attr.td

1835

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.

clang/include/clang/Basic/AttrDocs.td

2019

thanks! Will change.

2020

Thanks. Will change.

clang/test/Sema/attr-btf_tag.c

15

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.

[aaron.ballman]

In D106614#2912292, @yonghong-song wrote:

This currently has no codegen, so it only adds the attribute to the AST and does nothing with it. Can you also add the codegen components to this patch?

In one of my early PIC patches, David Blaikie suggested to break into manageable pieces
for review and that is why I have multiple patches instead of one giant one. Please let
me know if you have better suggestions.

Ah, nope, this is fine -- I had missed noticing that this was part of a patch stack.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

1835

Great, thank you!

clang/test/Sema/attr-btf_tag.c

15

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.

[yonghong-song]

address Aaron and Andrii's comments:

• warned and ignored if a btf_tag attribute is added to record forward declaration.
• warned and ignored if a btf_tag attribute is in a prototype declaration but that attribute is not in later redeclaration or definition
• added tests to cover the above warn situations and unsupported declaration like enum
• fixed some missing points and formats in docs.

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

2019

2021

clang/lib/Sema/SemaDeclAttr.cpp

6763

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.)

clang/test/Sema/attr-btf_tag.c

32

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.)

[yonghong-song]

clang/lib/Sema/SemaDeclAttr.cpp

6763

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.

clang/test/Sema/attr-btf_tag.c

32

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?

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

6763

I kind of thought that might be the case. I think you'll want to also test !Rec->isBeingDefined()

clang/test/Sema/attr-btf_tag.c

32

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!

Oh, that's... interesting! You want to accept multiple, potentially separate attribute specifiers on the initial declaration, but not allow any new attributes on redeclarations or the definition. Btw, there are some tests missing from your above comment. You also need to think about:

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void bar(); // succeed

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // succeed

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void __attribute__((btf_tag("one"))) bar(); // fail

void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // ok
void __attribute__((btf_tag("one"), btf_tag("two"), btf_tag("three"))) bar(); // fail

[[clang::btf_tag("one")]] void bar [[clang::btf_tag("two")]] (); // ok
void __attribute__((btf_tag("one"), btf_tag("two"))) bar(); // succeed

I don't think we have any attributes that behave like that currently. Can you remind me: what is the concern if the attributes are additive on redeclaration?

[yonghong-song]

clang/test/Sema/attr-btf_tag.c

32

Can you remind me: what is the concern if the attributes are additive on redeclaration?

I guess my main concern is people may look at one place for attribute availability. They may find it not there but actually it is defined in a different places. But in practice this may not be an issue as people typically will put attributes in the header *unique* declaration and/or the later *unique* definition.

Let me just to use additive approach which will simplify the whole thing a lot.

[anakryiko]

I think it's reasonable behavior to just merge all bpf_tags across declaration(s) and a definition. Kind of like __weak behaves, it doesn't matter if it's on actual function or its extern declaration.

[yonghong-song]

• for redeclarations, use additive approach to accumulate all btf_tag attributes and apply accumulated attributes to the redeclaration.

[yonghong-song]

besides the clang test in this patch, there are more clang tests in the following patches (involving codegen):

https://reviews.llvm.org/D106615 (for struct/union types)
https://reviews.llvm.org/D106618 (for functions)
https://reviews.llvm.org/D106619 (for global variables)

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

6768–6769

This should diagnose that the attribute is being ignored due to the mismatch in tags (and probably have a note to the previous declaration as well so users can see both sides of the conflict).

6775–6776

This should diagnose as well when ignoring the attribute.

[yonghong-song]

clang/lib/Sema/SemaDeclAttr.cpp

6768–6769

Just to make sure we are on the same page. The attribute is ignored because it has been defined in the declaration. This is specifically to handle cases like:

#define __tag1 __attribute__((btf_tag("info")))
#define __tag2 __attribute__((btf_tag("info2")))
int var __tag1 __tag1, __tag2;

The first tag1 will be added to declaration successfully, but the second tag1 will be ignored since there exists an identical one. __tag2 will be added to the declaration successfully.

I think handleBTFTagAttr() does not handle merging declarations? It is mergeBTFTagAttr below to handle merging? If handleBTFTagAttr() is to handle merging declarations, it will handle attributes the same as below mergeBTFTagAttr, i.e., merging all attributes at the same time doing dedup.

Do you want issue an warning for ignored attribute?

6775–6776

Okay will do.

[aaron.ballman]

Assuming we're back on the same page again, I think the only thing left in this review is a commenting nit.

clang/lib/Sema/SemaDeclAttr.cpp

6768–6769

ohhh, I think we weren't on the same page, sorry about that! I had the semantics wrong in my head -- I was thinking we wanted to reject different tags, but it's the opposite, we want to allow multiple tags so long as they have different arguments. Assuming I have that correct now, this approach looks correct to me (without diagnosing the ignored duplicates).

[yonghong-song]

Assuming I have that correct now, this approach looks correct to me (without diagnosing the ignored duplicates).

I agree that we don't need to diagnose the ignored duplicates. The following is an example,

$ cat t.c
int g attribute((section("a"))) attribute((section("a")));
$ clang -g -c t.c -Wall -Werror
$

duplicates are silently ignored and there is no warning.

A warning/error will be issued if two section names are different.
$ cat t.c
int g attribute((section("a"))) attribute((section("b")));
$ clang -g -c t.c -Wall -Werror
t.c:1:22: error: section does not match previous declaration [-Werror,-Wsection]
int g attribute((section("a"))) attribute((section("b")));

^

t.c:1:52: note: previous attribute is here
int g attribute((section("a"))) attribute((section("b")));

^

1 error generated.
$

So for btf_tag attributes, since we just ignore duplicated attributes, we should be fine without emitting any warn/error messages.

Assuming we're back on the same page again, I think the only thing left in this review is a commenting nit.

There are a couple of nits in AttrDocs.td. Will fix them and resubmit the patch.
Thanks for your careful review!

[yonghong-song]

• fix a few nits in AttrDocs.td

[aaron.ballman]

LGTM, thank you!

clang/include/clang/Basic/Attr.td

 def BPFPreserveAccessIndex : InheritableAttr,
                              TargetSpecificAttr<TargetBPF>  {
   let Spellings = [Clang<"preserve_access_index">];
   let Subjects = SubjectList<[Record], ErrorDiag>;
   let Documentation = [BPFPreserveAccessIndexDocs];
   let LangOpts = [COnly];
 }
 
+def BTFTag : InheritableAttr {
+  let Spellings = [Clang<"btf_tag">];
+  let Args = [StringArgument<"BTFTag">];
+  let Subjects = SubjectList<[Var, Function, Record, Field], ErrorDiag>;
+  let Documentation = [BTFTagDocs];
+  let LangOpts = [COnly];
+}
+
 def WebAssemblyExportName : InheritableAttr,
                             TargetSpecificAttr<TargetWebAssembly> {
   let Spellings = [Clang<"export_name">];
   let Args = [StringArgument<"ExportName">];
   let Documentation = [WebAssemblyExportNameDocs];
   let Subjects = SubjectList<[Function], ErrorDiag>;
 }
 

clang/include/clang/Basic/AttrDocs.td

def BPFPreserveAccessIndexDocs : Documentation {
   let Content = [{
 Clang supports the ``__attribute__((preserve_access_index))``
 attribute for the BPF target. This attribute may be attached to a
 struct or union declaration, where if -g is specified, it enables
 preserving struct or union member access debuginfo indices of this
 struct or union, similar to clang ``__builtin_preserve_access_index()``.
   }];
 }
+def BTFTagDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Clang supports the ``__attribute__((btf_tag("ARGUMENT")))`` attribute for all
+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
+will be emitted to .BTF ELF section too.
+  }];
+}
 
 def MipsInterruptDocs : Documentation {
   let Category = DocCatFunction;
   let Heading = "interrupt (MIPS)";
   let Content = [{
 Clang supports the GNU style ``__attribute__((interrupt("ARGUMENT")))`` attribute on
 MIPS targets. This attribute may be attached to a function definition and instructs
 the backend to generate appropriate function entry/exit code so that it can be used

clang/lib/Sema/SemaDeclAttr.cpp

 
 static void handleBPFPreserveAccessIndexAttr(Sema &S, Decl *D,
     const ParsedAttr &AL) {
   auto *Rec = cast<RecordDecl>(D);
   handleBPFPreserveAIRecord(S, Rec);
   Rec->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, AL));
 }
 
+static void handleBTFTagAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  StringRef Str;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
+    return;
+
+  D->addAttr(::new (S.Context) BTFTagAttr(S.Context, AL, Str));
+}
+
 static void handleWebAssemblyExportNameAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (!isFunctionOrMethod(D)) {
     S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
         << "'export_name'" << ExpectedFunction;
     return;
   }
 
   auto *FD = cast<FunctionDecl>(D);
   if (FD->isThisDeclarationADefinition()) {
     S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;
     return;
   }
 
   StringRef Str;
   SourceLocation ArgLoc;
   if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &ArgLoc))
     return;
 
   D->addAttr(::new (S.Context) WebAssemblyExportNameAttr(S.Context, AL, Str));
   D->addAttr(UsedAttr::CreateImplicit(S.Context));
 }
case ParsedAttr::AT_AMDGPUNumVGPR:
     handleAMDGPUNumVGPRAttr(S, D, AL);
     break;
   case ParsedAttr::AT_AVRSignal:
     handleAVRSignalAttr(S, D, AL);
     break;
   case ParsedAttr::AT_BPFPreserveAccessIndex:
     handleBPFPreserveAccessIndexAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_BTFTag:
+    handleBTFTagAttr(S, D, AL);
+    break;
   case ParsedAttr::AT_WebAssemblyExportName:
     handleWebAssemblyExportNameAttr(S, D, AL);
     break;
   case ParsedAttr::AT_WebAssemblyImportModule:
     handleWebAssemblyImportModuleAttr(S, D, AL);
     break;
   case ParsedAttr::AT_WebAssemblyImportName:
     handleWebAssemblyImportNameAttr(S, D, AL);

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: Annotate ()
 // CHECK-NEXT: AnyX86NoCfCheck (SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: ArcWeakrefUnavailable (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ArmBuiltinAlias (SubjectMatchRule_function)
 // CHECK-NEXT: AssumeAligned (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: Assumption (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Availability ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_implementation, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
 // CHECK-NEXT: BPFPreserveAccessIndex (SubjectMatchRule_record)
+// CHECK-NEXT: BTFTag (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record, SubjectMatchRule_field)
 // CHECK-NEXT: BuiltinAlias (SubjectMatchRule_function)
 // CHECK-NEXT: CFAuditedTransfer (SubjectMatchRule_function)
 // CHECK-NEXT: CFConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: CFICanonicalJumpTable (SubjectMatchRule_function)
 // CHECK-NEXT: CFUnknownTransfer (SubjectMatchRule_function)
 // CHECK-NEXT: CPUDispatch (SubjectMatchRule_function)
 // CHECK-NEXT: CPUSpecific (SubjectMatchRule_function)
 // CHECK-NEXT: CUDAConstant (SubjectMatchRule_variable)

clang/test/Sema/attr-btf_tag.c

+// RUN: %clang_cc1 -x c -triple x86_64-pc-linux-gnu -dwarf-version=4 -fsyntax-only -verify %s
+
+#define __tag __attribute__((btf_tag("tag")))
+#define __invalid __attribute__((btf_tag(1)))
+
+struct t1 {
+  int a __tag;
+} __tag;
+
+int g1 __tag;
+int i1 __invalid; // expected-error {{'btf_tag' attribute requires a string}}
+
+int __tag foo(struct t1 __tag *arg) {
+  return arg->a;
+}