[BPF] allow external calls

[tamird]

Summary

This patch lifts the restriction on external functions introduced in
e497548. It seems that restriction was intended to produce better
compiler errors when -Wno-strict-prototypes was used, but it produces
false positives when memory intrinsics are lowered to libcalls.

[alessandrod]

llvm/lib/Target/BPF/BPFISelLowering.cpp

43–50

If you do this, you also need to update the callers which are still doing work after calling fail, but fail now never returns.

Also in LowerCall, the fail call in the ExternalSymbolSDNode case should be removed. That is a legitimate condition when compiling rust code where the builtin is provided by another crate so it shouldn't abort.

[tamird]

Completely reworked the patch; no longer generating fatal errors.

[tamird]

llvm/lib/Target/BPF/BPFISelLowering.cpp

43–50

Thank you - I've reworked this patch.

[tamird]

Fix broken link in commit message.

[eddyz87]

Please note that a number of unit tests fails with this patch applied:

• LLVM :: CodeGen/BPF/many_args1.ll
• LLVM :: CodeGen/BPF/many_args2.ll
• LLVM :: CodeGen/BPF/struct_ret1.ll
• LLVM :: CodeGen/BPF/vararg1.ll
• LLVM :: CodeGen/BPF/warn-call.ll

These failures could be caught when ninja check-all target is executed.

I don't think that cleanup changes should be intermixed with functionality changes.
As the summary notes there three separate changes in this revision:

• code cleanup (which is, in fact, somewhat controversial):
  • LLVM Coding Standards actually require brace-less if statements (link)
  • I don't understand the crusade against assert and llvm_unreachable
• change of the behavior for ExternalSymbolSDNode: this was not a part of Diff 1, so I assume that it was not a part of the @tamird's initial intent. Is there a use case for this change? The libbpf commit 0a34245 deals only with external maps, not external functions.
• changes in LowerCallResult and LowerOperation regarding undef: I think both zeros and undef's are a bad choice here. If we can't compile it we should report error and abort compilation.

I think that all three should be tracked as separate revisions.

[tamird]

In D155894#4525984, @eddyz87 wrote:

Please note that a number of unit tests fails with this patch applied:

• LLVM :: CodeGen/BPF/many_args1.ll
• LLVM :: CodeGen/BPF/many_args2.ll
• LLVM :: CodeGen/BPF/struct_ret1.ll
• LLVM :: CodeGen/BPF/vararg1.ll
• LLVM :: CodeGen/BPF/warn-call.ll

These failures could be caught when ninja check-all target is executed.

Thanks. Most of the failures were caused by slight changes to error strings.

I don't think that cleanup changes should be intermixed with functionality changes.
As the summary notes there three separate changes in this revision:

• code cleanup (which is, in fact, somewhat controversial):
  • LLVM Coding Standards actually require brace-less if statements (link)
  • I don't understand the crusade against assert and llvm_unreachable

Extracted these changes to https://reviews.llvm.org/D156136. Let's discuss there.

• change of the behavior for ExternalSymbolSDNode: this was not a part of Diff 1, so I assume that it was not a part of the @tamird's initial intent. Is there a use case for this change? The libbpf commit 0a34245 deals only with external maps, not external functions.

When compiling Rust code, we see this warning trigger on memset intrinsics, but this is not a problem because we provide an implementation of memset and the intrinsic is eventually lowered to a call to the function we provide. Is there a finer-grained change that would achieve our goals?

• changes in LowerCallResult and LowerOperation regarding undef: I think both zeros and undef's are a bad choice here. If we can't compile it we should report error and abort compilation.

I agree in principle, but as note in code comments, this produces worse diagnostics when multiple functions contain such errors. Consider also that it was important to avoid exiting, hence D20571, but I do not understand why. Perhaps @ast can fill this in.

I think that all three should be tracked as separate revisions.

Will do once you have some guidance for me on the above.

[eddyz87]

In D155894#4528546, @tamird wrote:

• change of the behavior for ExternalSymbolSDNode: this was not a part of Diff 1, so I assume that it was not a part of the @tamird's initial intent. Is there a use case for this change? The libbpf commit 0a34245 deals only with external maps, not external functions.

When compiling Rust code, we see this warning trigger on memset intrinsics, but this is not a problem because we provide an implementation of memset and the intrinsic is eventually lowered to a call to the function we provide. Is there a finer-grained change that would achieve our goals?

Could you please point me to some example (on github or something like that)?
I want to insert some prints to see what is the difference in DAG nodes between this and functions declared extern in C code.

• changes in LowerCallResult and LowerOperation regarding undef: I think both zeros and undef's are a bad choice here. If we can't compile it we should report error and abort compilation.

I agree in principle, but as note in code comments, this produces worse diagnostics when multiple functions contain such errors. Consider also that it was important to avoid exiting, hence D20571, but I do not understand why. Perhaps @ast can fill this in.

If we want to report this for multiple functions I think we can set some flag and exit in TargetLoweringBase::finalizeLowering().

[tamird]

In D155894#4529382, @eddyz87 wrote:

In D155894#4528546, @tamird wrote:

• change of the behavior for ExternalSymbolSDNode: this was not a part of Diff 1, so I assume that it was not a part of the @tamird's initial intent. Is there a use case for this change? The libbpf commit 0a34245 deals only with external maps, not external functions.

When compiling Rust code, we see this warning trigger on memset intrinsics, but this is not a problem because we provide an implementation of memset and the intrinsic is eventually lowered to a call to the function we provide. Is there a finer-grained change that would achieve our goals?

Could you please point me to some example (on github or something like that)?
I want to insert some prints to see what is the difference in DAG nodes between this and functions declared extern in C code.

This reproduces in the aya integration tests in this PR on this commit: https://github.com/aya-rs/bpf-linker/pull/77/commits/6624b7d673a7b65cc634e94a4a8a63f79aa90f64

It's not exactly a simple repro. I can try to give you just the IR if that'd be helpful?

• changes in LowerCallResult and LowerOperation regarding undef: I think both zeros and undef's are a bad choice here. If we can't compile it we should report error and abort compilation.

I agree in principle, but as note in code comments, this produces worse diagnostics when multiple functions contain such errors. Consider also that it was important to avoid exiting, hence D20571, but I do not understand why. Perhaps @ast can fill this in.

If we want to report this for multiple functions I think we can set some flag and exit in TargetLoweringBase::finalizeLowering().

OK, I can do that, but that still doesn't address the original motivation. @ast can you please explain why exiting was considered as something to avoid?

[eddyz87]

This reproduces in the aya integration tests in this PR on this commit: https://github.com/aya-rs/bpf-linker/pull/77/commits/6624b7d673a7b65cc634e94a4a8a63f79aa90f64

It's not exactly a simple repro. I can try to give you just the IR if that'd be helpful?

Yes, please, that would be helpful.

[tamird]

In D155894#4529756, @eddyz87 wrote:

This reproduces in the aya integration tests in this PR on this commit: https://github.com/aya-rs/bpf-linker/pull/77/commits/6624b7d673a7b65cc634e94a4a8a63f79aa90f64

It's not exactly a simple repro. I can try to give you just the IR if that'd be helpful?

Yes, please, that would be helpful.

https://gist.github.com/tamird/f3ff414f183b085c3e37cd7d5b5754de

[eddyz87]

In D155894#4529763, @tamird wrote:

In D155894#4529756, @eddyz87 wrote:

This reproduces in the aya integration tests in this PR on this commit: https://github.com/aya-rs/bpf-linker/pull/77/commits/6624b7d673a7b65cc634e94a4a8a63f79aa90f64

It's not exactly a simple repro. I can try to give you just the IR if that'd be helpful?

Yes, please, that would be helpful.

https://gist.github.com/tamird/f3ff414f183b085c3e37cd7d5b5754de

I use a reasonably fresh LLVM main (88b6d291bbdf) with the following debug print:

diff --git a/llvm/lib/Target/BPF/BPFISelLowering.cpp b/llvm/lib/Target/BPF/BPFISelLowering.cpp
index 83a4bfb2f758..7937f8918fc8 100644
--- a/llvm/lib/Target/BPF/BPFISelLowering.cpp
+++ b/llvm/lib/Target/BPF/BPFISelLowering.cpp
@@ -455,6 +455,8 @@ SDValue BPFTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
     InGlue = Chain.getValue(1);
   }
 
+  llvm::dbgs() << "Callee: ";
+  Callee.dump();
   // If the callee is a GlobalAddress node (quite common, every direct call is)
   // turn it into a TargetGlobalAddress node so that legalize doesn't hack it.
   // Likewise ExternalSymbol -> TargetExternalSymbol.

I try to llc the above gist and see the following:

$ llc -mcpu=v3 extern-builtins-from-tamird.ll -o /dev/null && echo "done"
Callee: t10: i64 = Constant<1>
Callee: t81: i64 = Constant<25>
Callee: t8: i64 = Constant<1>
Callee: t81: i64 = Constant<25>
Callee: t8: i64 = Constant<1>
Callee: t81: i64 = Constant<25>
Callee: t8: i64 = Constant<1>
Callee: t82: i64 = Constant<25>
Callee: t8: i64 = Constant<1>
Callee: t81: i64 = Constant<25>
Callee: t8: i64 = Constant<1>
Callee: t81: i64 = Constant<25>
done

It looks like the ExternalSymbolSDNode branch is not triggered by the gist :(

[tamird]

Yeah, I also can't reproduce using llc. But in the actual code this module is fed into LLVMTargetMachineEmitToFile and those errors do show up.

[eddyz87]

In D155894#4529918, @tamird wrote:

Yeah, I also can't reproduce using llc. But in the actual code this module is fed into LLVMTargetMachineEmitToFile and those errors do show up.

Basing on the previous comments I assume that one of the functions for which such errors are printed is llvm.memset.p0.i64.
When llc processes the module.ll example that you have shared I see that calls to llvm.memset.p0.i64 are replaced by store instructions, e.g. the following IR:

%8 = call noundef ptr inttoptr (i64 1 to ptr)(ptr noundef nonnull @AYA_LOG_BUF, ptr noundef nonnull %2) #8
...
%68 = getelementptr inbounds i8, ptr %8, i64 62
...
call void @llvm.memset.p0.i64(ptr noundef nonnull align 1 dereferenceable(7) %68, i8 0, i64 7, i1 false)

Is converted to the following byte code:

r1 = AYA_LOG_BUF ll
call 1                 # bpf_map_lookup_elem
...
r1 = 0
*(u8 *)(r0 + 69) = w1
...
*(u8 *)(r0 + 62) = w1

This replacement happens when selection DAG is constructed (I assume this is default lowering logic for memset).
So, whatever is calling LLVMTargetMachineEmitToFile on your side must do some adjustments to the BPFISelLowering logic.
What tool does the LLVMTargetMachineEmitToFile call? Is it open source? Or am I confused?

[tamird]

Yeah, it's open source. All the code is in https://github.com/aya-rs/bpf-linker, and in particular the LLVMTargetMachineEmitToFile call is https://github.com/aya-rs/bpf-linker/blob/28bfade2a4c44911a8813b418e3bdfd39b9e3b6a/src/llvm/mod.rs#L279-L299

I've been mucking with this locally. I wrote a simple C program, that loads the IR and calls LLVMTargetMachineEmitToFile, and I can confirm that I *do not* see any warnings. Very strange. Here's the code: https://gist.github.com/tamird/e180a08d00a6572d95ae2035594ddd1e

I also modified bpf-linker to cycle the module to disk and back, and while I still see the error, the number of times it is emitted goes from 6 to 1 (!!).

diff
diff --git a/src/llvm/mod.rs b/src/llvm/mod.rs
index 4cc4181..f2e66fe 100644
--- a/src/llvm/mod.rs
+++ b/src/llvm/mod.rs
@@ -11,6 +11,7 @@ use std::{
 
 use libc::c_char as libc_char;
 use llvm_sys::bit_reader::*;
+use llvm_sys::ir_reader::*;
 use llvm_sys::core::*;
 use llvm_sys::debuginfo::LLVMStripModuleDebugInfo;
 use llvm_sys::linker::LLVMLinkModules2;
@@ -47,6 +48,7 @@ unsafe fn parse_command_line_options<T: AsRef<str>>(args: &[T], overview: &str)
         .collect::<Vec<_>>();
     let c_ptrs = c_args.iter().map(|s| s.as_ptr()).collect::<Vec<_>>();
     let overview = CString::new(overview).unwrap();
+    error!("args={:?}, overview={:?}", c_args, overview);
     LLVMParseCommandLineOptions(c_ptrs.len() as i32, c_ptrs.as_ptr(), overview.as_ptr());
 }
 
@@ -284,6 +286,28 @@ pub unsafe fn codegen(
 ) -> Result<(), String> {
     let mut message = Message::new();
 
+    let path = "/home/tamird/src/aya/test/integration-ebpf/module.ll";
+            // dump IR for the final linked module for debugging purposes
+            let path = CString::new(path).unwrap();
+
+        write_ir(module, path.as_c_str()).unwrap();
+
+                    let mut buffer = ptr::null_mut();                                                 
+            let ret = unsafe { LLVMCreateMemoryBufferWithContentsOfFile(path.as_ptr(), &mut buffer, ptr::null_mut()) };
+            assert_eq!(ret, 0);                                                                                                                                                              
+                                         
+            let mut module = ptr::null_mut();
+            let ret = unsafe { LLVMParseIRInContext(LLVMGetGlobalContext(), buffer, &mut module, ptr::null_mut()) };
+            assert_eq!(ret, 0);                                                                                                                                                              
+                                                                                                                                                                                             
+            error!("buffer={:p}", buffer);    
+
+            let triple = "bpfel-unknown-none";
+            let target = target_from_triple(CString::new(triple).unwrap().as_c_str()).unwrap();
+            let tm = create_target_machine(target, triple, "", "").unwrap();
+
     if LLVMTargetMachineEmitToFile(

Before:

23:45:03 [ERROR] llvm: <unknown>:0:0: in function test_log i32 (ptr): A call to built-in function 'memset' is not supported.
 
 23:45:03 [ERROR] llvm: <unknown>:0:0: in function test_log i32 (ptr): A call to built-in function 'memset' is not supported.
 
 23:45:03 [ERROR] llvm: <unknown>:0:0: in function test_log i32 (ptr): A call to built-in function 'memset' is not supported.
 
 23:45:03 [ERROR] llvm: <unknown>:0:0: in function test_log i32 (ptr): A call to built-in function 'memset' is not supported.
 
 23:45:03 [ERROR] llvm: <unknown>:0:0: in function test_log i32 (ptr): A call to built-in function 'memset' is not supported.
 
 23:45:03 [ERROR] llvm: <unknown>:0:0: in function test_log i32 (ptr): A call to built-in function 'memset' is not supported.

After:

23:51:04 [ERROR] buffer=0x5582fe80f5b0
error: <unknown>:0:0: in function test_log i32 (ptr): A call to built-in function 'memset' is not supported.

I do not understand what's going on here.

[tamird]

Actually, it seems that the change from multiple logged lines to a single one had to do with LLVMGetGlobalContext; if I pass the existing context then I get the same number of warning lines whether I cycle through disk or not.

[tamird]

Figured it out - if you pass --bpf-expand-memcpy-in-order to llc, you will reproduce the A call to built-in function 'memset' is not supported. prints.

[eddyz87]

In D155894#4532046, @tamird wrote:

Figured it out - if you pass --bpf-expand-memcpy-in-order to llc, you will reproduce the A call to built-in function 'memset' is not supported. prints.

I can see the error with this flag, thanks.

Also, here is a simpler example which could be triggered from clang:

$ cat test.c
extern void *memset(void *s, int c, unsigned long n);
extern void efoo(char *p);
void bar(char *p, unsigned long l) {
  memset(p, 0, l);
  efoo(p);
}

$ clang -O2 --target=bpf -mcpu=v3 test.c -emit-llvm -S -o test.ll
$ cat test.ll
...
; Function Attrs: nounwind
define dso_local void @bar(ptr noundef %p, i64 noundef %l) local_unnamed_addr #0 {
entry:
  tail call void @llvm.memset.p0.i64(ptr align 1 %p, i8 0, i64 %l, i1 false)
  tail call void @efoo(ptr noundef %p) #3
  ret void
}

; Function Attrs: mustprogress nocallback nofree nounwind willreturn memory(argmem: write)
declare void @llvm.memset.p0.i64(ptr nocapture writeonly, i8, i64, i1 immarg) #1

declare dso_local void @efoo(ptr noundef) local_unnamed_addr #2

attributes #0 = { nounwind "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="v3" }
attributes #1 = { mustprogress nocallback nofree nounwind willreturn memory(argmem: write) }
attributes #2 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="v3" }
attributes #3 = { nounwind }
...
$ llc -mcpu=v3 test.ll -o - 
error: <unknown>:0:0: in function bar void (ptr, i64): A call to built-in function 'memset' is not supported.

Callee: t21: i64 = GlobalAddress<ptr @efoo> 0
	.globl	bar                             # -- Begin function bar
	.p2align	3
	.type	bar,@function
bar:                                    # @bar
.Lbar$local:
	.type	.Lbar$local,@function
# %bb.0:                                # %entry
	r3 = r2
	r6 = r1
	w2 = 0
	call memset  # <------ Note the leftover call to `memset`
	r1 = r6
	call efoo
	exit
.Lfunc_end0:
	.size	bar, .Lfunc_end0-bar
	.size	.Lbar$local, .Lfunc_end0-bar
                                        # -- End function

The warnings would be reported for builtin extern functions (list is here: llvm/include/llvm/IR/RuntimeLibcalls.def).
But will not be reported for regular extern functions, like efoo in the example above.
As far as I understand all BPF code that uses clang uses llvm.memset patterns that could be statically expanded as a bunch of stores and BPF runtime does not really provide memset implementation.
So on one hand it probably would be better to retain this warning for clang, but on the other hand these warnings really belong to linker not compiler.
If libbpf is used as a linker I think it would report an error if code w/o suitable memset implementation is loaded.
Is there a way to pass a list of supported builtins as a parameter here?
@yonghong-song, what do you think?

[eddyz87]

To better reiterate my position on the warning for ExternalSymbolSDNode: in C analogy such warnings would be generated by the linker, not compiler.
So, I vote to agree with @tamird here and remove the warning (or at-least make it optional).

[tamird]

Updated to only allow external calls.

[ast]

llvm/lib/Target/BPF/BPFISelLowering.cpp

466

How does it help aya?

In general I think it's ok to remove this, but we need to define the behavior more accurately here.
Does it mean that we promote builtin to global ? What is the visibility of such symbol? What relocation do we use ? What libbpf and libbpf linker should do with it?

In particular with memcpy, do we generate asm that calls "llvm.memcpy.p0.p0.i64" name or does it become plain "memcpy" ?

[yonghong-song]

I applied this patch and did an experiment with bpftool linker functionality. The following are details.

First let us try one example with func mmemset which is not a builtin function.

$ cat t1.c                                                                                                                                  
#define __hidden __attribute__((visibility("hidden")))                                                                                                                      
__hidden extern void *mmemset(void *s, int c, unsigned long n);                                                                                                             
void do_memset(char *x, unsigned long l) {                                                                                                                                  
  mmemset(x, 0, l);                                                                                                                                                         
}                                                                                                                                                                           
$ cat t2.c                                                                                                                                  
void *mmemset(void *s, int c, unsigned long n);                                                                                                                             
void *mmemset(void *x, int c, unsigned long n) {
  // custom memset, I am using step of 2, otherwise the
  // compiler is smart enough and may transform
  // the code back to memset!
  char *ch = x;
  for (int i = 0; i < n; i+=2) ch[i] = c;
  return 0;
}
$ clang --target=bpf -O2 -g -c t1.c                    
$ clang --target=bpf -O2 -g -c t2.c                    
$ bpftool gen object final.o t1.o t2.o                 
$ llvm-readelf -s t1.o
   
Symbol table '.symtab' contains 13 entries: 
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT   ABS t1.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT     2 .text
     3: 0000000000000000     0 SECTION LOCAL  DEFAULT     4 .debug_loclists
     4: 0000000000000000     0 SECTION LOCAL  DEFAULT     5 .debug_abbrev
     5: 0000000000000000     0 SECTION LOCAL  DEFAULT     8 .debug_str_offsets
     6: 0000000000000000     0 SECTION LOCAL  DEFAULT    10 .debug_str
     7: 0000000000000000     0 SECTION LOCAL  DEFAULT    11 .debug_addr               
     8: 0000000000000000     0 SECTION LOCAL  DEFAULT    16 .debug_frame
     9: 0000000000000000     0 SECTION LOCAL  DEFAULT    18 .debug_line               
    10: 0000000000000000     0 SECTION LOCAL  DEFAULT    20 .debug_line_str
    11: 0000000000000000    32 FUNC    GLOBAL DEFAULT     2 do_memset                 
    12: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN    UND mmemset
$ llvm-readelf -s t2.o                                 
    
Symbol table '.symtab' contains 14 entries: 
   Num:    Value          Size Type    Bind   Vis       Ndx Name                      
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT   ABS t2.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT     2 .text
     3: 0000000000000038     0 NOTYPE  LOCAL  DEFAULT     2 LBB0_3
     4: 0000000000000010     0 NOTYPE  LOCAL  DEFAULT     2 LBB0_2
     5: 0000000000000000     0 SECTION LOCAL  DEFAULT     3 .debug_loclists
     6: 0000000000000000     0 SECTION LOCAL  DEFAULT     4 .debug_abbrev
     7: 0000000000000000     0 SECTION LOCAL  DEFAULT     7 .debug_str_offsets
     8: 0000000000000000     0 SECTION LOCAL  DEFAULT     9 .debug_str
     9: 0000000000000000     0 SECTION LOCAL  DEFAULT    10 .debug_addr
    10: 0000000000000000     0 SECTION LOCAL  DEFAULT    15 .debug_frame
    11: 0000000000000000     0 SECTION LOCAL  DEFAULT    17 .debug_line
    12: 0000000000000000     0 SECTION LOCAL  DEFAULT    19 .debug_line_str
    13: 0000000000000000    72 FUNC    GLOBAL DEFAULT     2 mmemset
$ llvm-readelf -s final.o

Symbol table '.symtab' contains 8 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT   ABS t1.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT     3 .text
     3: 0000000000000000    32 FUNC    GLOBAL DEFAULT     3 do_memset
     4: 0000000000000020    72 FUNC    GLOBAL HIDDEN      3 mmemset
     5: 0000000000000000     0 FILE    LOCAL  DEFAULT   ABS t2.c
     6: 0000000000000058     0 NOTYPE  LOCAL  DEFAULT     3 LBB0_3
     7: 0000000000000030     0 NOTYPE  LOCAL  DEFAULT     3 LBB0_2

So bpftool linking succeeded with global function mmemset.

Now let rename the mmemset to memset in the above t1.c and t2.c.

$ cat t1.c
#define __hidden __attribute__((visibility("hidden")))
__hidden extern void *memset(void *s, int c, unsigned long n);
void do_memset(char *x, unsigned long l) {
  memset(x, 0, l);
}
$ cat t2.c
void *memset(void *s, int c, unsigned long n);
void *memset(void *x, int c, unsigned long n) {
  // custom memset, I am using step of 2, otherwise the
  // compiler is smart enough and may transform
  // the code back to memset!
  char *ch = x;
  for (int i = 0; i < n; i+=2) ch[i] = c;
  return 0;
}
$ clang --target=bpf -O2 -g -c t1.c
$ clang --target=bpf -O2 -g -c t2.c
$ bpftool gen object final.o t1.o t2.o
libbpf: failed to find BTF info for global/extern symbol 'memset'
Error: failed to link 't1.o': No such file or directory (2)
$ llvm-readelf -s t1.o

Symbol table '.symtab' contains 13 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT   ABS t1.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT     2 .text
     3: 0000000000000000     0 SECTION LOCAL  DEFAULT     4 .debug_loclists
     4: 0000000000000000     0 SECTION LOCAL  DEFAULT     5 .debug_abbrev
     5: 0000000000000000     0 SECTION LOCAL  DEFAULT     8 .debug_str_offsets
     6: 0000000000000000     0 SECTION LOCAL  DEFAULT    10 .debug_str
     7: 0000000000000000     0 SECTION LOCAL  DEFAULT    11 .debug_addr
     8: 0000000000000000     0 SECTION LOCAL  DEFAULT    16 .debug_frame
     9: 0000000000000000     0 SECTION LOCAL  DEFAULT    18 .debug_line
    10: 0000000000000000     0 SECTION LOCAL  DEFAULT    20 .debug_line_str
    11: 0000000000000000    32 FUNC    GLOBAL DEFAULT     2 do_memset
    12: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT   UND memset
$ llvm-readelf -s t2.o

Symbol table '.symtab' contains 14 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT   ABS t2.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT     2 .text
     3: 0000000000000038     0 NOTYPE  LOCAL  DEFAULT     2 LBB0_3
     4: 0000000000000010     0 NOTYPE  LOCAL  DEFAULT     2 LBB0_2
     5: 0000000000000000     0 SECTION LOCAL  DEFAULT     3 .debug_loclists
     6: 0000000000000000     0 SECTION LOCAL  DEFAULT     4 .debug_abbrev
     7: 0000000000000000     0 SECTION LOCAL  DEFAULT     7 .debug_str_offsets
     8: 0000000000000000     0 SECTION LOCAL  DEFAULT     9 .debug_str
     9: 0000000000000000     0 SECTION LOCAL  DEFAULT    10 .debug_addr
    10: 0000000000000000     0 SECTION LOCAL  DEFAULT    15 .debug_frame
    11: 0000000000000000     0 SECTION LOCAL  DEFAULT    17 .debug_line
    12: 0000000000000000     0 SECTION LOCAL  DEFAULT    19 .debug_line_str
    13: 0000000000000000    72 FUNC    GLOBAL DEFAULT     2 memset

The major difference is in t1.o,

    12: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN    UND mmemset
vs.
    12: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT   UND memset

So for builtin function memset, the 'visibility' is not set to HIDDEN although
we have an attribute for it.

I would like to have more study for this to understand why and how to fix it
before we lift the error message.

[yonghong-song]

I checked the code and found that ExternalSymbolSDNode is indeed corresponding to builtin/library functions.
In SelectionDAG/SelectionDAG.cpp, we have

                    getExternalSymbol(TLI->getLibcallName(RTLIB::MEMCPY),
...
                    getExternalSymbol(TLI->getLibcallName(LibraryCall),
...
                    getExternalSymbol(TLI->getLibcallName(RTLIB::MEMMOVE),
...
                    getExternalSymbol(TLI->getLibcallName(LibraryCall),
...
        getExternalSymbol(BzeroName, TLI->getPointerTy(DL)), std::move(Args));
...
                     getExternalSymbol(TLI->getLibcallName(RTLIB::MEMSET),
...
                    getExternalSymbol(TLI->getLibcallName(LibraryCall),
...
  SDValue Callee = getExternalSymbol(TLI->getLibcallName(LC),

[tamird]

llvm/lib/Target/BPF/BPFISelLowering.cpp

466

How does it help aya?

In aya, we provide implementations of these built-ins (memset, memcpy, etc.). In bpf-linker, we combine all the IR into a single module before codegen. Currently this error fires, which we must treat as fatal or ignore by string matching.

It turns out that codegen generates a call to plain "memset", and the resulting code is completely valid. So this error is spurious.

In general I think it's ok to remove this, but we need to define the behavior more accurately here.
Does it mean that we promote builtin to global ? What is the visibility of such symbol? What relocation do we use ? What libbpf and libbpf linker should do with it?

Looks like yonghong discovered that this symbol is indeed promoted to global.

In particular with memcpy, do we generate asm that calls "llvm.memcpy.p0.p0.i64" name or does it become plain "memcpy" ?

Yes, it becomes "plain".

[tamird]

@yonghong-song I have learned some things. If you repeat your experiment (even without this patch) but pass -fno-builtin when compiling t1.c, everything works.

Here's the diff in IR:

diff --git a/t1.ll b/t1-fno-builtin.ll
index 64d172bf9ab5..324a98687ade 100644
--- a/t1.ll
+++ b/t1-fno-builtin.ll
@@ -3,24 +3,24 @@ source_filename = "t1.c"
 target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128"
 target triple = "bpf"
 
-; Function Attrs: mustprogress nofree nosync nounwind willreturn memory(argmem: write)
-define dso_local void @do_memset(ptr nocapture noundef writeonly %x, i64 noundef %l) local_unnamed_addr #0 !dbg !8 {
+; Function Attrs: nounwind
+define dso_local void @do_memset(ptr noundef %x, i64 noundef %l) local_unnamed_addr #0 !dbg !8 {
 entry:
   call void @llvm.dbg.value(metadata ptr %x, metadata !15, metadata !DIExpression()), !dbg !17
   call void @llvm.dbg.value(metadata i64 %l, metadata !16, metadata !DIExpression()), !dbg !17
-  tail call void @llvm.memset.p0.i64(ptr align 1 %x, i8 0, i64 %l, i1 false), !dbg !18
+  %call = tail call ptr @memset(ptr noundef %x, i32 noundef 0, i64 noundef %l) #3, !dbg !18
   ret void, !dbg !19
 }
 
-; Function Attrs: mustprogress nocallback nofree nounwind willreturn memory(argmem: write)
-declare void @llvm.memset.p0.i64(ptr nocapture writeonly, i8, i64, i1 immarg) #1
+declare !dbg !20 hidden ptr @memset(ptr noundef, i32 noundef, i64 noundef) local_unnamed_addr #1
 
 ; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
 declare void @llvm.dbg.value(metadata, metadata, metadata) #2
 
-attributes #0 = { mustprogress nofree nosync nounwind willreturn memory(argmem: write) "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
-attributes #1 = { mustprogress nocallback nofree nounwind willreturn memory(argmem: write) }
+attributes #0 = { nounwind "frame-pointer"="all" "no-builtins" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #1 = { "frame-pointer"="all" "no-builtins" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
 attributes #2 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+attributes #3 = { nobuiltin nounwind "no-builtins" }
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!2, !3, !4, !5, !6}
@@ -46,3 +46,12 @@ attributes #2 = { nocallback nofree nosync nounwind speculatable willreturn memo
 !17 = !DILocation(line: 0, scope: !8)
 !18 = !DILocation(line: 4, column: 3, scope: !8)
 !19 = !DILocation(line: 5, column: 1, scope: !8)
+!20 = !DISubprogram(name: "memset", scope: !1, file: !1, line: 2, type: !21, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !25)
+!21 = !DISubroutineType(types: !22)
+!22 = !{!23, !23, !24, !13}
+!23 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!24 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!25 = !{!26, !27, !28}
+!26 = !DILocalVariable(name: "s", arg: 1, scope: !20, file: !1, line: 2, type: !23)
+!27 = !DILocalVariable(name: "c", arg: 2, scope: !20, file: !1, line: 2, type: !24)
+!28 = !DILocalVariable(name: "n", arg: 3, scope: !20, file: !1, line: 2, type: !13)

The same result is obtained if I annotate do_memset with __attribute__((no_builtin("memset"))):

diff --git a/t1.c b/t1-no-builtin.c
index 2698b334b898..3cded7c57e99 100644
--- a/t1.c
+++ b/t1-no-builtin.c
@@ -1,5 +1,5 @@
 #define __hidden __attribute__((visibility("hidden")))
 __hidden extern void *memset(void *s, int c, unsigned long n);
-void do_memset(char *x, unsigned long l) {
+void do_memset(char *x, unsigned long l) __attribute__((no_builtin("memset"))) {
   memset(x, 0, l);
 }

diff --git a/t1.ll b/t1-no-builtin.ll
index 64d172bf9ab5..f9a2b088084e 100644
--- a/t1.ll
+++ b/t1-no-builtin.ll
@@ -1,25 +1,25 @@
-; ModuleID = 't1.c'
-source_filename = "t1.c"
+; ModuleID = 't1-no-builtin.c'
+source_filename = "t1-no-builtin.c"
 target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128"
 target triple = "bpf"
 
-; Function Attrs: mustprogress nofree nosync nounwind willreturn memory(argmem: write)
-define dso_local void @do_memset(ptr nocapture noundef writeonly %x, i64 noundef %l) local_unnamed_addr #0 !dbg !8 {
+; Function Attrs: mustprogress nofree nounwind willreturn memory(argmem: readwrite)
+define dso_local void @do_memset(ptr noundef %x, i64 noundef %l) local_unnamed_addr #0 !dbg !8 {
 entry:
   call void @llvm.dbg.value(metadata ptr %x, metadata !15, metadata !DIExpression()), !dbg !17
   call void @llvm.dbg.value(metadata i64 %l, metadata !16, metadata !DIExpression()), !dbg !17
-  tail call void @llvm.memset.p0.i64(ptr align 1 %x, i8 0, i64 %l, i1 false), !dbg !18
+  %call = tail call ptr @memset(ptr noundef %x, i32 noundef 0, i64 noundef %l), !dbg !18
   ret void, !dbg !19
 }
 
-; Function Attrs: mustprogress nocallback nofree nounwind willreturn memory(argmem: write)
-declare void @llvm.memset.p0.i64(ptr nocapture writeonly, i8, i64, i1 immarg) #1
+; Function Attrs: mustprogress nofree nounwind willreturn memory(argmem: readwrite)
+declare !dbg !20 hidden ptr @memset(ptr noundef writeonly, i32 noundef, i64 noundef) local_unnamed_addr #1
 
 ; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
 declare void @llvm.dbg.value(metadata, metadata, metadata) #2
 
-attributes #0 = { mustprogress nofree nosync nounwind willreturn memory(argmem: write) "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
-attributes #1 = { mustprogress nocallback nofree nounwind willreturn memory(argmem: write) }
+attributes #0 = { mustprogress nofree nounwind willreturn memory(argmem: readwrite) "frame-pointer"="all" "no-builtin-memset" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #1 = { mustprogress nofree nounwind willreturn memory(argmem: readwrite) "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
 attributes #2 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
 
 !llvm.dbg.cu = !{!0}
@@ -27,7 +27,7 @@ attributes #2 = { nocallback nofree nosync nounwind speculatable willreturn memo
 !llvm.ident = !{!7}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 17.0.0 (https://github.com/llvm/llvm-project.git 52807ba3836ef30c54622f227c4a23e2eb9375cd)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
-!1 = !DIFile(filename: "t1.c", directory: "/home/tamird/src/llvm-project-latest/foo", checksumkind: CSK_MD5, checksum: "7d959830f84c1a82a1efb0fbb5b9c592")
+!1 = !DIFile(filename: "t1-no-builtin.c", directory: "/home/tamird/src/llvm-project-latest/foo", checksumkind: CSK_MD5, checksum: "4a930bd64fe9fcad8934a5f1a66a94d4")
 !2 = !{i32 7, !"Dwarf Version", i32 5}
 !3 = !{i32 2, !"Debug Info Version", i32 3}
 !4 = !{i32 1, !"wchar_size", i32 4}
@@ -46,3 +46,12 @@ attributes #2 = { nocallback nofree nosync nounwind speculatable willreturn memo
 !17 = !DILocation(line: 0, scope: !8)
 !18 = !DILocation(line: 4, column: 3, scope: !8)
 !19 = !DILocation(line: 5, column: 1, scope: !8)
+!20 = !DISubprogram(name: "memset", scope: !1, file: !1, line: 2, type: !21, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !25)
+!21 = !DISubroutineType(types: !22)
+!22 = !{!23, !23, !24, !13}
+!23 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!24 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!25 = !{!26, !27, !28}
+!26 = !DILocalVariable(name: "s", arg: 1, scope: !20, file: !1, line: 2, type: !23)
+!27 = !DILocalVariable(name: "c", arg: 2, scope: !20, file: !1, line: 2, type: !24)
+!28 = !DILocalVariable(name: "n", arg: 3, scope: !20, file: !1, line: 2, type: !13)

bpftool gen object works with both of these.

I don't entirely understand the mechanism that causes memset to end up not-hidden, but it seems that BPF should always imply -fno-builtin for this reason. I've sent a PR to this effect in aya: https://github.com/aya-rs/aya/pull/698.

I'll abandon this, but it may be worthwhile to implement BPF-implies-fno-builtin *in clang*, if possible.

[tamird]

In D155894#4529382, @eddyz87 wrote:

If we want to report this for multiple functions I think we can set some flag and exit in TargetLoweringBase::finalizeLowering().

Doesn't this still run at the end of a function, rather than at the end of an entire file? We can report multiple functions have errored, but we can't emit all the errors in the file.

[eddyz87]

In D155894#4540874, @tamird wrote:

In D155894#4529382, @eddyz87 wrote:

If we want to report this for multiple functions I think we can set some flag and exit in TargetLoweringBase::finalizeLowering().

Doesn't this still run at the end of a function, rather than at the end of an entire file? We can report multiple functions have errored, but we can't emit all the errors in the file.

Indeed it does. My bad, for some reason I thought it does so at the end of module. And I don't see any hook that could be executed at the end of the module. Still, imho, backend should not proceed with compilation in such cases even if only first error is reported.

[tamird]

In D155894#4542263, @eddyz87 wrote:

In D155894#4540874, @tamird wrote:

In D155894#4529382, @eddyz87 wrote:

If we want to report this for multiple functions I think we can set some flag and exit in TargetLoweringBase::finalizeLowering().

Doesn't this still run at the end of a function, rather than at the end of an entire file? We can report multiple functions have errored, but we can't emit all the errors in the file.

Indeed it does. My bad, for some reason I thought it does so at the end of module. And I don't see any hook that could be executed at the end of the module. Still, imho, backend should not proceed with compilation in such cases even if only first error is reported.

Yes, we fixed the embedder. The question was is it possible to harden this such that future embedders don't make the same mistake.

[ast]

llvm/lib/Target/BPF/BPFISelLowering.cpp

466

Since backend couldn't unroll memset it should error hard.
The linking step doesn't exist most of the time.
From the user perspective it's better to fail at compile time instead of program loading time where the verifier errors are obscure.
I think it would be good to always unroll memset into a loop even when it's huge.
And similar with memcpy.
-fno-builtin and attribute((no_builtin)) tricks are unrelated to any of this.

[yonghong-song]

I'll abandon this, but it may be worthwhile to implement BPF-implies-fno-builtin *in clang*, if possible.

Thanks @tamird Indeed -fno-builtin or attribute no-builtin can fix the issue. But I think we should not put no-builtin as default since based on my experiment a lot of users using memset with a known constant size, expecting the compiler will transform it into a load, and we do not want to break such use cases. But we could advise users to use -fno-builtin or no-builtin attribute if their usage of memset involves a unknown size to a very large known size. This way, we can keep the fail message here since survival of builtin memset func means bpftool linking cannot resolve it.

[tamird]

llvm/lib/Target/BPF/BPFISelLowering.cpp

466

Right. The no_builtin tricks are too big of hammers for what we want here, which is to still apply constant size optimizations but not apply special treatment to these functions when they've been defined by the user, which is what @yonghong-song was saying in his earlier comments. I may dig into that when I have some time, but for now we're just ignoring these errors in aya's linker.

Is there a place (issue tracker, mailing list, other) to document these musings and describe possible future work?

[ast]

I may dig into that when I have some time, but for now we're just ignoring these errors in aya's linker.

what do you mean "ignoring in aya" ?
If bpf prog contains a call to memset and you link it with a generic unbounded loop in aya linker the verifier will reject such code.
I thought aya suppose to produce valid programs.

[alessandrod]

In D155894#4544073, @ast wrote:

I may dig into that when I have some time, but for now we're just ignoring these errors in aya's linker.

what do you mean "ignoring in aya" ?
If bpf prog contains a call to memset and you link it with a generic unbounded loop in aya linker the verifier will reject such code.
I thought aya suppose to produce valid programs.

We're ignoring the error because the error is in fact not an error, the target is doing the right thing, and aya (well, rustc) always provides memory builtins so memset gets linked correctly and a valid program is produced.

[ast]

In D155894#4544080, @alessandrod wrote:

In D155894#4544073, @ast wrote:

I may dig into that when I have some time, but for now we're just ignoring these errors in aya's linker.

what do you mean "ignoring in aya" ?
If bpf prog contains a call to memset and you link it with a generic unbounded loop in aya linker the verifier will reject such code.
I thought aya suppose to produce valid programs.

We're ignoring the error because the error is in fact not an error, the target is doing the right thing, and aya (well, rustc) always provides memory builtins so memset gets linked correctly and a valid program is produced.

Could you point to memset implementation that gets linked eventually?
Is it a global function or static ? Asking because they're verified differently by the kernel.
I have a hard time imagining a generic implementation that can pass the verifier.

[alessandrod]

In D155894#4545019, @ast wrote:

In D155894#4544080, @alessandrod wrote:

We're ignoring the error because the error is in fact not an error, the target is doing the right thing, and aya (well, rustc) always provides memory builtins so memset gets linked correctly and a valid program is produced.

Could you point to memset implementation that gets linked eventually?

Normally in rust programs, memory builtins are provided by the compiler-builtins crate, which is the equivalent of compiler-rt in LLVM. It's automatically linked in by the compiler to provide libcalls used by LLVM. This is the code https://github.com/rust-lang/compiler-builtins/blob/efd227f9c2ac220a8699188e937eb8be810df317/src/mem/mod.rs#L46. As you see it returns the dst pointer, which breaks the verifier if it's used on stack pointers (which is something rustc sometimes does when initializing stack values). So we have an override in aya that is pretty much the same but returns void.

Is it a global function or static ? Asking because they're verified differently by the kernel.
I have a hard time imagining a generic implementation that can pass the verifier.

I am aware - it's a static so it works. And also the bounds for the inputs are statically known in all the cases where rustc uses memset internally (eg to zero initialize values with a fixed known type layout). The usual bound checks are required when calling something like https://doc.rust-lang.org/stable/std/primitive.slice.html#method.copy_from_slice.

llvm/lib/Target/BPF/BPFISelLowering.h

private:
   // Lower the result values of a call, copying them out of physregs into vregs
   SDValue LowerCallResult(SDValue Chain, SDValue InGlue,
                           CallingConv::ID CallConv, bool IsVarArg,
                           const SmallVectorImpl<ISD::InputArg> &Ins,
                           const SDLoc &DL, SelectionDAG &DAG,
                           SmallVectorImpl<SDValue> &InVals) const;
 
   // Maximum number of arguments to a call
-  static const unsigned MaxArgs;
+  static const size_t MaxArgs;
 
   // Lower a call into CALLSEQ_START - BPFISD:CALL - CALLSEQ_END chain
   SDValue LowerCall(TargetLowering::CallLoweringInfo &CLI,
                     SmallVectorImpl<SDValue> &InVals) const override;
 
   // Lower incoming arguments, copy physregs into vregs
   SDValue LowerFormalArguments(SDValue Chain, CallingConv::ID CallConv,
                                bool IsVarArg,

llvm/lib/Target/BPF/BPFISelLowering.cpp

 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetLoweringObjectFileImpl.h"
 #include "llvm/CodeGen/ValueTypes.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
+#include <optional>
 using namespace llvm;
 
 #define DEBUG_TYPE "bpf-lower"
 
 static cl::opt<bool> BPFExpandMemcpyInOrder("bpf-expand-memcpy-in-order",
   cl::Hidden, cl::init(false),
   cl::desc("Expand memcpy into load/store pairs in order"));
 
-static void fail(const SDLoc &DL, SelectionDAG &DAG, const Twine &Msg) {
-  MachineFunction &MF = DAG.getMachineFunction();
-  DAG.getContext()->diagnose(
-      DiagnosticInfoUnsupported(MF.getFunction(), Msg, DL.getDebugLoc()));
-}
-
-static void fail(const SDLoc &DL, SelectionDAG &DAG, const char *Msg,
-                 SDValue Val) {
-  MachineFunction &MF = DAG.getMachineFunction();
+static void unsupported(const SDLoc &DL, SelectionDAG &DAG, const Twine &Msg,
+                        std::optional<SDValue> Val = std::nullopt) {
   std::string Str;
+  if (Val.has_value()) {
     raw_string_ostream OS(Str);
-  OS << Msg;
-  Val->print(OS);
-  OS.flush();
-  DAG.getContext()->diagnose(
-      DiagnosticInfoUnsupported(MF.getFunction(), Str, DL.getDebugLoc()));
+    (*Val)->print(OS);
+    OS << ' ';
+  }
+  MachineFunction &MF = DAG.getMachineFunction();
+  DAG.getContext()->diagnose(DiagnosticInfoUnsupported(
+      MF.getFunction(), Msg.concat(Str), DL.getDebugLoc()));
 }
 
 BPFTargetLowering::BPFTargetLowering(const TargetMachine &TM,
                                      const BPFSubtarget &STI)
     : TargetLowering(TM) {
 
   // Set up the register classes.
   addRegisterClass(MVT::i64, &BPF::GPRRegClass);
-  if (STI.getHasAlu32())
+  if (STI.getHasAlu32()) {
     addRegisterClass(MVT::i32, &BPF::GPR32RegClass);
+  }
 
   // Compute derived properties from the register classes
   computeRegisterProperties(STI.getRegisterInfo());
 
   setStackPointerRegisterToSaveRestore(BPF::R11);
 
   setOperationAction(ISD::BR_CC, MVT::i64, Custom);
   setOperationAction(ISD::BR_JT, MVT::Other, Expand);
   setOperationAction(ISD::BRIND, MVT::Other, Expand);
   setOperationAction(ISD::BRCOND, MVT::Other, Expand);
 
   setOperationAction(ISD::GlobalAddress, MVT::i64, Custom);
 
   setOperationAction(ISD::DYNAMIC_STACKALLOC, MVT::i64, Custom);
   setOperationAction(ISD::STACKSAVE, MVT::Other, Expand);
   setOperationAction(ISD::STACKRESTORE, MVT::Other, Expand);
 
   // Set unsupported atomic operations as Custom so
   // we can emit better error messages than fatal error
   // from selectiondag.
   for (auto VT : {MVT::i8, MVT::i16, MVT::i32}) {
     if (VT == MVT::i32) {
-      if (STI.getHasAlu32())
+      if (STI.getHasAlu32()) {
         continue;
+      }
     } else {
       setOperationAction(ISD::ATOMIC_LOAD_ADD, VT, Custom);
     }
 
     setOperationAction(ISD::ATOMIC_LOAD_AND, VT, Custom);
     setOperationAction(ISD::ATOMIC_LOAD_OR, VT, Custom);
     setOperationAction(ISD::ATOMIC_LOAD_XOR, VT, Custom);
     setOperationAction(ISD::ATOMIC_SWAP, VT, Custom);
     setOperationAction(ISD::ATOMIC_CMP_SWAP_WITH_SUCCESS, VT, Custom);
   }
 
   for (auto VT : { MVT::i32, MVT::i64 }) {
-    if (VT == MVT::i32 && !STI.getHasAlu32())
+    if (VT == MVT::i32 && !STI.getHasAlu32()) {
       continue;
+    }
 
     setOperationAction(ISD::SDIVREM, VT, Expand);
     setOperationAction(ISD::UDIVREM, VT, Expand);
     setOperationAction(ISD::SREM, VT, Expand);
     setOperationAction(ISD::MULHU, VT, Expand);
     setOperationAction(ISD::MULHS, VT, Expand);
     setOperationAction(ISD::UMUL_LOHI, VT, Expand);
     setOperationAction(ISD::SMUL_LOHI, VT, Expand);
BPFTargetLowering::BPFTargetLowering(const TargetMachine &TM,
   HasJmpExt = STI.getHasJmpExt();
 }
 
 bool BPFTargetLowering::isOffsetFoldingLegal(const GlobalAddressSDNode *GA) const {
   return false;
 }
 
 bool BPFTargetLowering::isTruncateFree(Type *Ty1, Type *Ty2) const {
-  if (!Ty1->isIntegerTy() || !Ty2->isIntegerTy())
+  if (!Ty1->isIntegerTy() || !Ty2->isIntegerTy()) {
     return false;
+  }
   unsigned NumBits1 = Ty1->getPrimitiveSizeInBits();
   unsigned NumBits2 = Ty2->getPrimitiveSizeInBits();
   return NumBits1 > NumBits2;
 }
 
 bool BPFTargetLowering::isTruncateFree(EVT VT1, EVT VT2) const {
-  if (!VT1.isInteger() || !VT2.isInteger())
+  if (!VT1.isInteger() || !VT2.isInteger()) {
     return false;
+  }
   unsigned NumBits1 = VT1.getSizeInBits();
   unsigned NumBits2 = VT2.getSizeInBits();
   return NumBits1 > NumBits2;
 }
 
 bool BPFTargetLowering::isZExtFree(Type *Ty1, Type *Ty2) const {
-  if (!getHasAlu32() || !Ty1->isIntegerTy() || !Ty2->isIntegerTy())
+  if (!getHasAlu32() || !Ty1->isIntegerTy() || !Ty2->isIntegerTy()) {
     return false;
+  }
   unsigned NumBits1 = Ty1->getPrimitiveSizeInBits();
   unsigned NumBits2 = Ty2->getPrimitiveSizeInBits();
   return NumBits1 == 32 && NumBits2 == 64;
 }
 
 bool BPFTargetLowering::isZExtFree(EVT VT1, EVT VT2) const {
-  if (!getHasAlu32() || !VT1.isInteger() || !VT2.isInteger())
+  if (!getHasAlu32() || !VT1.isInteger() || !VT2.isInteger()) {
     return false;
+  }
   unsigned NumBits1 = VT1.getSizeInBits();
   unsigned NumBits2 = VT2.getSizeInBits();
   return NumBits1 == 32 && NumBits2 == 64;
 }
 
 BPFTargetLowering::ConstraintType
 BPFTargetLowering::getConstraintType(StringRef Constraint) const {
   if (Constraint.size() == 1) {
     switch (Constraint[0]) {
     default:
       break;
     case 'w':
       return C_RegisterClass;
     }
   }
 
   return TargetLowering::getConstraintType(Constraint);
 }
 
 std::pair<unsigned, const TargetRegisterClass *>
 BPFTargetLowering::getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
                                                 StringRef Constraint,
                                                 MVT VT) const {
-  if (Constraint.size() == 1)
+  if (Constraint.size() == 1) {
     // GCC Constraint Letters
     switch (Constraint[0]) {
     case 'r': // GENERAL_REGS
       return std::make_pair(0U, &BPF::GPRRegClass);
     case 'w':
-      if (HasAlu32)
+      if (HasAlu32) {
         return std::make_pair(0U, &BPF::GPR32RegClass);
+      }
       break;
     default:
       break;
     }
+  }
 
   return TargetLowering::getRegForInlineAsmConstraint(TRI, Constraint, VT);
 }
 
 void BPFTargetLowering::ReplaceNodeResults(
   SDNode *N, SmallVectorImpl<SDValue> &Results, SelectionDAG &DAG) const {
-  const char *err_msg;
+  const char *Msg;
   uint32_t Opcode = N->getOpcode();
   switch (Opcode) {
-  default:
-    report_fatal_error("Unhandled custom legalization");
+  default: {
+    report_fatal_error("unhandled custom legalization: " + Twine(Opcode));
+  }
   case ISD::ATOMIC_LOAD_ADD:
   case ISD::ATOMIC_LOAD_AND:
   case ISD::ATOMIC_LOAD_OR:
   case ISD::ATOMIC_LOAD_XOR:
   case ISD::ATOMIC_SWAP:
   case ISD::ATOMIC_CMP_SWAP_WITH_SUCCESS:
-    if (HasAlu32 || Opcode == ISD::ATOMIC_LOAD_ADD)
-      err_msg = "Unsupported atomic operations, please use 32/64 bit version";
-    else
-      err_msg = "Unsupported atomic operations, please use 64 bit version";
+    if (HasAlu32 || Opcode == ISD::ATOMIC_LOAD_ADD) {
+      Msg = "unsupported atomic operation, please use 32/64 bit version";
+    } else {
+      Msg = "unsupported atomic operation, please use 64 bit version";
+    }
     break;
   }
 
   SDLoc DL(N);
-  fail(DL, DAG, err_msg);
+  // We'll still produce a fatal error downstream, but this diagnostic is more
+  // user-friendly.
+  unsupported(DL, DAG, Msg);
 }
 
 SDValue BPFTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
   switch (Op.getOpcode()) {
+  default: {
+    report_fatal_error("unimplemented opcode: " + Twine(Op.getOpcode()));
+  }
   case ISD::BR_CC:
     return LowerBR_CC(Op, DAG);
   case ISD::GlobalAddress:
     return LowerGlobalAddress(Op, DAG);
   case ISD::SELECT_CC:
     return LowerSELECT_CC(Op, DAG);
   case ISD::DYNAMIC_STACKALLOC:
-    report_fatal_error("Unsupported dynamic stack allocation");
-  default:
-    llvm_unreachable("unimplemented operand");
+    report_fatal_error("unsupported dynamic stack allocation");
   }
 }
 
 // Calling Convention Implementation
 #include "BPFGenCallingConv.inc"
 
 SDValue BPFTargetLowering::LowerFormalArguments(
     SDValue Chain, CallingConv::ID CallConv, bool IsVarArg,
     const SmallVectorImpl<ISD::InputArg> &Ins, const SDLoc &DL,
     SelectionDAG &DAG, SmallVectorImpl<SDValue> &InVals) const {
   switch (CallConv) {
-  default:
-    report_fatal_error("Unsupported calling convention");
+  default: {
+    report_fatal_error("unimplemented calling convention: " + Twine(CallConv));
+  }
   case CallingConv::C:
   case CallingConv::Fast:
     break;
   }
 
   MachineFunction &MF = DAG.getMachineFunction();
   MachineRegisterInfo &RegInfo = MF.getRegInfo();
 
   // Assign locations to all of the incoming arguments.
   SmallVector<CCValAssign, 16> ArgLocs;
   CCState CCInfo(CallConv, IsVarArg, MF, ArgLocs, *DAG.getContext());
   CCInfo.AnalyzeFormalArguments(Ins, getHasAlu32() ? CC_BPF32 : CC_BPF64);
 
-  for (auto &VA : ArgLocs) {
+  bool HasMemArgs = false;
+  for (size_t I = 0; I < ArgLocs.size(); ++I) {
+    auto &VA = ArgLocs[I];
+
     if (VA.isRegLoc()) {
       // Arguments passed in registers
       EVT RegVT = VA.getLocVT();
       MVT::SimpleValueType SimpleTy = RegVT.getSimpleVT().SimpleTy;
       switch (SimpleTy) {
       default: {
-        errs() << "LowerFormalArguments Unhandled argument type: "
-               << RegVT << '\n';
-        llvm_unreachable(nullptr);
+        std::string Str;
+        {
+          raw_string_ostream OS(Str);
+          RegVT.print(OS);
+        }
+        report_fatal_error("unhandled argument type: " + Twine(Str));
       }
       case MVT::i32:
       case MVT::i64:
         Register VReg = RegInfo.createVirtualRegister(
             SimpleTy == MVT::i64 ? &BPF::GPRRegClass : &BPF::GPR32RegClass);
         RegInfo.addLiveIn(VA.getLocReg(), VReg);
         SDValue ArgValue = DAG.getCopyFromReg(Chain, DL, VReg, RegVT);
 
         // If this is an value that has been promoted to wider types, insert an
         // assert[sz]ext to capture this, then truncate to the right size.
-        if (VA.getLocInfo() == CCValAssign::SExt)
+        if (VA.getLocInfo() == CCValAssign::SExt) {
           ArgValue = DAG.getNode(ISD::AssertSext, DL, RegVT, ArgValue,
                                  DAG.getValueType(VA.getValVT()));
-        else if (VA.getLocInfo() == CCValAssign::ZExt)
+        } else if (VA.getLocInfo() == CCValAssign::ZExt) {
           ArgValue = DAG.getNode(ISD::AssertZext, DL, RegVT, ArgValue,
                                  DAG.getValueType(VA.getValVT()));
+        }
 
-        if (VA.getLocInfo() != CCValAssign::Full)
+        if (VA.getLocInfo() != CCValAssign::Full) {
           ArgValue = DAG.getNode(ISD::TRUNCATE, DL, VA.getValVT(), ArgValue);
+        }
 
         InVals.push_back(ArgValue);
 
-  break;
+        break;
       }
-    } else {
-      fail(DL, DAG, "defined with too many args");
-      InVals.push_back(DAG.getConstant(0, DL, VA.getLocVT()));
+    } else if (VA.isMemLoc()) {
+      // NB: An earlier version of this code inserted constant zeros for "a
+      // chance that generated code is somewhat sane" (see comment in
+      // https://reviews.llvm.org/D20571?id=58249).
+      //
+      // Miscompiling and hoping that the result is "sane" is not a viable
+      // strategy. We should probably fatal here, but doing so has various
+      // downsides (e.g. subsequent errors will not reach the user, tests must
+      // be exit-on-error). The current approach (inserting UNDEF) produces
+      // rather obtuse verifier errors, roughly:
+      //
+      // clang-format off
+      // 320: (b7) r1 = 32                     ; R1_w=32
+      // 321: (b7) r2 = 1                      ; R2_w=1
+      // 322: (b7) r3 = 13                     ; R3_w=13
+      // 323: (b7) r4 = 184                    ; R4_w=184
+      // 324: (b7) r5 = 0                      ; R5_w=0
+      // 325: (85) call pc+665
+      // caller:
+      //  R6=17 R7=map_value(off=0,ks=4,vs=8192,imm=0) R8=0 R9=4 R10=fp0 fp-8=mmmm???? fp-16=ctx
+      // callee:
+      //  frame1: R1_w=32 R2_w=1 R3_w=13 R4_w=184 R5_w=0 R10=fp0
+      // 991: (b7) r0 = 1                      ; frame1: R0_w=1
+      // 992: (b7) r6 = 19                     ; frame1: R6_w=19
+      // 993: (2d) if r6 > r1 goto pc+22       ; frame1: R1=32 R6_w=19
+      // 994: (b7) r0 = 16                     ; frame1: R0_w=16
+      // 995: (73) *(u8 *)(r1 +1) = r0
+      // R1 invalid mem access 'scalar'
+      // verification time 522 usec
+      // stack depth 16+0+0+0
+      // processed 440 insns (limit 1000000) max_states_per_insn 1 total_states 15 peak_states 15 mark_read 8
+      // clang-format on
+      //
+      // It's not clear what the best thing to do here is. For now, err on the
+      // side of failing rather than running incorrectly.
+
+      HasMemArgs = true;
+      InVals.push_back(DAG.getUNDEF(VA.getValVT()));
     }
   }
-
-  if (IsVarArg || MF.getFunction().hasStructRetAttr()) {
-    fail(DL, DAG, "functions with VarArgs or StructRet are not supported");
+  if (HasMemArgs) {
+    unsupported(DL, DAG, "stack arguments are not supported");
+  }
+  if (IsVarArg) {
+    unsupported(DL, DAG, "variadic functions are not supported");
+  }
+  if (MF.getFunction().hasStructRetAttr()) {
+    unsupported(DL, DAG, "sret functions are not supported");
   }
 
   return Chain;
 }
 
-const unsigned BPFTargetLowering::MaxArgs = 5;
+const size_t BPFTargetLowering::MaxArgs = 5;
 
 SDValue BPFTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
                                      SmallVectorImpl<SDValue> &InVals) const {
   SelectionDAG &DAG = CLI.DAG;
   auto &Outs = CLI.Outs;
   auto &OutVals = CLI.OutVals;
   auto &Ins = CLI.Ins;
   SDValue Chain = CLI.Chain;
   SDValue Callee = CLI.Callee;
   bool &IsTailCall = CLI.IsTailCall;
   CallingConv::ID CallConv = CLI.CallConv;
   bool IsVarArg = CLI.IsVarArg;
   MachineFunction &MF = DAG.getMachineFunction();
 
   // BPF target does not support tail call optimization.
   IsTailCall = false;
 
   switch (CallConv) {
-  default:
-    report_fatal_error("Unsupported calling convention");
+  default: {
+    report_fatal_error("unsupported calling convention: " + Twine(CallConv));
+  }
   case CallingConv::Fast:
   case CallingConv::C:
     break;
   }
 
   // Analyze operands of the call, assigning locations to each operand.
   SmallVector<CCValAssign, 16> ArgLocs;
   CCState CCInfo(CallConv, IsVarArg, MF, ArgLocs, *DAG.getContext());
 
   CCInfo.AnalyzeCallOperands(Outs, getHasAlu32() ? CC_BPF32 : CC_BPF64);
 
   unsigned NumBytes = CCInfo.getStackSize();
 
-  if (Outs.size() > MaxArgs)
-    fail(CLI.DL, DAG, "too many args to ", Callee);
+  if (Outs.size() > MaxArgs) {
+    unsupported(CLI.DL, DAG, "too many arguments", Callee);
+  }
 
+  bool HasByVal = false;
   for (auto &Arg : Outs) {
     ISD::ArgFlagsTy Flags = Arg.Flags;
-    if (!Flags.isByVal())
-      continue;
-
-    fail(CLI.DL, DAG, "pass by value not supported ", Callee);
+    if (Flags.isByVal()) {
+      HasByVal = true;
+      break;
+    }
+  }
+  if (HasByVal) {
+    unsupported(CLI.DL, DAG, "pass by value not supported", Callee);
   }
 
   auto PtrVT = getPointerTy(MF.getDataLayout());
   Chain = DAG.getCALLSEQ_START(Chain, NumBytes, 0, CLI.DL);
 
   SmallVector<std::pair<unsigned, SDValue>, MaxArgs> RegsToPass;
 
   // Walk arg assignments
-  for (unsigned i = 0,
-                e = std::min(static_cast<unsigned>(ArgLocs.size()), MaxArgs);
-       i != e; ++i) {
-    CCValAssign &VA = ArgLocs[i];
-    SDValue Arg = OutVals[i];
+  for (size_t I = 0; I < std::min(ArgLocs.size(), MaxArgs); ++I) {
+    CCValAssign &VA = ArgLocs[I];
+    SDValue &Arg = OutVals[I];
 
     // Promote the value if needed.
     switch (VA.getLocInfo()) {
-    default:
-      llvm_unreachable("Unknown loc info");
+    default: {
+      report_fatal_error("unhandled location info: " + Twine(VA.getLocInfo()));
+    }
     case CCValAssign::Full:
       break;
     case CCValAssign::SExt:
       Arg = DAG.getNode(ISD::SIGN_EXTEND, CLI.DL, VA.getLocVT(), Arg);
       break;
     case CCValAssign::ZExt:
       Arg = DAG.getNode(ISD::ZERO_EXTEND, CLI.DL, VA.getLocVT(), Arg);
       break;
     case CCValAssign::AExt:
       Arg = DAG.getNode(ISD::ANY_EXTEND, CLI.DL, VA.getLocVT(), Arg);
       break;
     }
 
     // Push arguments into RegsToPass vector
-    if (VA.isRegLoc())
+    if (VA.isRegLoc()) {
       RegsToPass.push_back(std::make_pair(VA.getLocReg(), Arg));
-    else
-      llvm_unreachable("call arg pass bug");
+    } else {
+      report_fatal_error("stack arguments are not supported");
+    }
   }
 
   SDValue InGlue;
 
   // Build a sequence of copy-to-reg nodes chained together with token chain and
   // flag operands which copy the outgoing args into registers.  The InGlue in
   // necessary since all emitted instructions must be stuck together.
   for (auto &Reg : RegsToPass) {
     Chain = DAG.getCopyToReg(Chain, CLI.DL, Reg.first, Reg.second, InGlue);
     InGlue = Chain.getValue(1);
   }
 
   // If the callee is a GlobalAddress node (quite common, every direct call is)
   // turn it into a TargetGlobalAddress node so that legalize doesn't hack it.
   // Likewise ExternalSymbol -> TargetExternalSymbol.
   if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee)) {
     Callee = DAG.getTargetGlobalAddress(G->getGlobal(), CLI.DL, PtrVT,
                                         G->getOffset(), 0);
   } else if (ExternalSymbolSDNode *E = dyn_cast<ExternalSymbolSDNode>(Callee)) {
     Callee = DAG.getTargetExternalSymbol(E->getSymbol(), PtrVT, 0);
-    fail(CLI.DL, DAG, Twine("A call to built-in function '"
-                            + StringRef(E->getSymbol())
-                            + "' is not supported."));
   }
 
   // Returns a chain & a flag for retval copy to use.
   SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
   SmallVector<SDValue, 8> Ops;
   Ops.push_back(Chain);
   Ops.push_back(Callee);
 
   // Add argument registers to the end of the list so that they are
   // known live into the call.
   for (auto &Reg : RegsToPass)
     Ops.push_back(DAG.getRegister(Reg.first, Reg.second.getValueType()));
 
-  if (InGlue.getNode())
+  if (InGlue.getNode()) {
     Ops.push_back(InGlue);
+  }
 
   Chain = DAG.getNode(BPFISD::CALL, CLI.DL, NodeTys, Ops);
   InGlue = Chain.getValue(1);
 
   DAG.addNoMergeSiteInfo(Chain.getNode(), CLI.NoMerge);
 
   // Create the CALLSEQ_END node.
   Chain = DAG.getCALLSEQ_END(Chain, NumBytes, 0, InGlue, CLI.DL);
BPFTargetLowering::LowerReturn(SDValue Chain, CallingConv::ID CallConv,
   // CCValAssign - represent the assignment of the return value to a location
   SmallVector<CCValAssign, 16> RVLocs;
   MachineFunction &MF = DAG.getMachineFunction();
 
   // CCState - Info about the registers and stack slot.
   CCState CCInfo(CallConv, IsVarArg, MF, RVLocs, *DAG.getContext());
 
   if (MF.getFunction().getReturnType()->isAggregateType()) {
-    fail(DL, DAG, "only integer returns supported");
+    unsupported(DL, DAG, "aggregate returns are not supported");
     return DAG.getNode(Opc, DL, MVT::Other, Chain);
   }
 
   // Analize return values.
   CCInfo.AnalyzeReturn(Outs, getHasAlu32() ? RetCC_BPF32 : RetCC_BPF64);
 
   SDValue Glue;
   SmallVector<SDValue, 4> RetOps(1, Chain);
 
   // Copy the result values into the output registers.
-  for (unsigned i = 0; i != RVLocs.size(); ++i) {
-    CCValAssign &VA = RVLocs[i];
-    assert(VA.isRegLoc() && "Can only return in registers!");
+  for (size_t I = 0; I != RVLocs.size(); ++I) {
+    CCValAssign &VA = RVLocs[I];
+    if (!VA.isRegLoc()) {
+      unsupported(DL, DAG, "stack returns are not supported");
+      return DAG.getNode(Opc, DL, MVT::Other, Chain);
+    }
 
-    Chain = DAG.getCopyToReg(Chain, DL, VA.getLocReg(), OutVals[i], Glue);
+    Chain = DAG.getCopyToReg(Chain, DL, VA.getLocReg(), OutVals[I], Glue);
 
     // Guarantee that all emitted copies are stuck together,
     // avoiding something bad.
     Glue = Chain.getValue(1);
     RetOps.push_back(DAG.getRegister(VA.getLocReg(), VA.getLocVT()));
   }
 
   RetOps[0] = Chain; // Update chain.
 
   // Add the glue if we have it.
-  if (Glue.getNode())
+  if (Glue.getNode()) {
     RetOps.push_back(Glue);
+  }
 
   return DAG.getNode(Opc, DL, MVT::Other, RetOps);
 }
 
 SDValue BPFTargetLowering::LowerCallResult(
     SDValue Chain, SDValue InGlue, CallingConv::ID CallConv, bool IsVarArg,
     const SmallVectorImpl<ISD::InputArg> &Ins, const SDLoc &DL,
     SelectionDAG &DAG, SmallVectorImpl<SDValue> &InVals) const {
 
   MachineFunction &MF = DAG.getMachineFunction();
   // Assign locations to each value returned by this call.
   SmallVector<CCValAssign, 16> RVLocs;
   CCState CCInfo(CallConv, IsVarArg, MF, RVLocs, *DAG.getContext());
 
-  if (Ins.size() >= 2) {
-    fail(DL, DAG, "only small returns supported");
-    for (unsigned i = 0, e = Ins.size(); i != e; ++i)
-      InVals.push_back(DAG.getConstant(0, DL, Ins[i].VT));
+  if (Ins.size() > 1) {
+    // NB: An earlier version of this code inserted constant zeros. See the
+    // comment in LowerFormalArguments for details on why that's not a viable
+    // strategy.
+    unsupported(DL, DAG, "only small returns supported");
+    for (auto &In : Ins) {
+      InVals.push_back(DAG.getUNDEF(In.VT));
+    }
     return DAG.getCopyFromReg(Chain, DL, 1, Ins[0].VT, InGlue).getValue(1);
   }
 
   CCInfo.AnalyzeCallResult(Ins, getHasAlu32() ? RetCC_BPF32 : RetCC_BPF64);
 
   // Copy all of the result registers out of their specified physreg.
   for (auto &Val : RVLocs) {
     Chain = DAG.getCopyFromReg(Chain, DL, Val.getLocReg(),
                                Val.getValVT(), InGlue).getValue(1);
     InGlue = Chain.getValue(2);
     InVals.push_back(Chain.getValue(0));
   }
 
   return Chain;
 }
 
-static void NegateCC(SDValue &LHS, SDValue &RHS, ISD::CondCode &CC) {
+static void negateCondCode(SDValue &LHS, SDValue &RHS, ISD::CondCode &CC) {
   switch (CC) {
   default:
     break;
   case ISD::SETULT:
   case ISD::SETULE:
   case ISD::SETLT:
   case ISD::SETLE:
     CC = ISD::getSetCCSwappedOperands(CC);
     std::swap(LHS, RHS);
     break;
   }
 }
 
 SDValue BPFTargetLowering::LowerBR_CC(SDValue Op, SelectionDAG &DAG) const {
   SDValue Chain = Op.getOperand(0);
   ISD::CondCode CC = cast<CondCodeSDNode>(Op.getOperand(1))->get();
   SDValue LHS = Op.getOperand(2);
   SDValue RHS = Op.getOperand(3);
   SDValue Dest = Op.getOperand(4);
   SDLoc DL(Op);
 
-  if (!getHasJmpExt())
-    NegateCC(LHS, RHS, CC);
+  if (!getHasJmpExt()) {
+    negateCondCode(LHS, RHS, CC);
+  }
 
   return DAG.getNode(BPFISD::BR_CC, DL, Op.getValueType(), Chain, LHS, RHS,
                      DAG.getConstant(CC, DL, LHS.getValueType()), Dest);
 }
 
 SDValue BPFTargetLowering::LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const {
   SDValue LHS = Op.getOperand(0);
   SDValue RHS = Op.getOperand(1);
   SDValue TrueV = Op.getOperand(2);
   SDValue FalseV = Op.getOperand(3);
   ISD::CondCode CC = cast<CondCodeSDNode>(Op.getOperand(4))->get();
   SDLoc DL(Op);
 
-  if (!getHasJmpExt())
-    NegateCC(LHS, RHS, CC);
+  if (!getHasJmpExt()) {
+    negateCondCode(LHS, RHS, CC);
+  }
 
   SDValue TargetCC = DAG.getConstant(CC, DL, LHS.getValueType());
   SDVTList VTs = DAG.getVTList(Op.getValueType(), MVT::Glue);
   SDValue Ops[] = {LHS, RHS, TargetCC, TrueV, FalseV};
 
   return DAG.getNode(BPFISD::SELECT_CC, DL, VTs, Ops);
 }
 
const char *BPFTargetLowering::getTargetNodeName(unsigned Opcode) const {
   case BPFISD::MEMCPY:
     return "BPFISD::MEMCPY";
   }
   return nullptr;
 }
 
 SDValue BPFTargetLowering::LowerGlobalAddress(SDValue Op,
                                               SelectionDAG &DAG) const {
-  auto N = cast<GlobalAddressSDNode>(Op);
-  assert(N->getOffset() == 0 && "Invalid offset for global address");
+  auto *N = cast<GlobalAddressSDNode>(Op);
+  if (N->getOffset()) {
+    report_fatal_error("invalid offset for global address: " +
+                       Twine(N->getOffset()));
+  }
 
   SDLoc DL(Op);
   const GlobalValue *GV = N->getGlobal();
   SDValue GA = DAG.getTargetGlobalAddress(GV, DL, MVT::i64);
 
   return DAG.getNode(BPFISD::Wrapper, DL, MVT::i64, GA);
 }
 
-unsigned
-BPFTargetLowering::EmitSubregExt(MachineInstr &MI, MachineBasicBlock *BB,
-                                 unsigned Reg, bool isSigned) const {
+unsigned BPFTargetLowering::EmitSubregExt(MachineInstr &MI,
+                                          MachineBasicBlock *BB, unsigned Reg,
+                                          bool IsSigned) const {
   const TargetInstrInfo &TII = *BB->getParent()->getSubtarget().getInstrInfo();
   const TargetRegisterClass *RC = getRegClassFor(MVT::i64);
-  int RShiftOp = isSigned ? BPF::SRA_ri : BPF::SRL_ri;
+  int RShiftOp = IsSigned ? BPF::SRA_ri : BPF::SRL_ri;
   MachineFunction *F = BB->getParent();
   DebugLoc DL = MI.getDebugLoc();
 
   MachineRegisterInfo &RegInfo = F->getRegInfo();
 
-  if (!isSigned) {
+  if (!IsSigned) {
     Register PromotedReg0 = RegInfo.createVirtualRegister(RC);
     BuildMI(BB, DL, TII.get(BPF::MOV_32_64), PromotedReg0).addReg(Reg);
     return PromotedReg0;
   }
   Register PromotedReg0 = RegInfo.createVirtualRegister(RC);
   Register PromotedReg1 = RegInfo.createVirtualRegister(RC);
   Register PromotedReg2 = RegInfo.createVirtualRegister(RC);
   BuildMI(BB, DL, TII.get(BPF::MOV_32_64), PromotedReg0).addReg(Reg);
BPFTargetLowering::EmitInstrWithCustomInserterMemcpy(MachineInstr &MI,
   MIB.addReg(ScratchReg,
              RegState::Define | RegState::Dead | RegState::EarlyClobber);
 
   return BB;
 }
 
 MachineBasicBlock *
 BPFTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
-                                               MachineBasicBlock *BB) const {
-  const TargetInstrInfo &TII = *BB->getParent()->getSubtarget().getInstrInfo();
+                                               MachineBasicBlock *MBB) const {
+  const TargetInstrInfo &TII = *MBB->getParent()->getSubtarget().getInstrInfo();
   DebugLoc DL = MI.getDebugLoc();
   unsigned Opc = MI.getOpcode();
   bool isSelectRROp = (Opc == BPF::Select ||
                        Opc == BPF::Select_64_32 ||
                        Opc == BPF::Select_32 ||
                        Opc == BPF::Select_32_64);
 
-  bool isMemcpyOp = Opc == BPF::MEMCPY;
+  bool IsMemcpyOp = Opc == BPF::MEMCPY;
 
 #ifndef NDEBUG
-  bool isSelectRIOp = (Opc == BPF::Select_Ri ||
-                       Opc == BPF::Select_Ri_64_32 ||
-                       Opc == BPF::Select_Ri_32 ||
-                       Opc == BPF::Select_Ri_32_64);
+  bool IsSelectRiOp = (Opc == BPF::Select_Ri || Opc == BPF::Select_Ri_64_32 ||
+                       Opc == BPF::Select_Ri_32 || Opc == BPF::Select_Ri_32_64);
 
-
-  assert((isSelectRROp || isSelectRIOp || isMemcpyOp) &&
-         "Unexpected instr type to insert");
+  if (!(isSelectRROp || IsSelectRiOp || IsMemcpyOp)) {
+    report_fatal_error("unhandled instruction type: " + Twine(Opc));
+  }
 #endif
 
-  if (isMemcpyOp)
-    return EmitInstrWithCustomInserterMemcpy(MI, BB);
+  if (IsMemcpyOp) {
+    return EmitInstrWithCustomInserterMemcpy(MI, MBB);
+  }
 
   bool is32BitCmp = (Opc == BPF::Select_32 ||
                      Opc == BPF::Select_32_64 ||
                      Opc == BPF::Select_Ri_32 ||
                      Opc == BPF::Select_Ri_32_64);
 
   // To "insert" a SELECT instruction, we actually have to insert the diamond
   // control-flow pattern.  The incoming instruction knows the destination vreg
   // to set, the condition code register to branch on, the true/false values to
   // select between, and a branch opcode to use.
-  const BasicBlock *LLVM_BB = BB->getBasicBlock();
-  MachineFunction::iterator I = ++BB->getIterator();
+  const BasicBlock *BB = MBB->getBasicBlock();
+  MachineFunction::iterator I = ++MBB->getIterator();
 
   // ThisMBB:
   // ...
   //  TrueVal = ...
   //  jmp_XX r1, r2 goto Copy1MBB
   //  fallthrough --> Copy0MBB
-  MachineBasicBlock *ThisMBB = BB;
-  MachineFunction *F = BB->getParent();
-  MachineBasicBlock *Copy0MBB = F->CreateMachineBasicBlock(LLVM_BB);
-  MachineBasicBlock *Copy1MBB = F->CreateMachineBasicBlock(LLVM_BB);
+  MachineBasicBlock *ThisMBB = MBB;
+  MachineFunction *F = MBB->getParent();
+  MachineBasicBlock *Copy0MBB = F->CreateMachineBasicBlock(BB);
+  MachineBasicBlock *Copy1MBB = F->CreateMachineBasicBlock(BB);
 
   F->insert(I, Copy0MBB);
   F->insert(I, Copy1MBB);
   // Update machine-CFG edges by transferring all successors of the current
   // block to the new block which will contain the Phi node for the select.
-  Copy1MBB->splice(Copy1MBB->begin(), BB,
-                   std::next(MachineBasicBlock::iterator(MI)), BB->end());
-  Copy1MBB->transferSuccessorsAndUpdatePHIs(BB);
+  Copy1MBB->splice(Copy1MBB->begin(), MBB,
+                   std::next(MachineBasicBlock::iterator(MI)), MBB->end());
+  Copy1MBB->transferSuccessorsAndUpdatePHIs(MBB);
   // Next, add the true and fallthrough blocks as its successors.
-  BB->addSuccessor(Copy0MBB);
-  BB->addSuccessor(Copy1MBB);
+  MBB->addSuccessor(Copy0MBB);
+  MBB->addSuccessor(Copy1MBB);
 
   // Insert Branch if Flag
   int CC = MI.getOperand(3).getImm();
   int NewCC;
   switch (CC) {
 #define SET_NEWCC(X, Y) \
   case ISD::X: \
     if (is32BitCmp && HasJmp32) \
#define SET_NEWCC(X, Y) \
   SET_NEWCC(SETULT, JULT);
   SET_NEWCC(SETLE, JSLE);
   SET_NEWCC(SETULE, JULE);
   default:
     report_fatal_error("unimplemented select CondCode " + Twine(CC));
   }
 
   Register LHS = MI.getOperand(1).getReg();
-  bool isSignedCmp = (CC == ISD::SETGT ||
-                      CC == ISD::SETGE ||
-                      CC == ISD::SETLT ||
-                      CC == ISD::SETLE);
+  bool IsSignedCmp = (CC == ISD::SETGT || CC == ISD::SETGE ||
+                      CC == ISD::SETLT || CC == ISD::SETLE);
 
   // eBPF at the moment only has 64-bit comparison. Any 32-bit comparison need
   // to be promoted, however if the 32-bit comparison operands are destination
   // registers then they are implicitly zero-extended already, there is no
   // need of explicit zero-extend sequence for them.
   //
   // We simply do extension for all situations in this method, but we will
   // try to remove those unnecessary in BPFMIPeephole pass.
-  if (is32BitCmp && !HasJmp32)
-    LHS = EmitSubregExt(MI, BB, LHS, isSignedCmp);
+  if (is32BitCmp && !HasJmp32) {
+    LHS = EmitSubregExt(MI, MBB, LHS, IsSignedCmp);
+  }
 
   if (isSelectRROp) {
     Register RHS = MI.getOperand(2).getReg();
 
-    if (is32BitCmp && !HasJmp32)
-      RHS = EmitSubregExt(MI, BB, RHS, isSignedCmp);
+    if (is32BitCmp && !HasJmp32) {
+      RHS = EmitSubregExt(MI, MBB, RHS, IsSignedCmp);
+    }
 
-    BuildMI(BB, DL, TII.get(NewCC)).addReg(LHS).addReg(RHS).addMBB(Copy1MBB);
+    BuildMI(MBB, DL, TII.get(NewCC)).addReg(LHS).addReg(RHS).addMBB(Copy1MBB);
   } else {
-    int64_t imm32 = MI.getOperand(2).getImm();
+    int64_t Imm32 = MI.getOperand(2).getImm();
     // Check before we build J*_ri instruction.
-    assert (isInt<32>(imm32));
-    BuildMI(BB, DL, TII.get(NewCC))
-        .addReg(LHS).addImm(imm32).addMBB(Copy1MBB);
+    if (!isInt<32>(Imm32)) {
+      report_fatal_error("immediate overflows 32 bits: " + Twine(Imm32));
+    }
+    BuildMI(MBB, DL, TII.get(NewCC)).addReg(LHS).addImm(Imm32).addMBB(Copy1MBB);
   }
 
   // Copy0MBB:
   //  %FalseValue = ...
   //  # fallthrough to Copy1MBB
-  BB = Copy0MBB;
+  MBB = Copy0MBB;
 
   // Update machine-CFG edges
-  BB->addSuccessor(Copy1MBB);
+  MBB->addSuccessor(Copy1MBB);
 
   // Copy1MBB:
   //  %Result = phi [ %FalseValue, Copy0MBB ], [ %TrueValue, ThisMBB ]
   // ...
-  BB = Copy1MBB;
-  BuildMI(*BB, BB->begin(), DL, TII.get(BPF::PHI), MI.getOperand(0).getReg())
+  MBB = Copy1MBB;
+  BuildMI(*MBB, MBB->begin(), DL, TII.get(BPF::PHI), MI.getOperand(0).getReg())
       .addReg(MI.getOperand(5).getReg())
       .addMBB(Copy0MBB)
       .addReg(MI.getOperand(4).getReg())
       .addMBB(ThisMBB);
 
   MI.eraseFromParent(); // The pseudo instruction is gone now.
-  return BB;
+  return MBB;
 }
 
 EVT BPFTargetLowering::getSetCCResultType(const DataLayout &, LLVMContext &,
                                           EVT VT) const {
   return getHasAlu32() ? MVT::i32 : MVT::i64;
 }
 
 MVT BPFTargetLowering::getScalarShiftAmountTy(const DataLayout &DL,
                                               EVT VT) const {
   return (getHasAlu32() && VT == MVT::i32) ? MVT::i32 : MVT::i64;
 }
 
 bool BPFTargetLowering::isLegalAddressingMode(const DataLayout &DL,
                                               const AddrMode &AM, Type *Ty,
                                               unsigned AS,
                                               Instruction *I) const {
   // No global is ever allowed as a base.
-  if (AM.BaseGV)
+  if (AM.BaseGV) {
     return false;
+  }
 
   switch (AM.Scale) {
   case 0: // "r+i" or just "i", depending on HasBaseReg.
     break;
   case 1:
     if (!AM.HasBaseReg) // allow "r+i".
       break;
     return false; // disallow "r+r" or "r+r+i".
   default:
     return false;
   }
 
   return true;
 }