In D142046#4066758, @yonghong-song wrote:

@ast With this patch, gentoo clang compilation will hit the warning even if people appends -fno-stack-protector. Is this okay? In general, if the option is '-fstack-protector -fno-stack-protector', we should not issue warning, right?

This approach looks better to me than NVPTX warn and I agree with Ed that it's better to leave NVPTX as-is to avoid any regression.

looks fine, but could you add a unit test for it?

clang -target bpf -O2 -S t3.c
t3.c:1:5: error: defined with too many args

The workaround is not pretty, but I couldn't come up with a better way.

good catch.

a test got lost?

lgtm as well.

Thanks!

assert is a heavy hammer.
can we exit with report_fatal_error instead?

The fix to prevent the crash looks good, but what should be the expected codegen for a global variable?
CO-RE needs a constant.
Are we assuming that the global data will be inited with some constant and a later pass will propagate it further into __builtin_preserve,
so at the end it will become a constant?

builtin_memcpy is not inlined

makes sense. without scalar evolution in the verfiier we cannot really propagate the range of r3 back into r2 and adjust it through reverse transformation of 13: (07) r3 += -3.
The llvm path is better.
Thanks!

In D116063#3350010, @tstellar wrote:

@ast Is this OK to backport to release/14.x ?

Yep LLVM does have some custom attributes and such, eg: https://github.com/llvm/llvm-project/blob/effb87dfa810a28e763f914fe3e6e984782cc846/llvm/include/llvm/BinaryFormat/Dwarf.def#L592

I think addition of emitFuncDataSections is justified.
The rest is bpf specific.

Makes sense to me. Only BPF target will notice this difference.

thanks for the quick fix!

Thanks!

looks great. thanks

why the current behavior is not enough?
BTF_KIND_FUNC (foo1) -> BTF_KIND_FUNC_PROTO (foo1)
BTF_KIND_FUNC (foo2) -> BTF_KIND_FUNC_PROTO (foo2)
libbpf can use order to find proper function. foo can be extern as it is right now. Then libbpf will first try to match between multiple elf bpf files. If still not found it will search in vmlinux/mods.
I think using KIND_VAR is incorrect. There is no variable here.

Does it work already without specifying the section, right?
void foo1(int); works ?
void foo2(int) __ attribute __((section("bar"))); sort-of works, but this one goes into the same section as foo1 ?

What's a use case?
The test is necessary.

Looks like the test didn't change. Only commit log... that's fine. I think the diff is ready to land, but let's wait for the kernel patches to be ready as well.

looks good. Before landing we need to agree on the full set of instructions that -mcpu=v4 will support.
atomic_fetch_or|xor|and are probably needed as instructions. The kernel JIT will generate x86 cmpxchg for them.
Because if llvm generates bpf cmpxchg insn then we'd need to teach the verifier to recognize infinite loops.

looks like x86 64-bit is ILP, so it's not niche.
Looks like Source gets the least amount of testing (judging by archs that use it).
RegPressure is probably 2nd most used after ILP.
It feels that we should fix it inside ILP instead.

ouch. sounds like we don't have much choice. Let's figure out what happened with test_verif_scale3 first, but looks like we have to land regardless.

is this based on D87153 ?
the passthrough will be removed later, right?

thanks for detailed analysis.

Is it a cleanup or is it a fix for some bug? If latter there should be a new test for it?

lgtm.
curious what happens when type is defined within args, like:
__builtin_preserve_field_info(*(struct { int a; } *)0, 2);

lgtm

'float *var;' in the struct won't be trivial to handle with a skip indeed.
My concern with float->char[] substitute is that the kernel decides on calling convention based on these types.
If there is a function fn(int a, float b); and pahole emits BTF for it as fn(int a, char b[4])
the kernel will let bpf progs to attach to it with wrong register passing.
Currently array is not allowed in btf_distill_func_proto(), so it's a theoretical issue, but still dangerous long term.
I think pahole/clang should either skip generating BTF for anything with float or BTF should be extended to encode it.
I think extending BTF would be easier.
I don't like KIND_INT_FLOATING though. KIND_FLOAT is better. Just like single KIND_INT that represents char/int/long
the KIND_FLOAT should be able to represent float/double/long double.

does pahole convert float/double to int ? Is it really the case?
I think it's better to skip float/double when they are part of a struct and leave a hole in there.
I worry that representing them as 'char' array may cause backward compatibility issues later.
If pahole is doing such hack now it probably should be fixed too.

that's an important fix to backport

It feels that the same thing can be represented as inline asm.
What advantage builtin has?