[Tarantool-patches] [PATCH luajit v3 1/4] Handle on-trace OOM errors from helper functions.
Sergey Kaplun
skaplun at tarantool.org
Wed Apr 5 09:32:07 MSK 2023
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
More information about the Tarantool-patches
mailing list