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

[David Faust via Phabricator via cfe-commits]

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
> DW_AT_type  (0x45 "int")
>   
>   0x41: DW_TAG_LLVM_annotation
>   DW_AT_name("btf:type_tag")
>   DW_AT_const_value ("__b")

I am not sure about this case. Do we want the tag on the CVR qualifier or on 
the type being qualified?

Current patches for GCC place the annotation DIE as child of the 'int':

  0x001e:   DW_TAG_variable
  DW_AT_name("b")
  DW_AT_type(0x0047 "volatile int")
  
  0x0032:   DW_TAG_base_type
  DW_AT_byte_size   (0x04)
  DW_AT_name("int")
  
  0x003d: DW_TAG_LLVM_annotation
DW_AT_name  ("btf:type_tag")
DW_AT_const_value   ("__b")
  
  0x0047:   DW_TAG_volatile_type
  DW_AT_type(0x0032 "int")

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 think we discussed this particular case, and I'm not sure whether there 
is any precedent here.

WDYT @eddyz87, @jemarch ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143967/new/

https://reviews.llvm.org/D143967

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Faust via Phabricator via cfe-commits]

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 ok with this.

Ok I was not aware of that requirement.
Internally GCC converts DWARF representation to BTF for BTF emission so we get 
(for the volatile example):

  [1] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
  [2] TYPE_TAG '__b' type_id=1
  [3] VOLATILE '(anon)' type_id=2
  [4] VAR 'b' type_id=3, linkage=global

i.e. `VOLATILE -> TYPE_TAG -> INT` which doesn't meet the above requirement 
that type tags precede CVR modifiers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143967/new/

https://reviews.llvm.org/D143967

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Faust via Phabricator via cfe-commits]

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 
problematic?
As far as I know it would be "ok" in the sense that it should not break DWARF 
or cause issues for DWARF consumers which do not know about the tags.

My initial reaction was that placing the annotations on CVR modifiers makes 
less sense conceptually than placing them on the types proper.
But, I suppose it is a question of what precisely the annotated type is. e.g. 
in `volatile int __attribute__((btf_type_tag("__b"))) b;` is it the case that:

- type tag "__b" applies to the type `volatile int` (i.e. cvr quals  "bind more 
closely" than the tag), or
- type tag "__b" and `volatile` both apply to `int` (i.e. cvr quals and type 
tags are "equal")

For all other cases the new "btf:type_tag" format places the annotation as a 
child of exactly the type that is annotated, so I think the answer determines 
where the annotation logically belongs in DWARF:

- If the tag applies to `volatile int` then it should be a child of the 
volatile DIE.
- If the tag applies to `int` it should be a child of the integer type DIE.

At the moment I can't say that one is more correct than the other, so I guess I 
have no real objection placing the annotation on the CVR modifier.

> - technical: what is the point where such reordering is easiest to implement? 
> For LLVM / pahole stack the path of least resistance is to modify DWARF 
> generation (but again @dblaikie might disagree). How hard is it to adjust DI 
> generation in GCC in this way?

It is not a difficult change in GCC. Some added complexity but not too much. I 
have a prototype that seems to work from initial testing.
It is probably simpler than instead modifying the internal transformation to 
BTF to reorder tags/CVR qualifiers as kernel currently requires.

But I don't think ease of implementation should be a factor unless we are sure 
the format makes sense.
We are changing the format because the old one was flawed, let's try to make 
sure we aren't just replacing it with something flawed in a different way 
because it is easy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143967/new/

https://reviews.llvm.org/D143967

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Faust via Phabricator via cfe-commits]

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  
>>> https://reviews.llvm.org/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 
>>> problematic?
>>> As far as I know it would be "ok" in the sense that it should not
>>> break DWARF or cause issues for DWARF consumers which do not know
>>> about the tags.
>>>
>>> My initial reaction was that placing the annotations on CVR
>>> modifiers makes less sense conceptually than placing them on the
>>> types proper.
>>
>> I agree conceptually.
>
> That was my initial reaction too.  But see below.
>
>>> But, I suppose it is a question of what precisely the annotated type
>>> is. e.g. in `volatile int __attribute__((btf_type_tag("__b"))) b;`
>>> is it the case that:
>>>
>>> - type tag "__b" applies to the type `volatile int` (i.e. cvr quals  "bind 
>>> more closely" than the tag), or
>>> - type tag "__b" and `volatile` both apply to `int` (i.e. cvr quals and 
>>> type tags are "equal")
>>>
>>> For all other cases the new "btf:type_tag" format places the
>>> annotation as a child of exactly the type that is annotated, so I
>>> think the answer determines where the annotation logically belongs
>>> in DWARF:
>>>
>>> - If the tag applies to `volatile int` then it should be a child of the 
>>> volatile DIE.
>>> - If the tag applies to `int` it should be a child of the integer type DIE.
>
> I think that is the main point.  DWARF composes types by chaining DIEs
> via DW_AT_type attributes, like in:
>
>   const -> volatile -> pointer-to -> int
>
> Conceptually, there are four different types in that chain, not just
> one:
>
> 1. const volatile pointer-to int
> 2. volatile pointer-to int
> 3. pointer-to int
> 4. int
>
> Ideally speaking, we would have liked to implement the annotations just
> like the qualifiers, so we could have something like:
>
>   const -> volatile -> annotated -> pointer-to -> int
>
> That is what would suite us best conceptually speaking.  But since that
> would break DWARF consumers, we decided to go with a parent-child
> relationship instead, having:
>
>   const -> volatile -> pointer-to -> int
>|
>annotated
>
> Which would be a different situation than, say:
>
>   const -> volatile -> pointer-to -> int
> |
>   annotated
>
> So, I think it only makes sense to have the child annotation node in the
> node that starts the type to which the annotation refers.  Both
> qualifiers and type DIEs are types on that sense.

Ok, I understand your point. 
For example if we have:

  volatile int __tag1 b;

The tag applies to the type (volatile int) and we have 3 types total:

1. __tag1 volatile int
2. volatile int
3. int

And in DWARF the tag must be child of the qualifier:

  var(b) -> volatile -> int
   |
  __tag1
  
   

Doing it the other way would be incorrect - if we instead did this:

  var(b) -> volatile -> int
  |
   __tag1

We encode a different chain of types:

  var(b) -> volatile -> __tag1 -> int

Which reflects a different set of types than the ones we actually have:

1. volatile __tag1 int
2. __tag1 int
3. int

So I guess it is clear, conceptually the tags do belong on the qualifiers.

> And now that I think about this.  In this conceptual situation:
>
>   const -> volatile -> annotated (foo) -> annotated (bar) -> pointer-to -> int
>
> I guess we are encoding it with several DW_TAG_LLVM_annotation children:
>
>   const -> volatile -> pointer-to -> int
>|
>  +-+-+
>  |   |
> anotated (foo)annotated (bar)
>
> Basically accumulating/reducing what are conceptually two different
> types into one.  Since typedefs are explicitly encoded in the chain as
> their own node, this situation will only happen when there are several
> tags specified consecutively in the source code (this is another reason
> why putting the annotation DIEs as children of the qualifiers is
> necessary, because otherwise we would be accumulating/reducing
> annotation across these nodes and we could end with situations where
> annotations get lost.)
>
> When accumulating/reducign tags like above:
>
> Does the ordering matter here?  Does it matter?  Should we require
> keeping the order among the annotation siblings as part of the
> DW_TAG_LLVM_annotation specification?  Even if we don't, we probably
> want to behave the same in both compilers.

A quick check suggests order does not matter for DWARF

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

[David Faust via Phabricator via cfe-commits]

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 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 well, so that the following
 have identical meaning:

 - `__tag volatile int`
 - `volatile __tag int`
>>>
>>> Makes sense.
>>>
>>> But then I think we should allow the tags to be anywhere in the chain in
>>> the DWARF, and not necessarily in the qualified type.  For example
>>> given:
>>>
>>>   typedef const int bar;
>>>   volatile bar __tag1 foo;
>>>
>>> The resulting DWARF type chain is:
>>>
>>>   volatile -> typedef (bar) -> const -> int
>>>
>>> IMO we should allow __tag1 to be a child of any of the elements of the
>>> chain, like:
>>>
>>>   volatile -> typedef (bar) -> const -> int
>>>|
>>>  __tag1
>>>
>>> or
>>>
>>>   volatile -> typedef (bar) -> const -> int
>>>  |
>>>   __tag1
>>>
>>> or
>>>
>>>   volatile -> typedef (bar) -> const -> int
>>>  |
>>>   __tag1
>>>
>>> or
>>>
>>>   volatile -> typedef (bar) -> const -> int
>>>  |
>>>__tag1
>>>
>>> would be all accepted and equivalent.  Also Faust noted that GCC
>>> sometimes optimizes out the typedef nodes, so we need to be able to
>>> place the tags (in the internal representatinos) in the typedef'ed type
>>> instead.
>>
>> Well, it's a logical consequence, I guess.
>> In practice I'm going to emit it like below:
>>
>>   volatile -> const -> int
>> |
>>  __tag1
>
> We can probably do the same in GCC, to be consistent.
>
>> But please remember that in BTF it would have to be encoded like this:
>>
>>   __tag1 -> volatile -> const -> int
>>
>> (that's how kernel expects it, we can change the kernel but will loose
>> backwards compatibility).  For DWARF -> BTF transformation in `pahole`
>> I will make the necessary modifications, but BTF might also be emitted
>> C->BPF backend.
>
> We can do the same thing when we generate BTF directly from GCC.
> Faust, do you agree with the above?

OK, that's fine with me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143967/new/

https://reviews.llvm.org/D143967

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Faust via Phabricator via cfe-commits]

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 format we've been discussing and agreed upon.
Also compared against my WIP patches implementing the same in GCC, and looks 
like it is all consistent (apart from one known bug in my patches)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143967/new/

https://reviews.llvm.org/D143967

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Faust via Phabricator via cfe-commits]

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 type definitions. So, I think type tags 
> should not be moved past typedefs.

Yes I agree. Tags should not be moved past typedefs.
In the GCC implementation we are not moving tags past typedefs.

> It uses `btf_type_is_modifier()` utility function, which treats typedef as a 
> modifier.
>
> So, in theory the transformation moving tags past typedef is necessary. On 
> the other hand, such transformation is not applied now, and this does not 
> cause issues. Which suggests that there are no cases in practice where type 
> tag follows typedef (and thus, this is not required for backwards 
> compatibility).

It seems to me then that the appropriate fix is to remove the expectation of 
this transformation from the verifier (for the reasons above).

If it is not important for backwards compatibility, then I guess it should not 
be problematic to change in the kernel? (I say, as not a kernel hacker...)

I find it odd that `BTF_KIND_TYPEDEF` is included in `btf_type_is_modifier()`, 
seems like the expectation of this transformation is almost an unintended side 
effect of that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143967/new/

https://reviews.llvm.org/D143967

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits