[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. In D143967#4656478 , @dblaikie wrote: > Fair enough - all seems a bit unfortunate to be pushing these attributes > through to places they don't technically apply to (both complicates the > implementation, and might be confusing

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 updated this revision to Diff 558086. eddyz87 marked 2 inline comments as done. eddyz87 added a comment. Rebase, fixes for review feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143967/new/ https://reviews.llvm.org/D143967 Files:

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Fair enough - all seems a bit unfortunate to be pushing these attributes through to places they don't technically apply to (both complicates the implementation, and might be confusing to

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. Hi @dblaikie, If you have some time, could you please take a look at my response ? It would be great if decision on this revision could be reached before next LLVM release. The overall design of this thing was discussed with

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. Hi @dblaikie, If you have some time, could you please take a look at my response ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143967/new/ https://reviews.llvm.org/D143967

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. Hi @dblaikie, Could you please take a look at my response ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143967/new/ https://reviews.llvm.org/D143967

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. Hi @dblaikie Thank you for the detailed response! > So you get some bunch of annotations from the BTFTagAttributedType, then you > build the underlying type (which might already be built/needed/fine because > it's used without attributes in other places/needs to exist

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dblaikie added a subscriber: aprantl. dblaikie added a comment. In D143967#4441333 , @eddyz87 wrote: > In D143967#4438815 , @dblaikie > wrote: > >> I haven't looked closely at this, but from a vague/quick

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. Hi @dblaikie, Could you please take a look at my reply ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143967/new/ https://reviews.llvm.org/D143967

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. Hi @dblaikie , Could you please take a look at my reply here ? It would be really great if this could be sorted out before LLVM 17 release... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 updated this revision to Diff 533746. eddyz87 edited the summary of this revision. eddyz87 added a comment. Rebase: replace usage of removeLocalCVRQualifiers by removeLocalFastQualifiers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. In D143967#4438815 , @dblaikie wrote: > I haven't looked closely at this, but from a vague/quick reading, it sounds > like this is creating annotations on certain type DINodes, but then moving > them around to different types?

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dblaikie added a comment. I haven't looked closely at this, but from a vague/quick reading, it sounds like this is creating annotations on certain type DINodes, but then moving them around to different types? It'd be nice if we could avoid that/create them in the right place in the first

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. Hi @dblaikie , Could you please take a look at this revision (and the previous one in the stack, D143966 ) or suggest some other reviewer familiar with this part of the code base? Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 updated this revision to Diff 523233. eddyz87 added a comment. Fix for case when CVR qualifier is behind type ignored by UnwrapTypeForDebugInfo(). E.g. for the following example: const int *foo; typeof(*foo) __attribute__((btf_type_tag("tag"))) buz; Repository: rG LLVM Github

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. In D143967#4346934 , @dfaust wrote: > In D143967#4343841 , @eddyz87 wrote: > >> Changes to avoid attaching type tags to DWARF derived types for >> const/volatile/restrict qualifiers. > >

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dfaust added a comment. In D143967#4343841 , @eddyz87 wrote: > Changes to avoid attaching type tags to DWARF derived types for > const/volatile/restrict qualifiers. I just tested this locally and can confirm it LGTM in terms of implementing the DWARF

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

yonghong-song added a comment. @dblaikie could you help take a look at this patch? Similar to https://reviews.llvm.org/D143966, this patch is the clang frontend change to support new btf_type_tag format, as we discussed and agreed with gcc community

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 updated this revision to Diff 522338. eddyz87 edited the summary of this revision. eddyz87 added a comment. Changes to avoid attaching type tags to DWARF derived types for const/volatile/restrict qualifiers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 planned changes to this revision. eddyz87 added a comment. In D143967#4312746 , @yonghong-song wrote: > I see the following in the Summary: > > Type tag for CVR modifier type > > C: > > volatile int

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

yonghong-song added a comment. I see the following in the Summary: Type tag for CVR modifier type C: volatile int __attribute__((btf_type_tag("__b"))) b; DWARF: 0x31: DW_TAG_variable DW_AT_name ("b") DW_AT_type (0x3c "volatile int")

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

yonghong-song added a comment. Related to `btf_type_is_modifier()` issue, Not that depending on call site, sometimes typedef is skipped and sometimes are not. So we could keep btf_type_is_modifier() as is and modify the call size to not skip typedef in specific places. Or we could remove

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dfaust added a comment. In D143967#4233742 , @eddyz87 wrote: > Moving type tags past typedefs would also make C code reconstruction from BTF > incomplete. Such reconstruction is used now by e.g. bpftool to create a > vmlinux.h header with all kernel

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. Moving type tags past typedefs would also make C code reconstruction from BTF incomplete. Such reconstruction is used now by e.g. bpftool to create a vmlinux.h header with all kernel type definitions. So, I think type tags should not be moved past typedefs.

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. In D143967#4233414 , @jemarch wrote: >> If some tooling applies such tags movement it should also apply >> appropriate copying of tags, e.g. it should transform DWARF like this: >> >> var1 -> const -> typedef (bar) -> int >>

Re: [PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

> eddyz87 added a comment. > > In D143967#4232089 , @jemarch wrote: > >> Thinking about typedef, some C cases may be problematic if we adopt the >> flexible rule we are discussing: >> >> typedef int bar; >> const bar __tag1 var1; >> bar var2; >>

Re: [PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

> eddyz87 added a subscriber: anakryiko. > eddyz87 added a comment. > > In D143967#4231746 , @jemarch wrote: > >>> eddyz87 added a comment. >>> ... >>> If we consider type tag a qualifier, conceptually it would be simpler >>> if ordering would not matter

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. In D143967#4232089 , @jemarch wrote: > Thinking about typedef, some C cases may be problematic if we adopt the > flexible rule we are discussing: > > typedef int bar; > const bar __tag1 var1; > bar var2; > > DWARF: >

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dfaust added a comment. In D143967#4231911 , @jemarch wrote: >> eddyz87 added a subscriber: anakryiko. >> eddyz87 added a comment. >> >> In D143967#4231746 >> https://reviews.llvm.org/D143967#4231746, @jemarch

Re: [PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

> eddyz87 added a subscriber: anakryiko. > eddyz87 added a comment. > > In D143967#4231746 , @jemarch wrote: > >>> eddyz87 added a comment. >>> ... >>> If we consider type tag a qualifier, conceptually it would be simpler >>> if ordering would not matter

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a subscriber: anakryiko. eddyz87 added a comment. In D143967#4231746 , @jemarch wrote: >> eddyz87 added a comment. >> ... >> If we consider type tag a qualifier, conceptually it would be simpler >> if ordering would not matter for it as

Re: [PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

> eddyz87 added a comment. > > In D143967#4231517 , @dfaust wrote: > >> In D143967#4220233 , @jemarch >> wrote: >> >>> >> >> Ok, I understand your point. >> For example if we have: >> >> volatile int __tag1

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. In D143967#4231517 , @dfaust wrote: > In D143967#4220233 , @jemarch wrote: > >> > > Ok, I understand your point. > For example if we have: > > volatile int __tag1 b; > > The tag

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dfaust added a comment. In D143967#4220233 , @jemarch wrote: >> eddyz87 added a comment. >> >> In D143967#4200331 >> https://reviews.llvm.org/D143967#4200331, @dfaust wrote: >> >>> In D143967#4197288

Re: [PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

> eddyz87 added a comment. > > In D143967#4200331 , @dfaust wrote: > >> In D143967#4197288 , @eddyz87 >> wrote: >> >>> ... >>> I think there are two sides to this: >>> >>> - conceptual: is it ok to allow

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. In D143967#4200331 , @dfaust wrote: > In D143967#4197288 , @eddyz87 wrote: > >> ... >> I think there are two sides to this: >> >> - conceptual: is it ok to allow annotations for CVR

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dfaust added a comment. In D143967#4197288 , @eddyz87 wrote: > ... > I think there are two sides to this: > > - conceptual: is it ok to allow annotations for CVR modifiers? Maybe someone more an expert in DWARF could chime in whether this is

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. > ! In D143967#4197142 , @jemarch > wrote: > ... > In GCC we also translate from the internal DWARF structures into BTF. > So, for us, it would also imply to reoder (before generating the BTF > from the interal DWARF) in case

Re: [PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

> eddyz87 added a comment. > > In D143967#4197041 , @dfaust wrote: > >> The way I see it both 'volatile' and the type tag are modifying >> 'int' type here, so the annotation DIE more properly fits as a child >> of 'int' rather than the 'volatile'. >> >> I

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. In D143967#4197041 , @dfaust wrote: > The way I see it both 'volatile' and the type tag are modifying 'int' type > here, so the annotation DIE more properly fits as a child of 'int' rather > than the 'volatile'. > > I don't

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dfaust added a comment. > in the final BTF, type tags have to precede CVR modifiers, e.g. TYPE_TAG > 'foo' -> CONST -> INT. Right now pahole does not do any reordering, so I > ended up putting the type tag annotations on the DIE with outermost modifier. > Will see if DI maintainers would be

Re: [PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

On 3/15/23 09:17, Eduard Zingerman wrote: > On Wed, 2023-03-15 at 17:10 +0100, Jose E. Marchesi wrote: >>> Could you please take a look to verify that this implementation is on >>> the same page with what is planned for GCC? >> >> Sure. Can you please add David Faust as a subscriber as well? I

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dfaust added a comment. > Type tag for CVR modifier type > > C: > > volatile int __attribute__((btf_type_tag("__b"))) b; > > DWARF: > > 0x31: DW_TAG_variable > DW_AT_name ("b") > DW_AT_type (0x3c "volatile int") > > 0x3c: DW_TAG_volatile_type >

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a comment. Hi @dfaust , Two head-ups, the following items turned out to be important when I tested using kernel BPF testsuite: - in the final BTF, type tags have to precede CVR modifiers, e.g. TYPE_TAG 'foo' -> CONST -> INT. Right now `pahole` does not do any reordering, so I

Re: [PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

On Wed, 2023-03-15 at 17:10 +0100, Jose E. Marchesi wrote: > > Could you please take a look to verify that this implementation is on > > the same page with what is planned for GCC? > > Sure. Can you please add David Faust as a subscriber as well? I don't > know if he has an account in

Re: [PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

> eddyz87 added a subscriber: jemarch. > eddyz87 added a comment. > > Hi @jemarch, > > Could you please take a look to verify that this implementation is on > the same page with what is planned for GCC? Sure. Can you please add David Faust as a subscriber as well? I don't know if he has an

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 added a subscriber: jemarch. eddyz87 added a comment. Hi @jemarch, Could you please take a look to verify that this implementation is on the same page with what is planned for GCC? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143967/new/

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

eddyz87 created this revision. Herald added a subscriber: hiraditya. Herald added a project: All. eddyz87 updated this revision to Diff 497835. eddyz87 added a comment. eddyz87 retitled this revision from "[DebugInfo][BPF] 'annotations' on a target type to represent btf_type_tag" to