[PATCH] D156497: [BPF] report fatal error rather than miscompile

[Tamir Duberstein via Phabricator via llvm-commits]

tamird added a comment.

In D156497#4556071 <https://reviews.llvm.org/D156497#4556071>, @ast wrote:

> In D156497#4556048 <https://reviews.llvm.org/D156497#4556048>, @tamird wrote:
>
>> In D156497#4555836 <https://reviews.llvm.org/D156497#4555836>, @ast wrote:
>>
>>> Agree with Eduard. Doing report_fatal_error() in fail() is cleaner. Proceeding with miscompiled code can only cause further issues in other parts of the backend.
>>> Not sure why we didn't do this earlier.
>>
>> Counterpoint: D98471 <https://reviews.llvm.org/D98471>. @yonghong-song can you weigh in please?
>>
>> It's not apparent that there's an obviously right thing to do here. The standard error emitting mechanisms seem to always involve a risk of miscompilation. Does anyone know how other backends deal with this situation, where code generation cannot reasonably proceed? Looks like other backends use `UNDEF` in at least some similar cases (https://github.com/llvm/llvm-project/blob/cd328c10ad503b243cc333f813c0c6ee66598ffc/llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp#L1265-L1269). From my perspective, as long as we miscompile more severely - that is we produce code that definitely doesn't work, rather than code that might work but do the wrong thing - then I'm happy. @ast I believe you suggested constant zeroes rather than UNDEF in earlier reviews -- can you shed some light on why that was deemed preferable?
>
> actually. let's step back.
> The compilation doesn't report success at the end, so what is the problem?
> diagnose() and proceed is a normal pattern used in many backends.

This is half-correct. As I write in the description: whether to use the result of compilation is decided by the embedding application. In earlier versions of bpf-linker, these diagnostics were not considered fatal, which resulted in miscompiled code (which uses constant 0 when nothing sensible can be done). This in turn produced very hard to debug failures.

It seems obvious that doing anything other than emitting diagnostics is going against the grain in LLVM. This means we have to allow compilation superficially "succeed". Having said that, I think we should produce **obviously broken** code rather than producing code that //might// work if the stars align, but might produce subtly incorrect behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156497