[PATCH] D56275: x86 interrupt calling convention: Fix argument offsets

Fri Jan 4 14:19:01 PST 2019

phil-opp added a comment.

In D56275#1345293 <https://reviews.llvm.org/D56275#1345293>, @lebedev.ri wrote:

> In D56275#1345277 <https://reviews.llvm.org/D56275#1345277>, @phil-opp wrote:
>
> > > Test?
> >
> > I tried to add additional checks to the `x86-64-intrcc.ll` test, but the patched version of `llc` generates the exact same assembly as the unpatched version for this test (with and without -O0). I think this is because the test doesn't use the relevant code paths. I'm not sure whether it's practical to construct new tests for every code path and how to construct such tests.
>
>
> Either something is 'broken', and this patch fixes things, or the patch is not needed.
>
> How do you know that this patch helps/does the right thing?
>  There is probably some code that now is not broken?
>  Can you write an automated check for that?
>  If yes, you could try to creduce/bugpoint that code to the minimal snippet that still shows the miscompile(?) fix.

Yes, there is some broken Rust code that multiple people are able to reproduce: https://github.com/phil-opp/blog_os/issues/513. With this patch applied, the issue no longer occurs.

I spent my entire day trying to create a test case from the LLVM IR that the Rust code is translated to, but I could not reproduce the failing behavior in a test. I'm not sure why, maybe because of the different properties of the compilation target (e.g. disabled SSE). I'm sure that it would be relatively simple for someone with more LLVM experience, but I would need to invest much more time to familiarize myself with LLVM first, and I don't have the time to do so currently. If someone wants to take this from me or fix it in a different way (e.g. how @rnk suggested), that's fine with me, I just want this issue to be fixed.

That being said, this patch doesn't break any tests and only affects the x86 interrupt calling convention. Also, it does not introduce any new behavior, but just reverts the behavior change that was done in 4c3428b604324ed1528a78dbc31c8c8805d3c3e6. This behavior change was most likely accidental, because it was not mentioned in the commit and no new tests for the x86 interrupt calling convention were added either. So even though I understand the policy of requiring a test, it makes more sense to me to merge this without a test than to keep the wrong behavior until someone invests the needed time to construct a failing test.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56275