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

Mon Jul 24 14:45:40 PDT 2023

tamird added a comment.

In D155894#4529382 <https://reviews.llvm.org/D155894#4529382>, @eddyz87 wrote:

> In D155894#4528546 <https://reviews.llvm.org/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 <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?
>
> 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 <https://reviews.llvm.org/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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155894