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

Wed Aug 2 20:26:47 PDT 2023

ast added a comment.

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

> 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.

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