[BPF] Handle traps with kfunc call __bpf_trap

[yonghong-song]

Currently, middle-end generates 'unreachable' insn if the compiler
feels the code is indeed unreachable or the code becomes invalid
due to some optimizaiton (e.g. code optimization with uninitialized
variables).

Right now BPF backend ignores 'unreachable' insn during selectiondag
lowering. For cases where 'unreachable' is due to invalid code
transformation, such a signal will be missed. Later on, users needs
some effort to debug it which impacts developer productivity.

This patch enabled selectiondag lowering for 'unreachable' insn.

Previous attempt ([1]) tries to have a backend IR pass to filter
out 'unreachable' insns in a number of cases. But such pattern
matching may misalign with future middle-end optimization with
'unreachable' insns.

This patch takes a different approach. The 'unreachable' insn is
lowered with special encoding in bpf object file and verifier
will do proper verification for the bpf prog. More specifically,
the 'unreachable' insn is replaced by a __bpf_trap() function.
This function will be a kfunc (in ".ksyms" section) with a weak
attribute, but does not have definition. The actual kfunc definition
is expected to be in kernel. The __bpf_trap() extern function
is also encoded in BTF. The name __bpf_trap() is chosen to satisfy
reserved identifier requirement.

Besides the uninitialized variable case, the builtin function
'__builtin_trap' can also generate kfunc __bpf_trap(). For example
in [3], we have

 # define __bpf_unreachable() __builtin_trap() 

If the compiler didn't remove __builtin_trap() during middle-end
optimization, compilation will fail.

With this patch, compilation will not fail and __builtin_trap()
is converted to __bpf_trap() kfunc. The eventual failure will be
in verifier instead of llvm compilation. To keep compilation
time failure, user can add an option like -ftrap-function=<something>.

I tested this patch on bpf selftests and all tests are passed.
I also tried original example in [2] and the code looks like below:

 ; { 0: bf 16 00 00 00 00 00 00 r6 = r1 ; bpf_printk("Start"); 1: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll 0000000000000008: R_BPF_64_64 .rodata 3: b4 02 00 00 06 00 00 00 w2 = 0x6 4: 85 00 00 00 06 00 00 00 call 0x6 ; DEFINE_FUNC_CTX_POINTER(data) 5: 61 61 4c 00 00 00 00 00 w1 = *(u32 *)(r6 + 0x4c) ; bpf_printk("pre ipv6_hdrlen_offset"); 6: 18 01 00 00 06 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x6 ll 0000000000000030: R_BPF_64_64 .rodata 8: b4 02 00 00 17 00 00 00 w2 = 0x17 9: 85 00 00 00 06 00 00 00 call 0x6 10: 85 10 00 00 ff ff ff ff call -0x1 0000000000000050: R_BPF_64_32 __bpf_trap 11: 95 00 00 00 00 00 00 00 exit <END> 

Eventually kernel verifier will emit the following logs:

 10: (85) call __bpf_trap#74479 unexpected __bpf_trap() due to uninitialized variable? 

In another internal sched-ext bpf prog, with the patch we have bpf code:

 Disassembly of section .text: 0000000000000000 <scx_storage_init_single>: ; { 0: bc 13 00 00 00 00 00 00 w3 = w1 1: b4 01 00 00 00 00 00 00 w1 = 0x0 ; const u32 zero = 0; ... 0000000000003a80 <create_dom>: ; { 1872: bc 16 00 00 00 00 00 00 w6 = w1 ; bpf_printk("dom_id %d", dom_id); 1873: 18 01 00 00 3f 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x3f ll 0000000000003a88: R_BPF_64_64 .rodata 1875: b4 02 00 00 0a 00 00 00 w2 = 0xa 1876: bc 63 00 00 00 00 00 00 w3 = w6 1877: 85 00 00 00 06 00 00 00 call 0x6 ; ret = scx_bpf_create_dsq(dom_id, 0); 1878: bc 61 00 00 00 00 00 00 w1 = w6 1879: b4 02 00 00 00 00 00 00 w2 = 0x0 1880: 85 10 00 00 ff ff ff ff call -0x1 0000000000003ac0: R_BPF_64_32 scx_bpf_create_dsq ; domc->node_cpumask = node_data[node_id]; 1881: 85 10 00 00 ff ff ff ff call -0x1 0000000000003ac8: R_BPF_64_32 __bpf_trap 1882: 95 00 00 00 00 00 00 00 exit <END> 

The verifier can easily report the error too.

A bpf flag -bpf-disable-trap-unreachable is introduced to disable
trapping for 'unreachable' or __builtin_trap.

[1] #126858
[2] https://github.com/msune/clang_bpf/blob/main/Makefile#L3
[3] https://github.com/libbpf/libbpf/blob/master/src/bpf_helpers.h

[yonghong-song]

cc @anakryiko @jemarch

[nikic]

The general approach here looks reasonable to me.

How does the resulting verifier error look like?

[nikic]

Probably this should be part of the generic handling for TrapUnreachable. IMHO we should never insert extra code into a naked function, for other targets as well.

[yonghong-song]

Sounds good to me. Let me check more and may have a patch for this later.

[anakryiko]

I don't like the approach of Clang emitting kfunc call, putting it in .ksyms section (which is just a libbpf convention, not some sort of gold standard or anything), and so on. I think the appropriate way to do this with would be a BPF instruction.
But if we have to go with kfunc approach, let's at least call it bpf_unreachable(). We can even implement this in the kernel (it would just WARN/panic), so libbpf successfully resolves it without unnecessary warnings on modern kernels. And BPF verifier would provide a meaningful error message if it detects reachable bpf_unreachable() call.

[yonghong-song]

From llvm perspective, adding a new insn is easy, likes below:

diff --git a/llvm/lib/Target/BPF/BPFInstrInfo.td b/llvm/lib/Target/BPF/BPFInstrInfo.td index 2dcf1eae086b..59bad209ea54 100644 --- a/llvm/lib/Target/BPF/BPFInstrInfo.td +++ b/llvm/lib/Target/BPF/BPFInstrInfo.td @@ -790,11 +790,25 @@ class RET<string OpcodeStr> let BPFClass = BPF_JMP; } +class TRAP<string OpcodeStr> + : TYPE_ALU_JMP<BPF_EXIT.Value, BPF_K.Value, + (outs), + (ins), + !strconcat(OpcodeStr, ""), + []> { + let Inst{31-0} = 1; + let BPFClass = BPF_JMP; +} + let isReturn = 1, isTerminator = 1, hasDelaySlot=0, isBarrier = 1, isNotDuplicable = 1 in { def RET : RET<"exit">; } +let isTrap = 1 in { + def UNREACHABLE : TRAP<"unreachable">; +} // isTerminator = 1 + // ADJCALLSTACKDOWN/UP pseudo insns let Defs = [R11], Uses = [R11], isCodeGenOnly = 1 in { def ADJCALLSTACKDOWN : Pseudo<(outs), (ins i64imm:$amt1, i64imm:$amt2), 

Different encoding is possible. I am chosing current kfunc approach since it minimizes the kernel change, spares one new insn, and can work on old kernel.

But I am open to the new insn approach or real bpf_unreachable() kfunc approach if we can reach a consensus.

[eddyz87]

While .ksyms is just a convention, I think that emitting kfunc calls from clang might be useful. E.g. currently memcpy, memset, memmove, memcmp intrinsics are unrolled, but there is no point in doing so.

[4ast]

While .ksyms is just a convention, I think that emitting kfunc calls from clang might be useful. E.g. currently memcpy, memset, memmove, memcmp intrinsics are unrolled, but there is no point in doing so.

This convention is pretty much a de-facto standard now. Several libraries support it.
It's not unusual for a compiler to use a particular section name like ".text" or ".rodata" for specific needs.
The name is target dependent. ".ksyms" is another special name.
So I think it's appropriate for a compiler to emit "call blah_unreachable" and "call blah_memcpy" with ".ksyms" section name. It's cleaner and more flexible than "call magic_const" or inventing a new trap-like insn.

[yonghong-song]

How does the resulting verifier error look like?

The current verifier error looks like

 func#0 @0 last insn is not an exit or jmp 

This is due to that there is a logic in verifier to ensure the last insn in the func must be an 'exit' or 'jmp' insn. If this approach sounds reasonable, if __unreachable_helper is the last insn, the verifier log can be changed to e.g.,

 last insn is marked as unreachable, maybe due to uninitialized variable? 

Such more explicit message will prompt users to check their auto variables.

Without this patch, the user will simply get error message

 last insn is not an exit or jmp 

and users needs to dig into asm codes to find why the code is generated that way.

[eddyz87]

Maybe consider adding a test case?
E.g. as below:

; RUN: llc -mtriple=bpfel -mcpu=v3 -filetype=obj -o %t1 %s ; RUN: llvm-objcopy --dump-section='.BTF'=%t2 %t1 ; RUN: %python %p/print_btf.py %t2 | FileCheck -check-prefixes=CHECK-BTF %s ; RUN: llc -mtriple=bpfel -mcpu=v3 < %s | FileCheck -check-prefixes=CHECK %s define void @foo() { entry: tail call void @bar() unreachable } ; CHECK: foo: ; CHECK-NEXT: .Lfunc_begin0: ; CHECK-NEXT: .cfi_startproc ; CHECK-NEXT: # %bb.0: ; CHECK-NEXT: call bar ; CHECK-NEXT: call __unreachable_helper ; CHECK-NEXT: .Lfunc_end0: define void @buz() #0 { entry: tail call void asm sideeffect "r0 = r1; exit;", ""() unreachable } ; CHECK: buz: ; CHECK-NEXT: .Lfunc_begin1: ; CHECK-NEXT: .cfi_startproc ; CHECK-NEXT: # %bb.0: ; CHECK-NEXT: #APP ; CHECK-NEXT: r0 = r1 ; CHECK-NEXT: exit ; CHECK-EMPTY: ; CHECK-NEXT: #NO_APP ; CHECK-NEXT: .Lfunc_end1: ; CHECK-BTF: [1] FUNC_PROTO '(anon)' ret_type_id=0 vlen=0 ; CHECK-BTF: [2] FUNC '__unreachable_helper' type_id=1 linkage=extern ; CHECK-BTF: [3] DATASEC '.ksyms' size=0 vlen=1 ; CHECK-BTF: type_id=2 offset=0 size=0 declare dso_local void @bar() attributes #0 = { naked } !llvm.dbg.cu = !{!0} !llvm.module.flags = !{!2, !3} !0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, emissionKind: FullDebug) !1 = !DIFile(filename: "test.c", directory: "/some/dir") !2 = !{i32 7, !"Dwarf Version", i32 5} !3 = !{i32 2, !"Debug Info Version", i32 3}

[eddyz87]

Overall lgtm, modulo absence of tests.

[eddyz87]

Nit: maybe declare "__unreachable_helper" as a constant?

[yonghong-song]

Do you mean a constant variable? This probably not a good idea as the func later is added to BTF. Or you mean a constant function? What does it mean for a constant function?

[eddyz87]

I mean something like static const char *UNREACHABLE_HELPER = "__unreachable_helper"; at the file level, or a #define ....

[yonghong-song]

I see. Yes, it is a good idea. Will do this.

[eddyz87]

While .ksyms is just a convention, I think that emitting kfunc calls from clang might be useful. E.g. currently memcpy, memset, memmove, memcmp intrinsics are unrolled, but there is no point in doing so.

[yonghong-song]

I will add some tests in the next revision.

Maybe consider adding a test case? E.g. as below:

; RUN: llc -mtriple=bpfel -mcpu=v3 -filetype=obj -o %t1 %s ; RUN: llvm-objcopy --dump-section='.BTF'=%t2 %t1 ; RUN: %python %p/print_btf.py %t2 | FileCheck -check-prefixes=CHECK-BTF %s ; RUN: llc -mtriple=bpfel -mcpu=v3 < %s | FileCheck -check-prefixes=CHECK %s define void @foo() { entry: tail call void @bar() unreachable } ; CHECK: foo: ; CHECK-NEXT: .Lfunc_begin0: ; CHECK-NEXT: .cfi_startproc ; CHECK-NEXT: # %bb.0: ; CHECK-NEXT: call bar ; CHECK-NEXT: call __unreachable_helper ; CHECK-NEXT: .Lfunc_end0: define void @buz() #0 { entry: tail call void asm sideeffect "r0 = r1; exit;", ""() unreachable }

As suggested by @nikic, I will later have a patch to prevent 'unreachable' for naked functions for all architectures. So this test will not be included.

; CHECK: buz:
; CHECK-NEXT: .Lfunc_begin1:
; CHECK-NEXT: .cfi_startproc
; CHECK-NEXT: # %bb.0:
; CHECK-NEXT: #APP
; CHECK-NEXT: r0 = r1
; CHECK-NEXT: exit
; CHECK-EMPTY:
; CHECK-NEXT: #NO_APP
; CHECK-NEXT: .Lfunc_end1:

; CHECK-BTF: [1] FUNC_PROTO '(anon)' ret_type_id=0 vlen=0
; CHECK-BTF: [2] FUNC '__unreachable_helper' type_id=1 linkage=extern
; CHECK-BTF: [3] DATASEC '.ksyms' size=0 vlen=1
; CHECK-BTF: type_id=2 offset=0 size=0

declare dso_local void @bar()

attributes #0 = { naked }

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!2, !3}

!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, emissionKind: FullDebug)
!1 = !DIFile(filename: "test.c", directory: "/some/dir")
!2 = !{i32 7, !"Dwarf Version", i32 5}
!3 = !{i32 2, !"Debug Info Version", i32 3}

[eddyz87]

define void @buz() #0 {
entry:
tail call void asm sideeffect "r0 = r1; exit;", ""()
unreachable
}

As suggested by @nikic, I will later have a patch to prevent 'unreachable' for naked functions for all architectures. So this test will not be included.

As far as I understand:

• that would be a separate PR merged at some later point;
• the code change in BPFDAGToDAGISel::Select is only necessary to handle naked functions.

Thus, I'd keep the test for now, just to have all code paths covered.

[yonghong-song]

define void @buz() #0 {
entry:
tail call void asm sideeffect "r0 = r1; exit;", ""()
unreachable
}

As suggested by @nikic, I will later have a patch to prevent 'unreachable' for naked functions for all architectures. So this test will not be included.

As far as I understand:

* that would be a separate PR merged at some later point; 

Yes.

* the code change in `BPFDAGToDAGISel::Select` is only necessary to handle naked functions. 

Yes.

Thus, I'd keep the test for now, just to have all code paths covered.
Okay, will keep the test for now at least.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[yonghong-song]

Rebase on top of latest llvm-project code base.

[RKSimon]

@yonghong-song I'm seeing a lot of gcc warnings:

In file included from /home/simon/LLVM/llvm-project/llvm/lib/Target/BPF/BPFMISimplifyPatchable.cpp:30: /home/simon/LLVM/llvm-project/llvm/lib/Target/BPF/BPF.h:25:20: warning: ‘llvm::BPF_TRAP’ defined but not used [-Wunused-variable] 25 | static const char *BPF_TRAP = "__bpf_trap"; 

Any chance you can change this to:

#define BPF_TRAP "__bpf_trap"

[yonghong-song]

@RKSimon I could do the change. But can you let me know how to reproduce the warning? Maybe I missed some compilation flags?

[RKSimon]

gcc9 -DCMAKE_C_FLAGS='-Wall -Wextra' -DCMAKE_CXX_FLAGS='-Wall -Wextra'