[Tarantool-patches] [PATCH luajit 0/2] Fix frames for on trace errors

[Tarantool-patches] [PATCH luajit 0/2] Fix frames for on trace errors

[Maxim Kokryashkin via Tarantool-patches]

This patchset fixes frames for OOM and TABOV on-trace errors.
Tests are disabled on macOS because of macOS 12 issues with
old version of libunwind.

PR: https://github.com/tarantool/tarantool/pull/8909
Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-1004-oom-error-frame
Issues:
https://github.com/luajit/luajit/issues/1004
https://github.com/luajit/luajit/issues/1034

Maxim Kokryashkin (1):
  Fix frame for on-trace out-of-memory error.

Mike Pall (1):
  Fix frame for more types of on-trace error messages.

 src/lj_err.c                                  |  8 +++++
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../lj-1004-oom-error-frame.test.lua          | 36 +++++++++++++++++++
 .../lj-1004-oom-error-frame/CMakeLists.txt    |  1 +
 .../lj-1004-oom-error-frame/testoomframe.c    | 17 +++++++++
 .../lj-1034-tabov-error-frame.test.lua        | 27 ++++++++++++++
 6 files changed, 90 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame.test.lua
 create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
 create mode 100644 test/tarantool-tests/lj-1034-tabov-error-frame.test.lua

--
2.41.0

[Tarantool-patches] [PATCH luajit 1/2] Fix frame for on-trace out-of-memory error.

[Maxim Kokryashkin via Tarantool-patches]

Reported by ruidong007.

(cherry-picked from commit 2d8300c1944f3a62c10f0829e9b7847c5a6f0482)

When an on-trace OOM error is triggered from a frame that is
child in regard to `jit_base`, and `L->base` is not updated
correspondingly (FUNCC, for example), it is possible to
encounter an inconsistent Lua stack in the error handler.

This patch adds a fixup for OOM errors on the trace that always
sets the Lua stack base to `jit_base`, so the stack is
now consistent.

Part of tarantool/tarantool#8825
---
 src/lj_err.c                                  |  4 +++
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../lj-1004-oom-error-frame.test.lua          | 36 +++++++++++++++++++
 .../lj-1004-oom-error-frame/CMakeLists.txt    |  1 +
 .../lj-1004-oom-error-frame/testoomframe.c    | 17 +++++++++
 5 files changed, 59 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame.test.lua
 create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c

diff --git a/src/lj_err.c b/src/lj_err.c
index 89c51e98..763746e6 100644
--- a/src/lj_err.c
+++ b/src/lj_err.c
@@ -777,6 +777,10 @@ LJ_NOINLINE void lj_err_mem(lua_State *L)
 {
   if (L->status == LUA_ERRERR+1)  /* Don't touch the stack during lua_open. */
     lj_vm_unwind_c(L->cframe, LUA_ERRMEM);
+  if (LJ_HASJIT) {
+    TValue *base = tvref(G(L)->jit_base);
+    if (base) L->base = base;
+  }
   if (curr_funcisL(L)) L->top = curr_topL(L);
   setstrV(L, L->top++, lj_err_str(L, LJ_ERR_ERRMEM));
   lj_err_throw(L, LUA_ERRMEM);
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 6218f76a..93230677 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -66,6 +66,7 @@ add_subdirectory(lj-416-xor-before-jcc)
 add_subdirectory(lj-601-fix-gc-finderrfunc)
 add_subdirectory(lj-727-lightuserdata-itern)
 add_subdirectory(lj-flush-on-trace)
+add_subdirectory(lj-1004-oom-error-frame)
 
 # The part of the memory profiler toolchain is located in tools
 # directory, jit, profiler, and bytecode toolchains are located
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
new file mode 100644
index 00000000..b6b5a9f2
--- /dev/null
+++ b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
@@ -0,0 +1,36 @@
+local tap = require('tap')
+local ffi = require('ffi')
+local test  = tap.test('lj-1004-oom-error-frame'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+  ['Test requires GC64 mode disabled'] = ffi.abi('gc64'),
+  ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
+})
+
+test:plan(2)
+
+local testoomframe = require('testoomframe')
+
+local anchor_memory = {} -- luacheck: no unused
+local function eatchunks(size)
+  while true do
+    anchor_memory[ffi.new('char[?]', size)] = 1
+  end
+end
+
+pcall(eatchunks, 512 * 1024 * 1024)
+
+local anchor = {}
+local function extra_frame(val)
+  table.insert(anchor, val)
+end
+
+local function chomp()
+  while true do
+    extra_frame(testoomframe.allocate_userdata())
+  end
+end
+
+local st, err = pcall(chomp)
+test:ok(st == false, 'on-trace error handled successfully')
+test:like(err, 'not enough memory', 'error is OOM')
+test:done(true)
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
new file mode 100644
index 00000000..3bca5df8
--- /dev/null
+++ b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
@@ -0,0 +1 @@
+BuildTestCLib(testoomframe testoomframe.c)
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
new file mode 100644
index 00000000..a54eac63
--- /dev/null
+++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
@@ -0,0 +1,17 @@
+#include <lua.h>
+#include <lauxlib.h>
+
+static int allocate_userdata(lua_State *L) {
+	lua_newuserdata(L, 1);
+	return 1;
+}
+
+static const struct luaL_Reg testoomframe[] = {
+	{"allocate_userdata", allocate_userdata},
+	{NULL, NULL}
+};
+
+LUA_API int luaopen_testoomframe(lua_State *L) {
+	luaL_register(L, "testoomframe", testoomframe);
+	return 1;
+}
-- 
2.41.0

[Tarantool-patches] [PATCH luajit 2/2] Fix frame for more types of on-trace error messages.

[Maxim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Maxim Kokryashkin.

(cherry-picked from commit d5bbf9cdb4c5eddc404a90bd44f077cfb3a57a90)

This patch fixes the same issue with frame, as the previous
one, but now for the table overflow error in the `err_msgv`
function. The test for the problem uses the table of GC
finalizers, although they are not required to reproduce the
issue. They only used to make the test as simple as possible.

Resolves tarantool/tarantool#562
Part of tarantool/tarantool#8825
---
 src/lj_err.c                                  |  4 +++
 .../lj-1034-tabov-error-frame.test.lua        | 27 +++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1034-tabov-error-frame.test.lua

diff --git a/src/lj_err.c b/src/lj_err.c
index 763746e6..46fb81ee 100644
--- a/src/lj_err.c
+++ b/src/lj_err.c
@@ -875,6 +875,10 @@ LJ_NORET LJ_NOINLINE static void err_msgv(lua_State *L, ErrMsg em, ...)
   const char *msg;
   va_list argp;
   va_start(argp, em);
+  if (LJ_HASJIT) {
+    TValue *base = tvref(G(L)->jit_base);
+    if (base) L->base = base;
+  }
   if (curr_funcisL(L)) L->top = curr_topL(L);
   msg = lj_strfmt_pushvf(L, err2msg(em), argp);
   va_end(argp);
diff --git a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
new file mode 100644
index 00000000..b7520d92
--- /dev/null
+++ b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
@@ -0,0 +1,27 @@
+local tap = require('tap')
+local ffi = require('ffi')
+local test = tap.test('lj-1034-tabov-error-frame'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+  ['Test requires GC64 mode enabled'] = not ffi.abi('gc64'),
+  ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
+})
+
+test:plan(2)
+
+-- luacheck: no unused
+local anchor = {}
+local function on_gc(t) end
+
+local function test_finalizers()
+    local i = 1
+    while true do
+        anchor[i] = ffi.gc(ffi.cast('void *', 0), on_gc)
+        i = i + 1
+    end
+end
+
+local st, err = pcall(test_finalizers)
+st, err = pcall(test_finalizers)
+test:ok(st == false, 'error handled successfully')
+test:like(err, '^.+table overflow', 'error is table overflow')
+test:done(true)
-- 
2.41.0

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix frame for on-trace out-of-memory error.

[Sergey Bronnikov via Tarantool-patches]

Hi, Max


see my comments below

On 9/5/23 13:39, Maxim Kokryashkin wrote:
> Reported by ruidong007.
>
> (cherry-picked from commit 2d8300c1944f3a62c10f0829e9b7847c5a6f0482)
>
> When an on-trace OOM error is triggered from a frame that is
> child in regard to `jit_base`, and `L->base` is not updated
> correspondingly (FUNCC, for example), it is possible to
> encounter an inconsistent Lua stack in the error handler.
>
> This patch adds a fixup for OOM errors on the trace that always
> sets the Lua stack base to `jit_base`, so the stack is
> now consistent.
>
> Part of tarantool/tarantool#8825
> ---


<snipped>

> +local testoomframe = require('testoomframe')
> +
> +local anchor_memory = {} -- luacheck: no unused
> +local function eatchunks(size)
> +  while true do
> +    anchor_memory[ffi.new('char[?]', size)] = 1


Why ffi.new() is a key, not a value?

> +  end
> +end
> +
> +pcall(eatchunks, 512 * 1024 * 1024)

Why exactly this size is used?


> +
> +local anchor = {}
> +local function extra_frame(val)
> +  table.insert(anchor, val)
> +end
> +
> +local function chomp()
> +  while true do
> +    extra_frame(testoomframe.allocate_userdata())
> +  end
> +end
> +
> +local st, err = pcall(chomp)
> +test:ok(st == false, 'on-trace error handled successfully')
> +test:like(err, 'not enough memory', 'error is OOM')
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
> new file mode 100644
> index 00000000..3bca5df8
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
> @@ -0,0 +1 @@
> +BuildTestCLib(testoomframe testoomframe.c)
> diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> new file mode 100644
> index 00000000..a54eac63
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> @@ -0,0 +1,17 @@
> +#include <lua.h>
> +#include <lauxlib.h>

Test uses headers provided by systems instead of headers provided by 
LuaJIT-under-test. It is expected?

--- a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
+++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
@@ -1,5 +1,5 @@
-#include <lua.h>
-#include <lauxlib.h>
+#include "lua.h"
+#include "lauxlib.h"

  static int allocate_userdata(lua_State *L) {
         lua_newuserdata(L, 1);

> +
> +static int allocate_userdata(lua_State *L) {
> +	lua_newuserdata(L, 1);
> +	return 1;
> +}
> +
> +static const struct luaL_Reg testoomframe[] = {
> +	{"allocate_userdata", allocate_userdata},
> +	{NULL, NULL}
> +};
> +
> +LUA_API int luaopen_testoomframe(lua_State *L) {
> +	luaL_register(L, "testoomframe", testoomframe);
> +	return 1;
> +}

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix frame for more types of on-trace error messages.

[Sergey Bronnikov via Tarantool-patches]

Hi, Max


test is passed after reverting the patch with fix.

On 9/5/23 13:39, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> Thanks to Maxim Kokryashkin.
>
> (cherry-picked from commit d5bbf9cdb4c5eddc404a90bd44f077cfb3a57a90)
>
> This patch fixes the same issue with frame, as the previous
> one, but now for the table overflow error in the `err_msgv`
> function. The test for the problem uses the table of GC
> finalizers, although they are not required to reproduce the
> issue. They only used to make the test as simple as possible.
>
> Resolves tarantool/tarantool#562
> Part of tarantool/tarantool#8825
> ---
<snipped>

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix frame for more types of on-trace error messages.

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]


Hi! Did you test it with GC64 enabled?
 
 
--
Best regards,
Maxim Kokryashkin
 
  
>Четверг, 7 сентября 2023, 11:11 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>Hi, Max
>
>
>test is passed after reverting the patch with fix.
>
>On 9/5/23 13:39, Maxim Kokryashkin wrote:
>> From: Mike Pall <mike>
>>
>> Thanks to Maxim Kokryashkin.
>>
>> (cherry-picked from commit d5bbf9cdb4c5eddc404a90bd44f077cfb3a57a90)
>>
>> This patch fixes the same issue with frame, as the previous
>> one, but now for the table overflow error in the `err_msgv`
>> function. The test for the problem uses the table of GC
>> finalizers, although they are not required to reproduce the
>> issue. They only used to make the test as simple as possible.
>>
>> Resolves tarantool/tarantool#562
>> Part of tarantool/tarantool#8825
>> --- <snipped>
 

[-- Attachment #2: Type: text/html, Size: 1460 bytes --]

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix frame for more types of on-trace error messages.

[Sergey Bronnikov via Tarantool-patches]

On 9/7/23 11:56, Maxim Kokryashkin wrote:
> Hi! Did you test it with GC64 enabled?

Seems it was a build wo enabled GC64. So LGTM.

<snipped>

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix frame for on-trace out-of-memory error.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
See my answers below.

> > +local testoomframe = require('testoomframe')
> > +
> > +local anchor_memory = {} -- luacheck: no unused
> > +local function eatchunks(size)
> > +  while true do
> > +    anchor_memory[ffi.new('char[?]', size)] = 1
> 
> 
> Why ffi.new() is a key, not a value?
Its just a very common pattern in our tests, also I don't
really want to manage a separate counter for the key. ffi.new()
can be a value as well, no issue with that.
> 
> > +  end
> > +end
> > +
> > +pcall(eatchunks, 512 * 1024 * 1024)
> 
> Why exactly this size is used?
It is empirically selected number, dropped a comment.

> 
> 
> > +
> > +local anchor = {}
> > +local function extra_frame(val)
> > +  table.insert(anchor, val)
> > +end
> > +
> > +local function chomp()
> > +  while true do
> > +    extra_frame(testoomframe.allocate_userdata())
> > +  end
> > +end
> > +
> > +local st, err = pcall(chomp)
> > +test:ok(st == false, 'on-trace error handled successfully')
> > +test:like(err, 'not enough memory', 'error is OOM')
> > +test:done(true)
> > diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
> > new file mode 100644
> > index 00000000..3bca5df8
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
> > @@ -0,0 +1 @@
> > +BuildTestCLib(testoomframe testoomframe.c)
> > diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> > new file mode 100644
> > index 00000000..a54eac63
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> > @@ -0,0 +1,17 @@
> > +#include <lua.h>
> > +#include <lauxlib.h>
> 
> Test uses headers provided by systems instead of headers provided by
> LuaJIT-under-test. It is expected?
Fixed, thanks.
> 
> --- a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> +++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> @@ -1,5 +1,5 @@
> -#include <lua.h>
> -#include <lauxlib.h>
> +#include "lua.h"
> +#include "lauxlib.h"
> 
>  static int allocate_userdata(lua_State *L) {
>         lua_newuserdata(L, 1);
> 
> > +
> > +static int allocate_userdata(lua_State *L) {
> > +	lua_newuserdata(L, 1);
> > +	return 1;
> > +}
> > +
> > +static const struct luaL_Reg testoomframe[] = {
> > +	{"allocate_userdata", allocate_userdata},
> > +	{NULL, NULL}
> > +};
> > +
> > +LUA_API int luaopen_testoomframe(lua_State *L) {
> > +	luaL_register(L, "testoomframe", testoomframe);
> > +	return 1;
> > +}

Here is the diff with changes. Branch is force-psuhed.
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
index b6b5a9f2..3be6b555 100644
--- a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
+++ b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
@@ -17,6 +17,9 @@ local function eatchunks(size)
   end
 end
 
+-- The chunk size below is empirical. It is big enough, so the test
+-- is not too long, yet small enough for the OOM frame issue to have
+-- enough iterations in the second loop to trigger.
 pcall(eatchunks, 512 * 1024 * 1024)
 
 local anchor = {}
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
index a54eac63..dbfe17db 100644
--- a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
+++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
@@ -1,5 +1,5 @@
-#include <lua.h>
-#include <lauxlib.h>
+#include "lua.h"
+#include "lauxlib.h"
 
 static int allocate_userdata(lua_State *L) {
 	lua_newuserdata(L, 1);

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix frame for on-trace out-of-memory error.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM, just two minor nits below.

On 05.09.23, Maxim Kokryashkin wrote:
> Reported by ruidong007.

<snipped>

> diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> new file mode 100644
> index 00000000..a54eac63
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
> @@ -0,0 +1,17 @@
> +#include <lua.h>
> +#include <lauxlib.h>
> +
> +static int allocate_userdata(lua_State *L) {

Nit: Please, start function's block from the new line, i.e:

| {
| 	lua_newuserdata(L, 1);
| 	return 1;
| }

> +	lua_newuserdata(L, 1);
> +	return 1;
> +}
> +
> +static const struct luaL_Reg testoomframe[] = {
> +	{"allocate_userdata", allocate_userdata},
> +	{NULL, NULL}
> +};
> +
> +LUA_API int luaopen_testoomframe(lua_State *L) {

Ditto.

> +	luaL_register(L, "testoomframe", testoomframe);
> +	return 1;
> +}
> -- 
> 2.41.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix frame for more types of on-trace error messages.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM, with a few nits below.

On 05.09.23, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> Thanks to Maxim Kokryashkin.
> 
> (cherry-picked from commit d5bbf9cdb4c5eddc404a90bd44f077cfb3a57a90)
> 
> This patch fixes the same issue with frame, as the previous

Typo: s/frame,/the frame/

> one, but now for the table overflow error in the `err_msgv`
> function. The test for the problem uses the table of GC
> finalizers, although they are not required to reproduce the
> issue. They only used to make the test as simple as possible.

Typo: s/They only used/They are only used/

Minor: Feel free to add the similar comment to the test itself.

> 
> Resolves tarantool/tarantool#562
> Part of tarantool/tarantool#8825
> ---
>  src/lj_err.c                                  |  4 +++
>  .../lj-1034-tabov-error-frame.test.lua        | 27 +++++++++++++++++++
>  2 files changed, 31 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> 
> diff --git a/src/lj_err.c b/src/lj_err.c
> index 763746e6..46fb81ee 100644
> --- a/src/lj_err.c
> +++ b/src/lj_err.c

<snipped>

> diff --git a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> new file mode 100644
> index 00000000..b7520d92
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> @@ -0,0 +1,27 @@
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local test = tap.test('lj-1034-tabov-error-frame'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +  ['Test requires GC64 mode enabled'] = not ffi.abi('gc64'),
> +  ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
> +})
> +
> +test:plan(2)
> +

Side note: The test requires ~6Gb of memory to see the error.
Feel free to add the corresponding comment or ignore.

> +-- luacheck: no unused
> +local anchor = {}
> +local function on_gc(t) end
> +
> +local function test_finalizers()
> +    local i = 1
> +    while true do
> +        anchor[i] = ffi.gc(ffi.cast('void *', 0), on_gc)
> +        i = i + 1
> +    end
> +end

Minor: Please use 2 spaces for indentation instead.

> +
> +local st, err = pcall(test_finalizers)
> +st, err = pcall(test_finalizers)
> +test:ok(st == false, 'error handled successfully')
> +test:like(err, '^.+table overflow', 'error is table overflow')
> +test:done(true)
> -- 
> 2.41.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix frame for on-trace out-of-memory error.

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]


Hi!
Thanks for the review!
Fixed your comments, branch is force-pushed, here is the diff:
 
diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
index dbfe17db..82f64f01 100644
--- a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
+++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
@@ -1,7 +1,8 @@
 #include "lua.h"
 #include "lauxlib.h"
 
-static int allocate_userdata(lua_State *L) {
+static int allocate_userdata(lua_State *L)
+{
     lua_newuserdata(L, 1);
     return 1;
 }
@@ -11,7 +12,8 @@ static const struct luaL_Reg testoomframe[] = {
     {NULL, NULL}
 };
 
-LUA_API int luaopen_testoomframe(lua_State *L) {
+LUA_API int luaopen_testoomframe(lua_State *L)
+{
     luaL_register(L, "testoomframe", testoomframe);
     return 1;
 }
 
--
Best regards,
Maxim Kokryashkin
 
  
>Понедельник, 11 сентября 2023, 11:09 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>Thanks for the patch!
>LGTM, just two minor nits below.
>
>On 05.09.23, Maxim Kokryashkin wrote:
>> Reported by ruidong007.
>
><snipped>
>
>> diff --git a/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
>> new file mode 100644
>> index 00000000..a54eac63
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
>> @@ -0,0 +1,17 @@
>> +#include <lua.h>
>> +#include <lauxlib.h>
>> +
>> +static int allocate_userdata(lua_State *L) {
>
>Nit: Please, start function's block from the new line, i.e:
>
>| {
>| lua_newuserdata(L, 1);
>| return 1;
>| }
>
>> + lua_newuserdata(L, 1);
>> + return 1;
>> +}
>> +
>> +static const struct luaL_Reg testoomframe[] = {
>> + {"allocate_userdata", allocate_userdata},
>> + {NULL, NULL}
>> +};
>> +
>> +LUA_API int luaopen_testoomframe(lua_State *L) {
>
>Ditto.
>
>> + luaL_register(L, "testoomframe", testoomframe);
>> + return 1;
>> +}
>> --
>> 2.41.0
>>
>
>--
>Best regards,
>Sergey Kaplun
 

[-- Attachment #2: Type: text/html, Size: 3109 bytes --]

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix frame for more types of on-trace error messages.

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4474 bytes --]


Hi, Sergey!
Thanks for the review!
Fixed your comments, branch is force-pushed.
Here is the new commit message:
===
Fix frame for more types of on-trace error messages.
 
Thanks to Maxim Kokryashkin.
 
(cherry-picked from commit d5bbf9cdb4c5eddc404a90bd44f077cfb3a57a90)
 
This patch fixes the same issue with the frame as the previous
one, but now for the table overflow error in the `err_msgv`
function. The test for the problem uses the table of GC
finalizers, although they are not required to reproduce the
issue. They are only used to make the test as simple as possible.
 
Resolves tarantool/tarantool#562
Part of tarantool/tarantool#8825
===
 
And here is the diff:
 
diff --git a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
index b7520d92..0e23fdb2 100644
--- a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
+++ b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
@@ -6,6 +6,12 @@ local test = tap.test('lj-1034-tabov-error-frame'):skipcond({
   ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
 })
 
+-- XXX: The test for the problem uses the table of GC
+-- finalizers, although they are not required to reproduce
+-- the issue. They are only used to make the test as simple
+-- as possible.
+--
+-- XXX: The test requires ~6Gb of memory to see the error.
 test:plan(2)
 
 -- luacheck: no unused
@@ -13,11 +19,11 @@ local anchor = {}
 local function on_gc(t) end
 
 local function test_finalizers()
-    local i = 1
-    while true do
-        anchor[i] = ffi.gc(ffi.cast('void *', 0), on_gc)
-        i = i + 1
-    end
+  local i = 1
+  while true do
+    anchor[i] = ffi.gc(ffi.cast('void *', 0), on_gc)
+    i = i + 1
+  end
 end
 
 local st, err = pcall(test_finalizers)
--
Best regards,
Maxim Kokryashkin
 
  
>Понедельник, 11 сентября 2023, 11:25 +03:00 от Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>Hi, Maxim!
>Thanks for the patch!
>LGTM, with a few nits below.
>
>On 05.09.23, Maxim Kokryashkin wrote:
>> From: Mike Pall <mike>
>>
>> Thanks to Maxim Kokryashkin.
>>
>> (cherry-picked from commit d5bbf9cdb4c5eddc404a90bd44f077cfb3a57a90)
>>
>> This patch fixes the same issue with frame, as the previous
>
>Typo: s/frame,/the frame/
>
>> one, but now for the table overflow error in the `err_msgv`
>> function. The test for the problem uses the table of GC
>> finalizers, although they are not required to reproduce the
>> issue. They only used to make the test as simple as possible.
>
>Typo: s/They only used/They are only used/
>
>Minor: Feel free to add the similar comment to the test itself.
>
>>
>> Resolves tarantool/tarantool#562
>> Part of tarantool/tarantool#8825
>> ---
>> src/lj_err.c | 4 +++
>> .../lj-1034-tabov-error-frame.test.lua | 27 +++++++++++++++++++
>> 2 files changed, 31 insertions(+)
>> create mode 100644 test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
>>
>> diff --git a/src/lj_err.c b/src/lj_err.c
>> index 763746e6..46fb81ee 100644
>> --- a/src/lj_err.c
>> +++ b/src/lj_err.c
>
><snipped>
>
>> diff --git a/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
>> new file mode 100644
>> index 00000000..b7520d92
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
>> @@ -0,0 +1,27 @@
>> +local tap = require('tap')
>> +local ffi = require('ffi')
>> +local test = tap.test('lj-1034-tabov-error-frame'):skipcond({
>> + ['Test requires JIT enabled'] = not jit.status(),
>> + ['Test requires GC64 mode enabled'] = not ffi.abi('gc64'),
>> + ['Disabled on MacOS due to #8652'] = jit.os == 'OSX',
>> +})
>> +
>> +test:plan(2)
>> +
>
>Side note: The test requires ~6Gb of memory to see the error.
>Feel free to add the corresponding comment or ignore.
>
>> +-- luacheck: no unused
>> +local anchor = {}
>> +local function on_gc(t) end
>> +
>> +local function test_finalizers()
>> + local i = 1
>> + while true do
>> + anchor[i] = ffi.gc(ffi.cast('void *', 0), on_gc)
>> + i = i + 1
>> + end
>> +end
>
>Minor: Please use 2 spaces for indentation instead.
>
>> +
>> +local st, err = pcall(test_finalizers)
>> +st, err = pcall(test_finalizers)
>> +test:ok(st == false, 'error handled successfully')
>> +test:like(err, '^.+table overflow', 'error is table overflow')
>> +test:done(true)
>> --
>> 2.41.0
>>
>
>--
>Best regards,
>Sergey Kaplun
 

[-- Attachment #2: Type: text/html, Size: 6061 bytes --]

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix frame for on-trace out-of-memory error.

[Sergey Bronnikov via Tarantool-patches]

Hi,


On 9/8/23 16:18, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the review!
> See my answers below.
>

Thanks for update!


In version on the branch I see that function test_finalizers called twice.

Let's add a comment with explanation for this.


LGTM


Sergey

Re: [Tarantool-patches] [PATCH luajit 0/2] Fix frames for on trace errors

[Igor Munkin via Tarantool-patches]

Max,

I've checked the patchset into tarantool/master branch in
tarantool/luajit and bumped a new version in master.

On 05.09.23, Maxim Kokryashkin via Tarantool-patches wrote:
> This patchset fixes frames for OOM and TABOV on-trace errors.
> Tests are disabled on macOS because of macOS 12 issues with
> old version of libunwind.
> 
> PR: https://github.com/tarantool/tarantool/pull/8909
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-1004-oom-error-frame
> Issues:
> https://github.com/luajit/luajit/issues/1004
> https://github.com/luajit/luajit/issues/1034
> 
> Maxim Kokryashkin (1):
>   Fix frame for on-trace out-of-memory error.
> 
> Mike Pall (1):
>   Fix frame for more types of on-trace error messages.
> 
>  src/lj_err.c                                  |  8 +++++
>  test/tarantool-tests/CMakeLists.txt           |  1 +
>  .../lj-1004-oom-error-frame.test.lua          | 36 +++++++++++++++++++
>  .../lj-1004-oom-error-frame/CMakeLists.txt    |  1 +
>  .../lj-1004-oom-error-frame/testoomframe.c    | 17 +++++++++
>  .../lj-1034-tabov-error-frame.test.lua        | 27 ++++++++++++++
>  6 files changed, 90 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame.test.lua
>  create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/CMakeLists.txt
>  create mode 100644 test/tarantool-tests/lj-1004-oom-error-frame/testoomframe.c
>  create mode 100644 test/tarantool-tests/lj-1034-tabov-error-frame.test.lua
> 
> --
> 2.41.0
> 

-- 
Best regards,
IM