Page MenuHomePhabricator

[WIP] BPF: add support for DWARF_AT_LLVM_annotations attribute
Needs ReviewPublic

Authored by yonghong-song on Jun 3 2021, 9:29 PM.

Details

Reviewers
dblaikie
Summary

Add a new LLVM custom attribute DWARF_AT_LLVM_annotation
so attributes with attribute((annotate("..."))) can be
passed to the dwarf debug_info. Please see
https://reviews.llvm.org/D103549 on why BPF community
wants this feature.

This patch added supports for annotation attributes
for global/local variables (including parameters),
static/global functions, record (struct/union) types
and record fields.

This is still work-in-progress. I would like to get feedback
on my current approach.

First, I named the new attribute as DWARF_AT_LLVM_annotations.
I avoided to use BTF/BPF in the name as this doesn't need
to be BPF specific and some other tools might find this useful
in the future.

Second, we don't want to emit DWARF_AT_LLVM_annotations by
default. Two reasons, first, the initial implementation will
be incomplete and heavily geared to BPF use case; second,
this is unlikely to be used outside BPF community initially.
So exposing to other users sounds unnecessary. So I added
a flag -gannotations to control the feature.
If -gannotations is not specified, DWARF_AT_LLVM_annotations
will not be generated.

Third, it is desirable to add IR/BitCode read/write support for
annotations. This will help writing tests too. I only
implemented for DIGlobalVariable. I am pretty sure that
my implementation may have some issues. It
be good to get some feedback before I continues.

Diff Detail

Event Timeline

yonghong-song created this revision.Jun 3 2021, 9:29 PM
yonghong-song requested review of this revision.Jun 3 2021, 9:29 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 3 2021, 9:29 PM

Is the C source syntax already fixed? Could it use a more specific name - then maybe wouldn't need the flag to opt-in if the only places the attribute appears is where it should be propagated?

This'll end up being broken down into many smaller patches (IR+LangRef+Bitcode support for each attribute separately, ideally, I think, for instance - separate patches for frontend generation and backend use in DWARF emission).

Probably worth an llvm-dev thread with the usual suspects (@probinson @aprantl @JDevlieghere and myself at least) - laying out the IR changes and DWARF changes (the name of the new attribute, the DIEs it'll appear on).

One thing that might be worth hashing out is whether this is DW_AT_LLVM or DW_AT_BTF (or BPF, not sure which one applies here) - whether it should be general for other uses. Given how opaque it is, I'm sort of hesitant to suggest it has a really general use - then it gets really fuzzy who's using it for what, what format it has, how to interpret it, etc. So I'd sort of be inclined towards naming it something BPF-specific, for instance.

Is the C source syntax already fixed? Could it use a more specific name - then maybe wouldn't need the flag to opt-in if the only places the attribute appears is where it should be propagated?

No. C source syntax is not fixed. The idea is to pass attributes to functions/variables/structures/fields to dwarf. We can discuss what is the best way to achieve this. Definitely we can change dwarf attribute name
or even the attribute name. If we only focus on BPF use cases (C only), yes, we really only have a few places and opt-in should be fine as I don't think a lot of people use annotate attributes.

This'll end up being broken down into many smaller patches (IR+LangRef+Bitcode support for each attribute separately, ideally, I think, for instance - separate patches for frontend generation and backend use in DWARF emission).

Thanks. This patch is a mix trying to show what the end result could be. Once we agreed on syntax and scope of the attribute, I can break it into multiple patches.

Probably worth an llvm-dev thread with the usual suspects (@probinson @aprantl @JDevlieghere and myself at least) - laying out the IR changes and DWARF changes (the name of the new attribute, the DIEs it'll appear on).

Yes, it is a good idea to discuss on llvm-dev instead here to hash out design issues. Will post in the next few days.

One thing that might be worth hashing out is whether this is DW_AT_LLVM or DW_AT_BTF (or BPF, not sure which one applies here) - whether it should be general for other uses. Given how opaque it is, I'm sort of hesitant to suggest it has a really general use - then it gets really fuzzy who's using it for what, what format it has, how to interpret it, etc. So I'd sort of be inclined towards naming it something BPF-specific, for instance.

That is all good points. Agreed that we don't want to generalize it for the sake of just generalization. I am okay either way. We can discuss more llvm-dev mailing list.

ormris removed a subscriber: ormris.Jun 11 2021, 2:57 PM