[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]

Okay, with __bpf_unreachable, we will have a conflict with kernel. In bpf_helpers.h,

/*
 * Helper macro to throw a compilation error if __bpf_unreachable() gets
 * built into the resulting code. This works given BPF back end does not
 * implement __builtin_trap(). This is useful to assert that certain paths
 * of the program code are never used and hence eliminated by the compiler.
 *
 * For example, consider a switch statement that covers known cases used by
 * the program. __bpf_unreachable() can then reside in the default case. If
 * the program gets extended such that a case is not covered in the switch
 * statement, then it will throw a build error due to the default case not
 * being compiled out.
 */
#ifndef __bpf_unreachable
# define __bpf_unreachable()    __builtin_trap()
#endif

We need to pick a different symbol name.

[eddyz87]

Okay, with __bpf_unreachable, we will have a conflict with kernel. In bpf_helpers.h,

/*
 * Helper macro to throw a compilation error if __bpf_unreachable() gets
 * built into the resulting code. This works given BPF back end does not
 * implement __builtin_trap(). This is useful to assert that certain paths
 * of the program code are never used and hence eliminated by the compiler.
 *
 * For example, consider a switch statement that covers known cases used by
 * the program. __bpf_unreachable() can then reside in the default case. If
 * the program gets extended such that a case is not covered in the switch
 * statement, then it will throw a build error due to the default case not
 * being compiled out.
 */
#ifndef __bpf_unreachable
# define __bpf_unreachable()    __builtin_trap()
#endif

We need to pick a different symbol name.

__builtin_trap() is now supported by the backend and is translated as a call to __bpf_unreachable, e.g.:

$ cat ~/work/tmp/test_trap.c 
void foo(void);
void bar(void);
void buz(int p) {
  if (p) {
    foo();
    __builtin_trap();
  } else {
    bar();
  }
}
$ build/bin/clang --target=bpf -mcpu=v3 -S -o - ~/work/tmp/test_trap.c 
	...
buz:                                    # @buz
# %bb.0:                                # %entry
	*(u32 *)(r10 - 4) = w1
	w1 = *(u32 *)(r10 - 4)
	if w1 == 0 goto LBB0_2
	goto LBB0_1
LBB0_1:                                 # %if.then
	call foo
	call __bpf_unreachable
	goto LBB0_3
LBB0_2:                                 # %if.else
	call bar
	goto LBB0_3
LBB0_3:                                 # %if.end
	exit
.Lfunc_end0:
	...

Looks like bpf_helpers.h now needs to distinguish between compile time and load time asserts (use different macro definitions).

[eddyz87]

@yonghong-song ,

BTW, just updated a new version to incorporate some of your changes. Please take a look!

The changes lgtm, I also tried end-to-end by adjusting your kernel series to __bpf_unreachable and inserting __builtin_trap() call in one of the C tests. Verifier reports error as expected.

[efriedma-quic]

I'm still not completely happy with this; see #136603 for my most recent commentary on producing warnings based on compiler optimizations. (That's not BPF, but under the proposal here, a similar construct compiled to BPF would produce a BPF verifier error.) But Linux kernel developers have a different perspective on how a compiler should work... I'm not sure how to bridge the gap.

[4ast]

I'm still not completely happy with this; see #136603 for my most recent commentary on producing warnings based on compiler optimizations. (That's not BPF, but under the proposal here, a similar construct compiled to BPF would produce a BPF verifier error.) But Linux kernel developers have a different perspective on how a compiler should work... I'm not sure how to bridge the gap.

I don't see how issues with __builtin_dynamic_object_size() related to this diff.

[efriedma-quic]

I don't see how issues with __builtin_dynamic_object_size() related to this diff.

It's not really about __builtin_dynamic_object_size; it's about the way unrolling works.

[4ast]

I don't see how issues with __builtin_dynamic_object_size() related to this diff.

It's not really about __builtin_dynamic_object_size; it's about the way unrolling works.

and? This patch has nothing to do with unrolling. are you commenting on the right diff?

[yonghong-song]

Just updated another change which should resolve kernel build issue. At the same time, I will investigate to do different handling of 'unreachable' and '@llvm.trap'.

[yonghong-song]

Rebase on top of latest llvm-project code base.