Re: [Tarantool-patches] [PATCH luajit v4 1/4] Handle on-trace OOM errors from helper functions.

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Max,

Thanks for the patch! LGTM with some minor comments.

Minor: All the changes for vm_{mips,mips64,ppc} are quite cryptic for
me, so I'd rather drop them. However, I believe that Sergey has already
verified them, so let's leave them for consistency.

On 15.05.23, Maksim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> (cherry-picked from commit 4bba29e697d00df5f020e76c2003bb9ce51c5d38)
> 
> This patch introduces handling of errors from internal helper
> functions on traces. FFI C++ exception interoperability is
> not yet implemented.
> 
> For each throwing trace, its mcode entry is augmented with a
> DWARF2 frame description entry and a common information entry.
> After that, a dynamic DWARF2 frame info is registered based on
> that entry with `__register_frame()`[1][2], which is just an
> adapter to `__unw_add_dynamic_fde`[3] on OSX. Because the ARM32
> architecture lacks the `__register_frame`, unwinding is not
> supported on it.
> 
> It is important to notice, that both the CIE and FDE are
> used on Linux, and only the FDE is used on OSX. The
> CIE is unnecessary on OSX, which can be clearly seen in
> the sources[3] of Apple's libunwind: there is an attempt
> to parse it, however its data is unused. In the same time,
> the CIE is required on Linux[4] to perfrom dynamic frame
> registration.
> 
> For each throwing function call, a snapshot is allocated.
> When we have a parent trace, our side trace head requires
> an additional snapshot allocation, so the additional
> `asm_snap_prev()` call is added.
> 
> The `lj_err_trace()` is introduced to use instead
> `lj_err_run()` for throwing the error on trace.
> 
> The following fields were added to the ASMState structure:
> * `snapalloc` -- flag showing whether the current snapshot
> needs allocation.
> * `mctoporig` -- holds the pointer to the top of the
> generated mcode, including the DWARF entries, if present.
> 
> And the following fields were added to the SnapShot structure:
> * `mcofs` -- offset into machine code in  MCode units,
> needed to skip the DWARF entries, if present.
> * `exitcode` -- exit code from unwound trace.
> 
> The following registers were chosen to act as EHRAREG
> (Exception Handler Return Address Register) on each platform:
> * X86 `eip` (8)
> * X64 `rip` (16)
> * ARM `lr` (14)
> * ARM64 `lr` (30) maps to x30
> * PPC `lr` (65) maps to SPR8
> * MIPS `$ra` (31) maps to $31
> 
> Also, introduction of `lj_err_trace` changes the semantics of
> `lj-603-err-snap-restore.test.lua`, since now those errors are handled
> on trace. The test was modified corresponding to the updates.
> 
> Maxim Kokryashkin:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#7745
> Part of tarantool/tarantool#8069
> 
> [1]: https://github.com/gcc-mirror/gcc/blob/ce83c3e492c2fa5a08c15b5f4619d58f42a5dcd0/libgcc/unwind-dw2-fde.c#L149
> [2]: https://opensource.apple.com/source/libunwind/libunwind-201/libunwind/src/UnwindLevel1-gcc-ext.c.auto.html
> [3]: https://opensource.apple.com/source/libunwind/libunwind-201/libunwind/src/libunwind.cpp.auto.html
> [4]: https://github.com/gcc-mirror/gcc/blob/ce83c3e492c2fa5a08c15b5f4619d58f42a5dcd0/libgcc/unwind-dw2-fde.c#L711
> ---
>  CMakeLists.txt                                |   6 +
>  doc/status.html                               |   7 -
>  src/Makefile.original                         |   3 +
>  src/lj_arch.h                                 |  12 +
>  src/lj_asm.c                                  |  77 ++++-
>  src/lj_dispatch.h                             |   4 +-
>  src/lj_err.c                                  | 274 +++++++++++++++++-
>  src/lj_err.h                                  |  19 +-
>  src/lj_ffrecord.c                             |   2 +
>  src/lj_jit.h                                  |   2 +
>  src/lj_mcode.c                                |   5 +-
>  src/lj_opt_loop.c                             |   1 +
>  src/lj_record.c                               |   3 +-
>  src/lj_snap.c                                 |   1 +
>  src/lj_state.c                                |   1 +
>  src/lj_target_x86.h                           |   2 +
>  src/lj_trace.c                                |  61 +++-
>  src/lj_trace.h                                |   3 +
>  src/lj_vm.h                                   |   3 +
>  src/vm_arm.dasc                               |   3 +-
>  src/vm_arm64.dasc                             |   4 +-
>  src/vm_mips.dasc                              |   9 +-
>  src/vm_mips64.dasc                            |  10 +-
>  src/vm_ppc.dasc                               |   3 +-
>  src/vm_x64.dasc                               |   6 +-
>  src/vm_x86.dasc                               |   4 +-
>  .../gh-7745-oom-on-trace.test.lua             |  41 +++
>  .../lj-603-err-snap-restore.test.lua          |  77 +++--
>  28 files changed, 553 insertions(+), 90 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-7745-oom-on-trace.test.lua
> 

<snipped>

> diff --git a/src/lj_asm.c b/src/lj_asm.c
> index fd31cd04..f7c40fea 100644
> --- a/src/lj_asm.c
> +++ b/src/lj_asm.c

<snipped>

> @@ -944,6 +946,14 @@ static void asm_snap_alloc(ASMState *as)
>      if (!irref_isk(ref)) {
>        asm_snap_alloc1(as, ref);
>        if (LJ_SOFTFP && (sn & SNAP_SOFTFPNUM)) {
> +        /*
> +        ** FIXME: The following assert was replaced with
> +        ** the conventional `lua_assert`.
> +        **
> +        ** lj_assertA(irt_type(IR(ref+1)->t) == IRT_SOFTFP,
> +		    ** "snap %d[%d] points to bad SOFTFP IR %04d",
> +		    ** snapno, n, ref - REF_BIAS);

Typo: Invalid indent.

> +        */
>  	lua_assert(irt_type(IR(ref+1)->t) == IRT_SOFTFP);
>  	asm_snap_alloc1(as, ref+1);
>        }

<snipped>

> diff --git a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
> new file mode 100644
> index 00000000..0c495f5c
> --- /dev/null
> +++ b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua

Typo: Mixed intent levels, let's adjust everything to 2 spaces.

> @@ -0,0 +1,41 @@

<snipped>

> diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> index 6eb53dfd..13b8e646 100644
> --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua

Typo: AFAIR, we use indent of 2 spaces instead of 4.

> @@ -6,6 +6,41 @@ local test = tap.test('lj-603-err-snap-restore')

<snipped>

> -- 
> 2.39.2 (Apple Git-143)
> 

-- 
Best regards,
IM