[PATCH] D74572: [BPF] preserve debuginfo types for builtin __builtin__btf_type_id()

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D74572#2042931 <https://reviews.llvm.org/D74572#2042931>, @yonghong-song wrote:

> @dblaikie I can revert but let me first understand the alternative way to do the work, at least in high level.
>
> [
>  I do believe my commit message, esp. the first couple of lines is badly inaccurate. I suspect this is the one causing your confusion as well. 
>  specially, In the beginning of commit message, I said:
>
>   u32 btf_type_id = __builtin_btf_type_id(param)
>
> But later on, I also said:
>
>   In the above example, the second argument for __builtin_btf_type_id() is 0, which means a relocation for local adjustment, i.e., w.r.t. bpf program BTF change, will be generated. The value 1 for the second argument means a relocation for remote adjustment, e.g., against vmlinux.
>   
>
> The above statement implies two arguments. 
>  The actually builtin signature should be
>
>   u32 btf_type_id = __builtin_btf_type_id(expr, reloc_type)
>
> The first parameter is not necessary a parameter and it can be any expression.
>  This is my fault as this patch go through several revision and I forgot to change that. I am happy to revert and rework the commit message to make it more clear.
>  ]


Appreciate the clarification - thanks!

> Maybe, the description not clear or too BPF specific. Let me try again to describe the purpose of this builtin.
>  The interface of this builtin likes below:
> 
>   __builtin_btf_type_id(expr, reloc_type)
> 
> The goal is to:
> 
> - the expression here can be a variable (like "a" int "struct { ...} a", or any valid C expression, e.g., &a, a->field_b).
> - return the type_id, and generates a reloc_type based on user input (the relocation is generated in .BTF.ext section).
> - here, the relocation is against the return value, e.g., unsigned btf_id = __builtin_btf_type_id(expr, reloc_type) a relocation against the BPF instruction bpf_id = ... needs to be generated.
> 
>   What is the use case for this builtin? Currently, various uses are all for pretty-print. Pass some data together with btf_id so kernel or use space can pretty-print it. For example struct {void *p, uint32_t bpf_id} v = {a->b->c, __builtin_btf_type_id(a->b->c, 1)}; printk("....%pT...", ..., &v, ...); here a->b->c might be a type "struct abc", and the kernel will be able to print out the object pointed by a->b->c in a pretty format based on "struct abc". Here, the second parameter "1" means the ressulted btf_id will be relocated against kernel BTF. The linux kernel target is x64, arm64, etc.
> 
>   In the future, we may have other use cases.
> 
>   The reason we need relocation is later BTF debug info may be merged (linked) with other bpf program and you need to adjust btf_id, or it may try to match against types against vmlinux.
> 
>   The type associated with the expression may be local which user has control or maybe in a header user does not have control.
> 
>   The following is what I understand the possible alternative:
> - Keep or the retained type for the expression, so we do not need the IR builtin. I guess, in order to do this, we need:
> - We still need to annotate the expression somehow so we later on we can generate proper relocation later on.
> - If we do have a variable here, we might be able to annotate the variable (special attributes?) so we do not carry debuginfo types. But for expressions, we would still need to carry debuginfo. Otherwise, it is very hard to get debug info in backend.
> 
>   Hopefully this is more clear with the interface/intention of this builtin. Please let me know in high level whether we could do better. Thanks!

Thanks for the explanation (& sorry for my very delayed response).

BPF uses its own debug info format, right? I guess it might be far enough out of my wheelhouse that I don't have a lot of understanding/experience to impart into your situation there. I think the explanation makes some sense to me as to why you're using an intrinsic like this, and I was a bit quick to call for a revert/suggest this wasn't the right direction - sorry about that.

It might be worth having an extra test that's just IR->IR to test the BPFPreserveDIType pass in a more isolated fashion.

Or would it be reasonable to have Clang generate the IR in the post-BPFPreserveDIType form to begin with, and avoid the need for that pass to run later?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74572