[Tarantool-patches] [PATCH luajit v2 3/7] vm: introduce VM states for Lua and fast functions

Mon Dec 28 02:48:26 MSK 2020

Sergey,

Thanks for the patch! Please consider the comments below.

On 25.12.20, Sergey Kaplun wrote:
> This patch introduces LJ_VMST_LFUNC and LJ_VMST_FFUNC VM states
> separated from LJ_VMST_INERP. New VM states allow to determine the
> context of Lua VM execution for x86 and x64 arches. Also, LJ_VMST_C is
> renamed to LJ_VMST_CFUNC for naming consistence with new VM states.
> 
> Also, this patch adjusts stack layout for x86 and x64 arches to save VM
> state for its consistency while stack unwinding when error is raised.
> 
> Part of tarantool/tarantool#5442
> ---
> 
> Changes in v2:
>  - Moved `.if not WIN` macro check inside (save|restore)_vmstate_through
>  - Fixed naming: SAVE_UNUSED\d -> UNUSED\d
> 
>  src/lj_frame.h     |  18 +++----
>  src/lj_obj.h       |   4 +-
>  src/lj_profile.c   |   5 +-
>  src/luajit-gdb.py  |  14 ++---
>  src/vm_arm.dasc    |   6 +--
>  src/vm_arm64.dasc  |   6 +--
>  src/vm_mips.dasc   |   6 +--
>  src/vm_mips64.dasc |   6 +--
>  src/vm_ppc.dasc    |   6 +--
>  src/vm_x64.dasc    |  93 ++++++++++++++++++++++----------
>  src/vm_x86.dasc    | 131 +++++++++++++++++++++++++++++----------------
>  11 files changed, 188 insertions(+), 107 deletions(-)
> 
> diff --git a/src/lj_frame.h b/src/lj_frame.h
> index 19c49a4..2e693f9 100644
> --- a/src/lj_frame.h
> +++ b/src/lj_frame.h

<snipped>

> @@ -152,11 +152,11 @@ enum { LJ_CONT_TAILCALL, LJ_CONT_FFI_CALLBACK };  /* Special continuations. */
>  #define CFRAME_OFS_NRES		(22*4)
>  #define CFRAME_OFS_MULTRES	(21*4)
>  #endif
> -#define CFRAME_SIZE		(10*8)
> +#define CFRAME_SIZE		(12*8)

This change looks Windows-related, so is excess.

>  #define CFRAME_SIZE_JIT		(CFRAME_SIZE + 9*16 + 4*8)

<snipped>

> diff --git a/src/lj_profile.c b/src/lj_profile.c
> index 116998e..637e03c 100644
> --- a/src/lj_profile.c
> +++ b/src/lj_profile.c
> @@ -157,7 +157,10 @@ static void profile_trigger(ProfileState *ps)
>      int st = g->vmstate;
>      ps->vmstate = st >= 0 ? 'N' :
>  		  st == ~LJ_VMST_INTERP ? 'I' :
> -		  st == ~LJ_VMST_C ? 'C' :
> +		  st == ~LJ_VMST_CFUNC ? 'C' :
> +		  /* Stubs for profiler hooks. */
> +		  st == ~LJ_VMST_FFUNC ? 'I' :
> +		  st == ~LJ_VMST_LFUNC ? 'I' :

Minor: Move this comparisons above to save the ordering from <lj_obj.h>.

>  		  st == ~LJ_VMST_GC ? 'G' : 'J';
>      g->hookmask = (mask | HOOK_PROFILE);
>      lj_dispatch_update(g);

<snipped>

> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
> index 80753e0..83cc3e1 100644
> --- a/src/vm_x64.dasc
> +++ b/src/vm_x64.dasc

<snipped>

> @@ -161,26 +161,29 @@

<snipped>

> +|.define SAVE_CFRAME,	qword [rsp+qword*6]
> +|.define UNUSED2,	qword [rsp+qword*5]
> +|.define UNUSED1,	dword [rsp+dword*8]

UNUSED1 should alias <dword [rsp + dword*9]>

Minor: What about UNUSEDd and UNUSEDq instead? Feel free to ignore.

> +|.define SAVE_VMSTATE,	dword [rsp+dword*8]

<snipped>

> @@ -342,6 +345,22 @@
>  |  mov dword [DISPATCH+DISPATCH_GL(vmstate)], ~LJ_VMST_..st
>  |.endmacro
>  |
> +|// Save vmstate through register.
> +|.macro save_vmstate_through, reg
> +|.if not WIN
> +|  mov reg, dword [DISPATCH+DISPATCH_GL(vmstate)]
> +|  mov SAVE_VMSTATE, reg

This is too complicated. VM is implemented as a file with 6kloc of
pseudo-assembly in it and I don't want to make its support harder. We've
recently found a bug with miscomparison that we coudn't catch for a
year. This is not a fun. This is not a kinda sport. This is damn 6k kloc
of assembly we ought to support. So I propose to make this macro work
with no args at all. Consider the following:
|.macro save_vmstate
|.if not WIN
|  mov TMPRd, dword [DISPATCH+DISPATCH_GL(vmstate)]	// TMPRd is r10d
|  mov SAVE_VMSTATE, TMPRd
|.endif // WIN
|
|.macro restore_vmstate
|.if not WIN
|  mov TMPRd, SAVE_VMSTATE	// XCHGd is r11d
|  mov dword [DISPATCH+DISPATCH_GL(vmstate)], TMPRd
|.endif // WIN

I agree that TMPRd is used in other places within vm_x64.dasc but this
is temporary register and I failed to find any conflict. I left the
comments below to ease the check.

> +|.endif // WIN
> +|.endmacro

<snipped>

> @@ -448,6 +467,8 @@ static void build_subroutines(BuildCtx *ctx)
>    |  xor eax, eax			// Ok return status for vm_pcall.
>    |
>    |->vm_leave_unw:
> +  |  // DISPATCH required to set properly.
> +  |  restore_vmstate_through RAd

Re TMPR: The TMP register is not used, so there is no need to restore
vmstate via RAd. Everything is OK.

>    |  restoreregs
>    |  ret
>    |

<snipped>

> @@ -521,7 +544,7 @@ static void build_subroutines(BuildCtx *ctx)
>    |  mov [BASE-16], RA			// Prepend false to error message.
>    |  mov [BASE-8], RB
>    |  mov RA, -16			// Results start at BASE+RA = BASE-16.
> -  |  set_vmstate INTERP
> +  |  set_vmstate INTERP // INTERP until jump to BC_RET* or return to C