[PATCH] D156497: [BPF] Emit UNDEF rather than constant 0

Thu Aug 17 21:52:35 PDT 2023

ast added a comment.

In D156497#4597500 <https://reviews.llvm.org/D156497#4597500>, @tamird wrote:

> In D156497#4597470 <https://reviews.llvm.org/D156497#4597470>, @ast wrote:
>
>> Sorry for the delay. I've been on PTO.
>> The change looks fine, but it's not clear to me what code will be generated with UNDEF. 
>> Before the change it would be 'r0=0; exit' and now just 'exit' ?
>> Meaning that r0 will be left in whatever state that previous code did.
>> So instead of generating consistent return 0 the user would have to debug random return values ?
>> The chance of failing the verifier is slim. The functions are rarely completely empty.
>> r0 is likely used in the earlier code and the verifier won't complain that it's not initialized.
>> So the change contradicts the intent to remove the footgun.
>
> When I first wrote this patch, I tested it on real code and did see verifier errors. It's a deep stack to unwind to reconstruct the state of the world back then to be able to quote what those errors were and what the generated code looked like. I can do it, but I'd like us to converge on what the right thing to do here is.
>
> It seems you now agree that there's a footgun here. I'm not married to the use of UNDEF, is there a better solution you can think of that would reliably produce errors? Would reconstructing the generated code and verifier errors from a month ago be useful?

I don't see a footgun tbh. Improper handling of diag is purely an aya issue.
But I agree with intent to generate something non-verifiable in case of error. We need to figure what it would be.
There is no trap instruction that we can generate.
We can generate 'ja -1', but it's worse than current return 0.

> Lastly: @yonghong-song suggested that the tests be removed. In light of your comments, should we restore the tests so that we can make claims about the behavior in the presence of more complicated functions?

I didn't get the point of bpf-unsupported-emit.c. It just demonstrates that it can incorrectly ignore DS_Error. We cannot have such "test" in the repo, since it would encourage incorrect diag processing.
In general I would still like to have a test for every commit.
Right now I don't see how UNDEF helps here. It looks worse form debugging of compiled bpf code pov.
If we can come up with something better than return 0 we certainly need a test for that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156497