bpf: Report an error properly for unsupported opcodes in lowering operations

[agentzh]

Summary

Without this patch, it leads to process crashes when unsupported gcc builtins are used, for example.

[yonghong-song]

Without this patch, it leads to process crashes when unsupported gcc builtins are used, for example.

This is not correct description. The "llvm_unreachable" is intentional to catch all unsupported isd opcodes. If we find one unsupported isd opcode, we can add one case similar to ISD::DYNAMIC_STACKALLOC.
Please do have enough details in the commit messages so people can reproduce the issue if possible.

llvm/lib/Target/BPF/BPFISelLowering.cpp

293

This is not correct. Did you actually test it? What is the actual printout?
The report_fatal_error() signature is report_fatal_error(llvm::StringRef/char[]/..., bool) and the second parameter is certainly not correct.

[dsanders]

In D116854#3229369, @yonghong-song wrote:

Without this patch, it leads to process crashes when unsupported gcc builtins are used, for example.

This is not correct description. The "llvm_unreachable" is intentional to catch all unsupported isd opcodes. If we find one unsupported isd opcode, we can add one case similar to ISD::DYNAMIC_STACKALLOC.
Please do have enough details in the commit messages so people can reproduce the issue if possible.

llvm_unreachable() only catches and errors on the default case in builds that have assertions enabled. When assertions are disabled, the default case won't exist (or on some compilers will exist but will do nothing) and in this case it could leave the function without executing a return statement which could potentially go on to crash as the caller will be expecting a valid object. I don't think the behaviour is defined when assertions are off (so it could take an unrelated path) but in my experience at least using it on default cases tends to act like there's no default case.

That said, most targets do the same as BPF for this situation. I see that a few targets (e.g. AArch64) have a return SDValue() which could ensure LowerOperations at least returns a defined value for this situation. I can't tell if adding that will fix the original problem but it'd be worth trying.

[yonghong-song]

@dsanders Thanks for pointing out llvm_unreachable is only available if assertions are enabled. I am not aware of this since I always have assertions on with my build. So I think the correct fix should be chaning llvm_unreachable() to report_fatal_error(). The current patch almost works except some minor issues I pointed in the above.

llvm/lib/Target/BPF/BPFISelLowering.cpp

 }
 
 SDValue BPFTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
-  switch (Op.getOpcode()) {
+  uint32_t Opcode = Op.getOpcode();
+  switch (Opcode) {
   case ISD::BR_CC:
     return LowerBR_CC(Op, DAG);
   case ISD::GlobalAddress:

   case ISD::DYNAMIC_STACKALLOC:
     report_fatal_error("Unsupported dynamic stack allocation");
   default:
-    llvm_unreachable("unimplemented operand");
+    report_fatal_error("Unimplemented lowering operation opcode %u", Opcode);
   }
 }