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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.

Typo: s/some branch/branches/
> adds the corresponding comparisons masked values with instruction
Typo: s/comparisons masked values/mask values for comparisons/
> 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/
> 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/
> 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/
> 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
> ---
>  src/lj_asm_mips.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lj_asm_mips.h b/src/lj_asm_mips.h
> index 03417013..03215821 100644
> --- a/src/lj_asm_mips.h
> +++ b/src/lj_asm_mips.h
> @@ -2472,7 +2472,11 @@ void lj_asm_patchexit(jit_State *J, GCtrace *T, ExitNo exitno, MCode *target)
>    MCode tjump = MIPSI_J|(((uintptr_t)target>>2)&0x03ffffffu);
>    for (p++; p < pe; p++) {
>      if (*p == exitload) {  /* Look for load of exit number. */
> -      if (((p[-1] ^ (px-p)) & 0xffffu) == 0) {  /* Look for exitstub branch. */
> +      /* Look for exitstub branch. Yes, this covers all used branch variants. */
> +      if (((p[-1] ^ (px-p)) & 0xffffu) == 0 &&
> +	  ((p[-1] & 0xf0000000u) == MIPSI_BEQ ||
> +	   (p[-1] & 0xfc1e0000u) == MIPSI_BLTZ ||
> +	   (p[-1] & 0xffe00000u) == MIPSI_BC1F)) {
>  	ptrdiff_t delta = target - p;
>  	if (((delta + 0x8000) >> 16) == 0) {  /* Patch in-range branch. */
>  	patchbranch:
> -- 
> 2.41.0
> 
Best regards,
Maxim Kokryashkin