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

[Tamir Duberstein via Phabricator via llvm-commits]

tamird marked an inline comment as done.
tamird added inline comments.


================
Comment at: llvm/test/Bindings/llvm-c/struct_ret2.ll:7
+; Before this test was introduced, the BPF backend would generate incorrect-but-viable code
+; that would pass the verifier and violate user expectations at runtime.
+;
----------------
eddyz87 wrote:
> tamird wrote:
> > eddyz87 wrote:
> > > yonghong-song wrote:
> > > > The same here. I guess I must miss something. Do you mean bpf backend generates incorrect code, or it detects incorrect condition but proceed and eventually report a success rather than a failure?
> > > > I must miss something here. 
> > > Hi @yonghong-song ,
> > > 
> > > BPF backend detects and reports unsupported patterns, the report is done using function `diagnose()`. For diagnostic events with error severity `diagnose()` calls `exit(1)` if no other diagnostic handlers are installed ([[ https://reviews.llvm.org/D156497#4557395 | see comment ]]). However, the backend is also designed to proceed after issuing error diagnostics by injecting incorrect instructions. @tamird suggests to change some of these instructions to make it more obvious for BPF verifier that bytecode is bogus. The argument is that this would help during development of standalone tools that embed LLVM and for some reason install error handlers that don't react properly to error level diagnostics. I wonder what is the default error handler for embedders, let me check.
> > @eddyz87 is mostly correct. 
> > 
> >  also wondered what the default handler would do; before I added the current handler in my test, I observed that the handler would `exit(1)`.
> > 
> > However note the following:
> > - clang obviously does something more sophisticated; we know this because when multiple diagnostics are issued, we seem them all. This means the diagnostic handler is stateful.
> > - it's very common to provide a custom handler to allow filtering on severity, logging to a file, etc.
> > - it is not at all obvious that DS_Error is to be considered fatal. When I first discovered the bug in bpf-linker I searched for documentation about these severity levels, and found basically nothing explaining this. Even @eddyz87's earlier comments refer to clang source code rather than documentation.
> > 
> > Fundamentally this is a documentation problem, but it would certainly not hurt to be a bit more defensive in the BPF backend IMO.
> > 
> > [DiagnosticSeverity](https://github.com/llvm/llvm-project/blob/39cf2104507a4b6734bddd2b894855a624b2bf68/llvm/include/llvm/IR/DiagnosticInfo.h#L49)
> > 
> > [LLVMContextSetDiagnosticHandler](https://github.com/llvm/llvm-project/blob/39cf2104507a4b6734bddd2b894855a624b2bf68/llvm/include/llvm-c/Core.h#L542)
> Imho, given the context, changing "get fake zero" to "get fake undef" in a few places is not a big deal. I'm also not sure that we need complicated tests infrastructure for this. ¯\_(ツ)_/¯.
>>! In D156497#4541015, @ast wrote:
> Please provide a test case that demonstrates the issue.
> Right now it looks to me that aya front-end generates broken IR and it causes all kinds of issues in the backend.
> We won't be doing defensive programming to help front-ends debug their issues.

>>! In D156497#4556197, @ast wrote:
> 
> Without specific test this all are hand wave excuses. Fix you front-end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156497