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

[Jose E. Marchesi via cfe-commits]

> 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 account in reviews.llvm.org.

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

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

[Jose E. Marchesi via cfe-commits]

> 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 think we discussed this particular case, and I'm not sure whether 
>> there is any precedent here.
>>
>> WDYT @eddyz87, @jemarch ?
>
> This is related to my previous comment. Kernel BTF parser wants all
> type tags to precede modifiers (see this
> 
> code). So such reordering will have to happen either during DWARF
> generation or by `pahole`. Currently `pahole` is "dumb" in a sense
> that it reflects DWARF structure in BTF effectively one-to-one. Adding
> such rewrite at a `pahole` level is possible but one-to-one mapping to
> DWARF would be lost, so the tool would require significant changes.
>
> So, at least for LLVM its a tradeoff, either do it in the compiler
> (less code), or in the pahole (more code). I opted for less code :) If
> you think that this is conceptually unacceptable, I'll do the change
> on the pahole side (DI maintainers on LLVM side might agree, the patch
> has not been reviewed yet).
>
> Another option would be to modify the kernel, but this might impede backwards 
> compatibility.

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 we wanted to use a different approach in
DWARF than in BTF.

I think the question is: what are the consequences of having the
annotation DIE as a child of the qualifiers instead of the qualified
type?

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

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

[Jose E. Marchesi via cfe-commits]

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

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.

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.

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


> I'm aware of three use-cases for "btf_type_tag": "percpu", "user",
> "rcu" marks in kernel code. First is used for per-cpu variables,
> second is used to mark pointers to userspace memory, third to mark the
> data that should be read with RCU lock. For these use-cases there is
> no difference between e.g. `const __user *ptr` and `__user const
> *ptr`. I don't think the difference was ever thought through.
>
>> 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?
>>
>> I

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

[Jose E. Marchesi via cfe-commits]

> 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 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.
>
> But that's just an encoding quirk. For the C language "const volatile
> int *" and "volatile const int *" have the same meaning. In clang AST
> such qualifiers are represented as bitfields.
>
> 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.

>>> 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):
>>
>>   0x001e:   DW_TAG_variable
>>   DW_AT_name ("x")
>>   DW_AT_type (0x0029 "const volatile int")
>>   
>>   0x0029:   DW_TAG_const_type
>>   DW_AT_type (0x002e "volatile int")
>>   
>>   0x002e:   DW_TAG_volatile_type
>>   DW_AT_type (0x0033 "int")
>>   
>>   0x0033:   DW_TAG_base_type
>>   DW_AT_name ("int")
>>   
>>   0x0037:   DW_TAG_variable
>>   DW_AT_name ("y")
>>   DW_AT_type (0x0029 "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.
>
> Unfortunately, for ordering we are also limited by a consumer. In the
> kernel case the order

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

[Jose E. Marchesi via cfe-commits]

> 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 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?

> @yonghong-song, @anakryiko, what do you think?
>
>
> 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

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

[Jose E. Marchesi via cfe-commits]

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

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:

  var1 -> const -> typedef (bar) -> int
   |   ^
  __tag1   |
   |
  var2 +

If we would allow __tag1 to be "moved" to either the typedef DWARF node
or the node for int like this:

  DWARF:

  var1 -> const -> typedef (bar) -> int
   ^ |
   |  __tag1
  var2 +

Then the __tag1 would also apply to var2's type.  But I would say in the
C snippet above __tag1 should apply to the type of var1, but not
to the type of var2.

This makes me think we shouldn't allow tags to "jump" over typedef
nodes, in neither DWARF nor BTF.

PS: I am a bit concerned with the fact that the kernel's interpretation
of BTF is so rigit in this case as to assume C's type system
semantics when it comes to type qualifiers.  Other languages may be
coming to the BPF world (Rust, for example) and in these languages
the ordering of type qualifiers may actually matter.  There is a
reason why DWARF encodes qualifiers as explicit nodes in the type
chain rather than as attributes of the type nodes.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jose E. Marchesi via cfe-commits]

> 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:
>>   
>>   var1 -> const -> typedef (bar) -> int
>>|   ^
>>   __tag1   |
>>|
>>   var2 +
>>
>> If we would allow __tag1 to be "moved" to either the typedef DWARF node
>> or the node for int like this:
>>
>>   DWARF:
>>   
>>   var1 -> const -> typedef (bar) -> int
>>^ |
>>|  __tag1
>>   var2 +
>>
>> Then the __tag1 would also apply to var2's type.  But I would say in the
>> C snippet above __tag1 should apply to the type of var1, but not
>> to the type of var2.
>
> I'm not sure I understand, the DWARF #2 is not a valid representation
> of the program for all the reasons you write.  Current LLVM
> implementation should generate DWARF #1 in this case.

Yes that was my point: tags can cross qualifiers and still keep C
language semantics, but not (all) typedefs.

> 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
>  |
>   __tag1
>   
>   var2 --> typedef (bar) -> int
>
> (and it is what needs to be implemented in pahole to get BTF
> qualifiers ordering expected by kernel, but the move is in the
> opposite direction).

So the kernel expects tags to precede typedefs as well as qualifiers?
i.e. given this DWARF:

   var1 -> const -> typedef (bar) -> int
^ |
|   __tag1
|
   var2 +


We have to transform to these two BTF type chains:

var1 -> __tag1 -> const -> typedef (bar) -> int
^
|
var2 -> __tag1 -+

Correct?

>> PS: I am a bit concerned with the fact that the kernel's interpretation
>>
>>   of BTF is so rigit in this case as to assume C's type system
>>   semantics when it comes to type qualifiers.  Other languages may be
>>   coming to the BPF world (Rust, for example) and in these languages
>>   the ordering of type qualifiers may actually matter.  There is a
>>   reason why DWARF encodes qualifiers as explicit nodes in the type
>>   chain rather than as attributes of the type nodes.
>
> Need to read a bit about Rust, can't comment right now.
>
>
> 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

[clang] [llvm] [BPF] Do atomic_fetch_*() pattern matching with memory ordering (PR #107343)

[Jose E. Marchesi via cfe-commits]

jemarch wrote:


> Merged #107343 into main.

I am preparing the corresponding patch for GCC.


https://github.com/llvm/llvm-project/pull/107343
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits