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