[PATCH] D116854: bpf: Report an error properly for unsupported opcodes in lowering operations

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In D116854#3229369 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116854/new/

https://reviews.llvm.org/D116854