[PATCH] D83242: [clang][BPF] support type exist/size and enum exist/value relocations

[Andrii Nakryiko via Phabricator via cfe-commits]

anakryiko added a comment.

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

> In D83242#2193989 <https://reviews.llvm.org/D83242#2193989>, @anakryiko wrote:
>
>>> This is different than testing whether a type exists or not where we want to precisely test whether that type exists or not, so we want to preserve `typedef` type vs. `struct t` type.
>>
>> `typedef t` and `struct t` are also different for field relocations (those t is in different namespaces, actually), so it might be a good idea to disambiguate those for field relocations as well?
>
> Just to be clear. The type recorded in relocation will be always valid since we have type_id there. For field exists, due to we may use nullptr as the object, if CSE happens, it is possible the root type id may be the "struct t" instead of "typedef t" (or the other ordering) assuming the definition "typedef struct t t;". But from root type_id, libbpf will be able to correctly trace down to the field and do the relocation properly.
>
> On the other hand, I agree that this is not ideal as we may use a different root type than what user specified in the source. But everything is hidden and invisible to user so I won't have user visible impact.

Hm.. This root type is used by libbpf to find candidate in kernel BTF. Name and kind of that type is what is used. When you have `typedef t` and `struct t`, their names match, but their kinds don't. So it could lead to inability to find a candidate.

> To fix this in clang is actually tricky, we do not want to add seq number to every preserve_field_info* IR builtin since this is not really necessary for non-existence relocations. Also preserve_field_info* although used by bpf only so far is llvm generic builtin, I would hesitate to change it with new seq_num fields. The best will be have another bpf specific builtin
> for field existence, but given the current mechanism also works, I probably will keep it this way.

A new built-in is definitely a big hassle. It hasn't been a problem so far and the possibility is really slim. If it's a problem to do this for existing built-in, let's leave it as is for now. Just something to keep in mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83242