In D147078#4231608, @nikic wrote:

In D147078#4231270, @anakryiko wrote:

If so, I think it would make the most sense to add an undo transform in the BPF backend.

Genuine curiosity (and keep in mind I'm no compiler expert at all), but why doing transformation and then customly undoing it in specific BPF backend would be a preferable solution to allowing backends to turn off some known-to-hurt transformations in the first place? It seems more complicated and error-prone to do the dance of undoing, rather than have a generic interface to prevent transformation in the first place.

Note, this is not the only transformation that hurts BPF verification, @yonghong-song previously added "undo transformation" logic, but it seems like a not very sustainable and very convoluted solution. So we are asking if we can have a simpler and more reliable opt-out for some small set of transformations.

Just blocking a transform is generally insufficient, because it does not handle the case where the code uses the problematic pattern in the first place. If the input code already contained the x < min(y, z) pattern, it would fail the verifier and converting it into x < y && y < z would be necessary to make it pass.

Note, this is not the only transformation that hurts BPF verification, @yonghong-song previously added "undo transformation" logic, but it seems like a not very sustainable and very convoluted solution. So we are asking if we can have a simpler and more reliable opt-out for some small set of transformations.

One goal of LICM is to target-independently canonicalise the IR to enable other IR optimizations, so IMO it would be preferable to avoid disabling parts via target hooks. Also, the currently proposed hook is not really scalable, e.g. what if another target wants to opt out as well?

In D147078#4229618, @nikic wrote:

Would it be fair to say that from the perspective of the BPF verifier, x < y && x < z is generally preferred over x < min(y, z), because the verifier does not know that x < min(y, z) implies x < y etc?

Are there any restrictions about number of struct arguments that can be passed in? Kernel changes were assuming at most 2, should we have a test that tests passing 3 structs that fit into 5 input registers and another test that passes 3 structs that do not fit in 5 input registers? And what should be the behavior in the latter case -- compilation error or switch to "pass by reference"?

In D131012#3697000, @aaron.ballman wrote:

In D131012#3696810, @anakryiko wrote:

In D131012#3696327, @aaron.ballman wrote:

In D131012#3695110, @anakryiko wrote:

This will severly break BPF users, as we have a heavy reliance on const volatile variables being allocated to .rodata, which are passed to BPF verifier in Linux kernel as read-only values. This is critical for BPF Complie Once - Run Everywhere approach of writing portable BPF applications. We've been relying on this behavior for years now and changes this will break many existing BPF applications.

Thank you for this feedback! I guess I'm a bit surprised given the contents from the issue seem to imply that BPF needed Clang's behavior to change: Note that this is causing practical difficulties: the kernel's bpftool is generating different skeleton headers for BPF code compiled from LLVM and GCC, because the names of the containing sections get reflected.

GCC folks are trying to make their BPF backend usable. But instead of working with community to make sure they do things the same way that Clang does (which for years now is de facto standard for compiling BPF code and we rely on this behavior heavily in libbpf and other BPF loader libraries), they instead work against BPF community and try to modify/adjust/break the world around them, instead of working with us to make GCC-BPF compatible with Clang.

Ah, that's background information I was unaware of. Thank you for sharing that perspective!

In D131012#3696327, @aaron.ballman wrote:

In D131012#3695110, @anakryiko wrote:

This will severly break BPF users, as we have a heavy reliance on const volatile variables being allocated to .rodata, which are passed to BPF verifier in Linux kernel as read-only values. This is critical for BPF Complie Once - Run Everywhere approach of writing portable BPF applications. We've been relying on this behavior for years now and changes this will break many existing BPF applications.

Thank you for this feedback! I guess I'm a bit surprised given the contents from the issue seem to imply that BPF needed Clang's behavior to change: Note that this is causing practical difficulties: the kernel's bpftool is generating different skeleton headers for BPF code compiled from LLVM and GCC, because the names of the containing sections get reflected.

This will severly break BPF users, as we have a heavy reliance on const volatile variables being allocated to .rodata, which are passed to BPF verifier in Linux kernel as read-only values. This is critical for BPF Complie Once - Run Everywhere approach of writing portable BPF applications. We've been relying on this behavior for years now and changes this will break many existing BPF applications.

In D111307#3190832, @rjmccall wrote:

When dealing with some gross situation where a source generator is generating things that don't really work cleanly with the normal language, I have a strong preference for saying that the source generator should change. In this case, that would mean e.g. generating different type names in the generated headers. Is that really such a non-starter? This is really quite a terrible feature.

In D111307#3124927, @yonghong-song wrote:

I am okay with an explicit option to enable this attribute. I remembered in the past we discussed with a possible option -fpermissive to control this. But this may be too generic. Maybe an option like -Xclang -target-feature -Xclang +attr_bpf_dominating_decl? Just a rough idea.

Great, thank you!

lgtm

lgmt

lgtm

I've hit similar issue in the past and it's a very frustrating experience having to work around that. So thumbs up for preventing this!

I think it's reasonable behavior to just merge all bpf_tags across declaration(s) and a definition. Kind of like __weak behaves, it doesn't matter if it's on actual function or its extern declaration.

LGTM from the BTF definition standpoint.

But bar[1] is not a field, why can't we just return error for such case? If someone wants to check type existence we have a separate TYPE_EXISTS relocation.

Nice, thanks! This will work for externs with explicit section name (.ksym) and with no section name (externs for static linking), right?

LGTM, thanks!

In D83289#2549050, @iii wrote:

Hi, it has been a long time, but I've finally implemented the support
for the floating-point types in BTF. Here are the other pieces:

• verifier+libbpf: https://github.com/iii-i/linux/commit/wip
• just libbpf (same as the libbpf part of the above): https://github.com/iii-i/libbpf/commit/wip

Having an explicit special section for extern functions clearly indicating the intent (that it should match kernel function) is really good. Especially with static linking. Otherwise, this "try local function, otherwise fallback to kernel function" is going to be really painful for users if there is any error, because all such errors will be indicated at the load time instead of at the link time, where they should be detected and reported.

In D83242#2195456, @yonghong-song wrote:

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

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.

Tested locally. Previously failing tests are now passing. All recorded relocations look correct. Thanks!

LGTM. One question: why didn't we run into the need for SeqNumVal trick with field-based relocations? We seem to need it for all other types (including type ID-based), but not for field-based?

Looks good. I've tested locally all this and it works as expected. Thanks!

looks great, I'll complete libbpf support for all these ASAP

After thinking about this a bit more, I think adding a new TYPE_EXISTENCE (instead of FIELD_EXISTENCE) relocation type is the better way to handle this. It would allow Clang to be stricter as to what is passed into FIELD_EXISTENCE (only fields, not types) vs TYPE_EXISTENCE(only types, no fields). It will make it cleaner on libbpf side as well, because the validation and relocation logic is different between handling fields and types. So overall, I think it will be beneficial on every level of the stack to separate them. I hope it's not too hard to make this change?

Thanks for the fix! LGTM.

This is awesome. I'll apply patches locally and will play with them tonight to see if everything works in libbpf as well. Thanks!

ah, makes sense. LGTM then :)

maybe let's just remove special-casing of .maps section? It's just like any other global variable, should be handled in exactly the same way, no?

I concur with @yonghong-song, making float disappear is much bigger pain.

pahole does a sensible thing, it represents it as BTF_KIND_INT (which maps to DWARF's DW_TAG_base_type), but with no encoding (so it's effectively unsigned integer with proper size).

Awesome, that's exactly what we need for BPF helper availability checks!

V >= Lo && V < Hi --> V - Lo u< Hi - Lo
V < Lo || V >= Hi --> V - Lo u>= Hi - Lo

IntrinsicsBPF.td uses __builtin_bpf_load_u32_to_ptr, while everywhere it is just __builtin_load_u32_to_ptr. I think having bpf prefix there is good, given this is bpf-specific. But either way, probably should be consistent everywhere?

For my purposes, having extern var BTF only for extern const void is just fine. Not crashing Clang for extern void is great as well :)

In D74668#2020554, @yonghong-song wrote:

In D74668#2019558, @anakryiko wrote:

what's the use case for flag==0 (no relocation)? why using built-in at all in such case? Also flag==1 means relocate to local BTF ID or remote (kernel) BTF ID? Do you plan to add flag=2 as well to cover both cases? Or am I misunderstanding the meaning of this flag?

Originally, I thought flag = 0 for the following use case:

e.g., they just want to know the type of a particular local structure for pretty print purpose.
Note that currently only types for global/extern variables, function parameters are recorded in btf.
  int test() {
     struct { int a; int b; ...} ctx;
     btf_id = __builtin_btf_type_id(ctx, 0);
     bpf_seq_write(seq, &btf_id, sizeof(btf_id));
     bpf_seq_write(seq, &ctx, sizeof(ctx));
     ...
  }

But obviously without relocation, this will not work with btf deduplication, future static linking etc.

what's the use case for flag==0 (no relocation)? why using built-in at all in such case? Also flag==1 means relocate to local BTF ID or remote (kernel) BTF ID? Do you plan to add flag=2 as well to cover both cases? Or am I misunderstanding the meaning of this flag?

Let's extend __builtin_btf_type_id() to accept second argument specifying whether it's local BTF ID (from program's BTF) or target BTF ID (from kernel/module BTF)? We can probably make it an enum just like with preserve_access_index() built-in, for easy future extension. WDYT?

looks simple enough, thanks for fixing it fast!

lgtm

I implemented libbpf relocation patching support for new classes of instructions and tested against kernel selftests and runqslower program. All of those pass. Thanks a lot, this is going to improve BPF CO-RE experience immensely!

awesome, thanks!

looks good, thanks!

hm... size is useful, actually. Ok, let me play with it, it's hard to get a good idea of BTF from LLVM tests. Verifier will definitely reject such variables, though, so we'll need to do sanitization for sure.

In D68822#1717603, @yonghong-song wrote:

Also, another thing. we COULD emit debug info type for externs for BPF in clang. We just have to think whether this is really necessary or not.

if there is no type information available for extern variables, there is little benefit in having them as DATASEC+VAR. No size, no type. We can get the same information from ELF itself without having to teach verifier and libbpf how to handle these new types. Let's drop this for now? The only potential benefit is having a section name associated with extern, but we can either add that later, or even add it in a way that will require no verifier changes - BTF.ext.

Looks good, thanks for fixing so fast!