[PATCH] D155894: [BPF] Do not miscompile, allow external calls

[Tamir Duberstein via Phabricator via llvm-commits]

tamird added a comment.

In D155894#4525984 <https://reviews.llvm.org/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 <https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements>)
>   - 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 <https://github.com/torvalds/linux/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 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155894