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

[Eduard Zingerman via Phabricator via cfe-commits]

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 independently) - and then at the end you copy any of these types that are needed and put the annotations on them?

Yes.

> Does the BTFTagAttributedType always have to apply to the immediate type that's going to be modified/mutated to have the attributes applied to it directly? Is the set of types that may have these attributes/annotations added small/closed? (struct types, void, anything else? could you add tags to int __tag *, for instance and get a DW_TAG_base_type for "int" with annotations on it?

We decided against applying it to const/volatile/restrict, but it can be applied to struct types, void, "base" types ("int"), pointers and typedefs.

> If it's really generic/can apply to any type - but always the /immediate/ type (I guess with the special handling you've got to skip applying it to the DW_TAG_const_type, etc)...
>
> What if you skipped getOrCreateType - and called into the CreateType dispatch below that? Since you never want a cached instance of a type, right? You want to create a new copy of the type you could then apply annotations to.
>
> Except, I guess, in the instance you showed, where the type is being completed - can't go and create another one, because we're part-way through building this one... hmm, maybe you can, though? What happens if we start making a totally distinct copy of that same type? I guess there's some map that contains such partially completed types, and that map would get confused/break if we tried to build two types for the same type without adding in some extra key goo that contained the annotations... - maybe that wouldn't be too invasive, though?

I made a prototype of such change, available here <https://github.com/llvm/llvm-project/commit/5ad452a913cd102d38deec130a09a9e5cc2d2b30#diff-ad303855624f31f09513ee5d6e5b435714d874b6d6c2afcb140b72a4b61a9166R1257>. The interesting function is `CGDebugInfo::CreateType(const BTFTagAttributedType *Ty, ...)`.
Pseudo code for old version (this revision):

  def CGDebugInfo::CreateType(const BTFTagAttributedType *Ty, ...):
    WrappedTy, Annotations = collect annotations are base type from Ty
    WrappedDI = getOrCreateType(WrappedTy, ...)
    Placeholder = create temporary placeholder node with
                  references to WrappedDI and Annotations
    AnnotationPlaceholders.push_back(Placeholder)
    return Placeholder
    
  def CGDebugInfo::finalize():
    ...
    for Placeholder in AnnotationPlaceholders:
      T = Placeholder.WrappedDI.Clone()
      T.replaceAnnotations(Placeholder.Annotations)
      replace Placeholder with T

Pseudo code for new version (link to my github from above):

  def CGDebugInfo::CreateType(const BTFTagAttributedType *Ty, ...):
    WrappedTy, Annotations = collect annotations are base type from Ty
    Placeholder = empty temporary node
    TypeCache[Ty] = Placeholder
    WrappedDI = CreateType(UnwrapTypeForDebugInfo(WrappedTy), ...)
    T = WrappedDI.Clone()
    T.replaceAnnotations(Annotations)
    replace Placeholder with T
    return T

Comparing the "old" and "new" versions "old" appears to be less hacky to me: it does not break `getOrCreateType()` abstraction and it does not process members two times for circular types.

Note that this <https://github.com/llvm/llvm-project/commit/5ad452a913cd102d38deec130a09a9e5cc2d2b30#diff-ad303855624f31f09513ee5d6e5b435714d874b6d6c2afcb140b72a4b61a9166R2805> diff is important for the new version, without it `CreateType` won't actually create a new object in the circular type case. I checked call-graph for `CreateType(RecordType *)` and it looks like it is only invoked from `getOrCreateType()`, so `getTypeOrNull()` within it is not actually needed. It was added in commit 3b1cc9b85846 back in 2013. Test cases for that commit are passing.

> given this code:
>
>   #define __tag1 __attribute__((btf_type_tag("tag1")))
>   
>   struct st {
>     struct st *self;
>   };
>   struct st __tag1 x;
>
> What's the expected DWARF here? x's type is a the attributed/annotated st type, but then does the self pointer inside there point to the attributed type or the unattributed type?

The `self` should have type w/o tag, the `x` should have type with tag.

> Maybe what you've got is the best of some bad options, but I'm not sure/trying to think it through. (@aprantl wouldn't mind your perspective - if it's too much to consume easily, let me know and I can make a best-effort at summarizing, maybe even better over a video chat if you've got a few minutes)

I appreciate the effort, if you and @aprantl decide that the call is necessary I can attend if you need me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967