[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 <https://reviews.llvm.org/D143967#4220233>, @jemarch wrote:

>> eddyz87 added a comment.
>>
>> In D143967#4200331 <https://reviews.llvm.org/D143967#4200331> https://reviews.llvm.org/D143967#4200331, @dfaust wrote:
>>
>>> In D143967#4197288 <https://reviews.llvm.org/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 for normal qualifiers:

const volatile int x;
volatile const int y;

Both compilers emit something like this ('x' and 'y' share type):

  0x0000001e:   DW_TAG_variable
                  DW_AT_name	("x")
                  DW_AT_type	(0x00000029 "const volatile int")
  
  0x00000029:   DW_TAG_const_type
                  DW_AT_type	(0x0000002e "volatile int")
  
  0x0000002e:   DW_TAG_volatile_type
                  DW_AT_type	(0x00000033 "int")
  
  0x00000033:   DW_TAG_base_type
                  DW_AT_name	("int")
  
  0x00000037:   DW_TAG_variable
                  DW_AT_name	("y")
                  DW_AT_type	(0x00000029 "const volatile int")

Interestingly, GCC and LLVM do not behave exactly the same in that in 
GCC the DWARF types for both variables are

  x, y   -> volatile -> const -> int

while in LLVM (shown above) it is

  x, y -> const -> volatile -> int

So I'm not sure we necessarily need both compilers to be exactly the same.

> In case two identical annotations are applied to the same type, are we
> deduplicating them?  Should we actually require that?  Even if we don't,
> we probably want to behave the same in both compilers.

FWIW, if you write something like:

  int __attribute__((btf_type_tag("__c"))) __attribute__((btf_type_tag("__c"))) c;

GCC de-duplicates attributes with the same value, so the tag "__c" will only appear
once. I don't know if this would be easy to change if we wanted to.

> (Note that since DW_TAG_typedef is also a type in the type chain, we
>  shouldn't worry about

Looks like this got cutoff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967