Re: [Tarantool-patches] [PATCH luajit 01/19] MIPS: Use precise search for exit jump patching.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the review!

On 15.08.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> LGTM, except for a few comments below.
> 
> On Wed, Aug 09, 2023 at 06:35:50PM +0300, Sergey Kaplun via Tarantool-patches wrote:
> > From: Mike Pall <mike>
> > 
> > Contributed by Djordje Kovacevic and Stefan Pejic.
> > 
> > (cherry-picked from commit 7381b620358c2561e8690149f1d25828fdad6675)
> > 
> > Without the aforementioned checks, some non-branch instructions may be
> > interpreted as some branch due to memory address collisions. This patch
> Please add a more comprehensive description of behavior before the patch.
> Because of magic values it is not obvious that the difference between the
> current PC and the jump address is XORed with the opcode, to make sure
> that this is a branching instruction.

Added. The new commit message is the following:

| MIPS: Use precise search for exit jump patching.
|
| Contributed by Djordje Kovacevic and Stefan Pejic.
|
| (cherry-picked from commit 7381b620358c2561e8690149f1d25828fdad6675)
|
| The branch instruction contains PC-relative mcode address in the lowest
| 4 bytes. To ensure that it is branch instruction we check that
| difference of the address of the current instruction and jump target is
| contained in the lowest 4 bytes of the instruction. But there is no
| check that opcode of this instruction is branch opcode. Without the
| aforementioned checks, some non-branch instructions may be interpreted
| as branches due to memory address collisions. This patch adds the
| corresponding mask values for comparisons with instruction opcodes used
| in the LuaJIT:
| * `MIPSI_BEQ` for `beq` and `bne`,
| * `MIPSI_BLTZ` for `bltz`, `blez`, `bgtz` and `bgez`,
| * `MIPSI_BC1F` for `bc1f` and `bc1t`,
| see <src/lj_target_mips.h> and MIPS Instruction Set Manual [1] for
| details.
|
| To reproduce this failure, we need specific memory mapping, so the test
| case is omitted.
|
| Since MIPS architecture is not supported by Tarantool (at the moment)
| this patch is not necessary for backport. OTOH, it gives us the
| following benefits:
| * Be in sync with the LuaJIT upstream not only for x86_64, arm64
|   architectures.
| * Avoid conflicts during future backporting.
| So, it's more useful to backport some of the patches to avoid conflicts
| with the future patch series.
|
| [1]: https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00086-2B-MIPS32BIS-AFP-6.06.pdf
|
| Sergey Kaplun:
| * added the description for the problem
|
| Part of tarantool/tarantool#8825

> 
> Typo: s/some branch/branches/

Fixed.

> > adds the corresponding comparisons masked values with instruction
> Typo: s/comparisons masked values/mask values for comparisons/

Fixed.

> > opcodes used in the LuaJIT:
> > * `MIPSI_BEQ` for `beq` and `bne`,
> > * `MIPSI_BLTZ` for `bltz`, `blez`, `bgtz` and `bgez`,
> > * `MIPSI_BC1F` for `bc1f` and `bc1t`,
> > see <src/lj_target_mips.h> and MIPS Instruction Set Manual [1] for
> > details.
> > 
> > To reproduce this failure, we need specific memory mapping, so testcase
> Typo: s/testcase/the test case/

Fixed.

> > is omitted.
> > 
> > Since MIPS architecture is not supported by Tarantool (at the moment)
> > this patch is not necessary for backport. OTOH, it gives to us the
> Typo: s/gives to us/gives us/

Fixed.

> > following benefits:
> > * Be in sync with the LuaJIT upstream not only for x86_64, arm64
> >   architectures.
> > * Avoid conflicts during the future backporting.
> Typo: s/during the future/during future/

Fixed.

> > So, it's more useful to backport some of the patches to avoid conflicts

<snipped>

> > 
> Best regards,
> Maxim Kokryashkin

-- 
Best regards,
Sergey Kaplun