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

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

Hi, Maxim!
Thanks for the fixes!

LGTM, after fixing the comments below.

On 28.03.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
> provided for traces on Linux, and only the FDE on OSX. The

This part is slightly misleading:
JIT compiler emits CIE and FDE unconditionaly for OS. OSX just don't use
CIE.

> 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.

Minor: linewidth is more than 72 symbols

> * `mctoporig` -- holds the pointer to the top of the generated mcode, including

Ditto.

> 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

Ditto.

> 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 `$31` (31)

Minor: `$31` maps to $ra.

> 
> 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
> ---
>  doc/status.html                               |   7 -
>  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             |  22 ++
>  .../lj-603-err-snap-restore.test.lua          |  77 +++--
>  26 files changed, 525 insertions(+), 90 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-7745-oom-on-trace.test.lua
> 

<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..e2cd0304
> --- /dev/null
> +++ b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
> @@ -0,0 +1,22 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +
> +local test = tap.test('OOM on trace'):skipcond({
> +  ['Broken unwiding in tarantool_panic_handler'] = jit.os == 'OSX',

So, maybe we should change the condition to the following:
`_TARANTOOL and (jit.os == 'OSX')`

> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local function memory_payload()
> +    local t = {}

Minor: Something wrong with offset.

> +    for i = 1, 1e10 do
> +        t[ffi.new("uint64_t")] = i

Minor: Typo: s/"/'/g
Also, please, add the comment that the non-GC64 build fails by OOM,
while GC64 build fails by TABOV.

> +    end
> +    print(t)

Looks like this print is excess.

> +end

Tests is very long even for disabled GC64, with the following patch
non-GC64 mode runs much faster (its better to add the corresponding
if, since we don't rely on OOM for GC64 mode):

===================================================================
diff --git a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
index e2cd0304..ce3bccf1 100644
--- a/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
+++ b/test/tarantool-tests/gh-7745-oom-on-trace.test.lua
@@ -13,10 +13,22 @@ local function memory_payload()
     for i = 1, 1e10 do
         t[ffi.new("uint64_t")] = i
     end
-    print(t)
 end
 
+local anchor = {}
+local function eatchunks(size)
+  while true do
+    anchor[ffi.new('char[?]', size)] = 1
+  end
+end
+pcall(eatchunks, 64 * 1024 * 1024)
+
 local res = pcall(memory_payload)
+
+-- Free memory for `test:ok()`.
+anchor = nil
+collectgarbage()
+
 test:ok(res == false)
 
 os.exit(test:check() and 0 or 1)
===================================================================

> +
> +local res = pcall(memory_payload)
> +test:ok(res == false)
> +
> +os.exit(test:check() and 0 or 1)
> 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

<snipped>

> -- 
> 2.37.1 (Apple Git-137.1)
> 

-- 
Best regards,
Sergey Kaplun