Page MenuHomePhabricator

[Clang] add btf_tag attribute
ClosedPublic

Authored by yonghong-song on Jul 22 2021, 5:15 PM.

Details

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.

Diff Detail

Event Timeline

yonghong-song created this revision.Jul 22 2021, 5:15 PM
yonghong-song requested review of this revision.Jul 22 2021, 5:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2021, 5:15 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
anakryiko added inline comments.Jul 27 2021, 3:00 PM
clang/include/clang/Basic/AttrDocs.td
2019

can it be also applied to function's argument?

clang/test/Sema/attr-btf_tag.c
14

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

yonghong-song added inline comments.Jul 27 2021, 3:57 PM
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
14

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

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
1841

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
16

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?

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 added inline comments.Jul 28 2021, 9:21 PM
clang/include/clang/Basic/Attr.td
1841

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
16

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.

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 added inline comments.Jul 29 2021, 5:15 AM
clang/include/clang/Basic/Attr.td
1841

Great, thank you!

clang/test/Sema/attr-btf_tag.c
16

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 edited the summary of this revision. (Show Details)

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 added inline comments.Aug 2 2021, 11:18 AM
clang/include/clang/Basic/AttrDocs.td
2019
2021
clang/lib/Sema/SemaDeclAttr.cpp
6852

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 added inline comments.Aug 3 2021, 12:01 AM
clang/lib/Sema/SemaDeclAttr.cpp
6852

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 added inline comments.Aug 3 2021, 5:17 AM
clang/lib/Sema/SemaDeclAttr.cpp
6852

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 added inline comments.Aug 3 2021, 8:25 AM
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.

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 edited the summary of this revision. (Show Details)
  • for redeclarations, use additive approach to accumulate all btf_tag attributes and apply accumulated attributes to the redeclaration.

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 added inline comments.Aug 9 2021, 1:27 PM
clang/lib/Sema/SemaDeclAttr.cpp
6857–6858

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

6864–6865

This should diagnose as well when ignoring the attribute.

yonghong-song added inline comments.Aug 9 2021, 11:51 PM
clang/lib/Sema/SemaDeclAttr.cpp
6857–6858

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?

6864–6865

Okay will do.

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
6857–6858

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

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!

  • fix a few nits in AttrDocs.td
aaron.ballman accepted this revision.Aug 12 2021, 4:11 AM

LGTM, thank you!

This revision is now accepted and ready to land.Aug 12 2021, 4:11 AM
This revision was landed with ongoing or failed builds.Aug 12 2021, 4:34 PM
This revision was automatically updated to reflect the committed changes.
acme added a subscriber: acme.Wed, Sep 8, 7:12 AM