[Tarantool-patches] [PATCH luajit][v2] Handle all stack layouts in (delayed) TRACE vmevent.

[Tarantool-patches] [PATCH luajit][v2] Handle all stack layouts in (delayed) TRACE vmevent.

[Sergey Bronnikov via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Sergey Bronnikov and Peter Cawley.

(cherry picked from commit b138ccfa918518a152bc830fef3d53cd0a922e36)

When recording a trace using a Lua code like
`repeat until `a >= 'b' > 'b'` a Lua error is encountered
(`attempt to compare string with nil`), which (along with
raising the error) causes an asynchronous trace abort.
The trace abort remains pending until the next reentrance to the
LuaJIT VM via `lj_vm_call()` or `lj_vm_pcall().

On handling abort LuaJIT is searching for the topmost Lua frame
on the stack, that was active when the trace abort happened,
it is needed to generate a better error message.
Unfortunately, because the abort was due to an error, and
the error was caught by a `lua_pcall` with an unspecified
error function (4th argument), the Lua frame that caused the abort
was already removed as part of error processing, so the search
cannot find it. Furthermore, in this particular case, there are
no Lua frames on the stack, which isn't something the search code
had considered possible.

Sergey Bronnikov:
* added the description and the test for the problem

Part of tarantool/tarantool#11055
---
Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-1087-vm-handler-call

Related issues:
- https://github.com/LuaJIT/LuaJIT/issues/1087
- https://github.com/tarantool/tarantool/issues/10709

Changes in the v2:
* Addressed comments by Sergey Kaplun

 src/lj_trace.c                                |  26 +--
 .../lj-1087-vm-handler-call.test.c            | 170 ++++++++++++++++++
 2 files changed, 186 insertions(+), 10 deletions(-)
 create mode 100644 test/tarantool-c-tests/lj-1087-vm-handler-call.test.c

diff --git a/src/lj_trace.c b/src/lj_trace.c
index 94cb27e5..6b97cc13 100644
--- a/src/lj_trace.c
+++ b/src/lj_trace.c
@@ -616,21 +616,27 @@ static int trace_abort(jit_State *J)
     J->cur.link = 0;
     J->cur.linktype = LJ_TRLINK_NONE;
     lj_vmevent_send(L, TRACE,
-      TValue *frame;
+      cTValue *bot = tvref(L->stack)+LJ_FR2;
+      cTValue *frame;
       const BCIns *pc;
-      GCfunc *fn;
+      BCPos pos = 0;
       setstrV(L, L->top++, lj_str_newlit(L, "abort"));
       setintV(L->top++, traceno);
       /* Find original Lua function call to generate a better error message. */
-      frame = J->L->base-1;
-      pc = J->pc;
-      while (!isluafunc(frame_func(frame))) {
-	pc = (frame_iscont(frame) ? frame_contpc(frame) : frame_pc(frame)) - 1;
-	frame = frame_prev(frame);
+      for (frame = J->L->base-1, pc = J->pc; ; frame = frame_prev(frame)) {
+	if (isluafunc(frame_func(frame))) {
+	  pos = proto_bcpos(funcproto(frame_func(frame)), pc);
+	  break;
+	} else if (frame_prev(frame) <= bot) {
+	  break;
+	} else if (frame_iscont(frame)) {
+	  pc = frame_contpc(frame) - 1;
+	} else {
+	  pc = frame_pc(frame) - 1;
+	}
       }
-      fn = frame_func(frame);
-      setfuncV(L, L->top++, fn);
-      setintV(L->top++, proto_bcpos(funcproto(fn), pc));
+      setfuncV(L, L->top++, frame_func(frame));
+      setintV(L->top++, pos);
       copyTV(L, L->top++, restorestack(L, errobj));
       copyTV(L, L->top++, &J->errinfo);
     );
diff --git a/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c b/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c
new file mode 100644
index 00000000..ebdf63e1
--- /dev/null
+++ b/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c
@@ -0,0 +1,170 @@
+#include "lua.h"
+#include "lauxlib.h"
+
+#include "test.h"
+#include "utils.h"
+
+/* Need for skipcond for BSD and JIT. */
+#include "lj_arch.h"
+
+/* XXX: Still need normal assert inside writer functions. */
+#undef NDEBUG
+#include <assert.h>
+
+/*
+ * Test file to demonstrate a segmentation fault, when C function
+ * is used as a VM handler in LuaJIT:
+ *
+ * Program received signal SIGSEGV, Segmentation fault.
+ * 0x000055555557e77d in trace_abort (J=0x7ffff7f9b6b8) at lj_trace.c:615
+ * 615         lj_vmevent_send(L, TRACE,
+ * (gdb) bt
+ *
+ * See details in https://github.com/LuaJIT/LuaJIT/issues/1087.
+ */
+
+static int nop(lua_State *L)
+{
+	UNUSED(L);
+	return 0;
+}
+
+/* Note, `event == NULL` disables the corresponding handler. */
+static void jit_attach(lua_State *L, lua_CFunction cb, const char *event)
+{
+	lua_getglobal(L, "jit");
+	lua_getfield(L, -1, "attach");
+	lua_pushcfunction(L, cb);
+	if (event != NULL)
+		lua_pushstring(L, event);
+	else
+		lua_pushnil(L);
+	lua_pcall(L, 2, 0, 0);
+}
+
+static int handle_luafunc_frame(void *test_state)
+{
+	/* Setup. */
+	lua_State *L = test_state;
+	jit_attach(L, nop, "trace");
+
+	/* Loading and executing of a broken Lua code. */
+	int rc = luaL_dostring(L, "repeat until nil > 1");
+	assert(rc == 1);
+
+	/* The Lua chunk generates a Lua frame. */
+	rc = luaL_dostring(L, "return function() end");
+	assert(rc == 0);
+
+	/* Teardown. */
+	lua_settop(L, 0);
+
+	return TEST_EXIT_SUCCESS;
+}
+
+static int cframe(lua_State *L)
+{
+	int rc = luaL_dostring(L, "repeat until nil > 1");
+	assert(rc == 1);
+	/* Remove errmsg. */
+	lua_pop(L, 1);
+
+	lua_pushcfunction(L, nop);
+	lua_call(L, 0, 0);
+
+	return 0;
+}
+
+static int handle_c_frame(void *test_state)
+{
+	/* Setup. */
+	lua_State *L = test_state;
+	jit_attach(L, nop, "trace");
+
+	lua_pushcfunction(L, cframe);
+	lua_call(L, 0, 0);
+
+	/* Teardown. */
+	lua_settop(L, 0);
+
+	return TEST_EXIT_SUCCESS;
+}
+
+static int handle_cont_frame(void *test_state)
+{
+	const char lua_chunk[] =
+		"local t = setmetatable({}, { __index = global_f })"
+		"for i = 1, 4 do"
+		"    _ = t[1]"
+		"end";
+
+	/* Setup. */
+	lua_State *L = test_state;
+	jit_attach(L, nop, "trace");
+
+	/*
+	 * The number 32767 is `REF_DROP - REF_BIAS`. See the commit
+	 * 0fdf06b456e6 ("test: relax JIT setup in misc.getmetrics
+	 * test") for the details.
+	 */
+	int res = luaL_dostring(L, "jit.opt.start('minstitch=32767')");
+	assert(res == 0);
+	lua_pushcfunction(L, nop);
+	lua_setglobal(L, "global_f");
+
+	res = luaL_dostring(L, lua_chunk);
+	assert(res == 0);
+
+	/* Teardown. */
+	lua_settop(L, 0);
+	res = luaL_dostring(L, "jit.opt.start('minstitch=0')");
+	assert(res == 0);
+
+	return TEST_EXIT_SUCCESS;
+}
+
+static int handle_bottom_frame(void *test_state)
+{
+	lua_State *L = test_state;
+
+	/* Attach VM call handler. */
+	jit_attach(L, nop, "trace");
+
+	/* Load a Lua code that generate a trace abort. */
+	int rc = luaL_dostring(L, "repeat until nil > 1");
+	assert(rc == 1);
+
+	/*
+	 * Note, protected call is used inside `jit_attach()` to
+	 * return to the VM on disabling the handler. Then
+	 * a segmentation fault is triggered.
+	 */
+	jit_attach(L, nop, NULL);
+
+	/* Teardown. */
+	lua_settop(L, 0);
+
+	return TEST_EXIT_SUCCESS;
+}
+
+int main(void)
+{
+	if (!LJ_HASJIT)
+		return skip_all("JIT is disabled");
+
+	if (LUAJIT_OS == LUAJIT_OS_BSD)
+		return skip_all("Disabled on *BSD due to #4819");
+
+	lua_State *L = utils_lua_init();
+	const struct test_unit tgroup[] = {
+		test_unit_def(handle_luafunc_frame),
+		test_unit_def(handle_bottom_frame),
+		test_unit_def(handle_cont_frame),
+		test_unit_def(handle_c_frame),
+	};
+	int res = luaL_dostring(L, "jit.opt.start('hotloop=1')");
+	assert(res == 0);
+	const int test_result = test_run_group(tgroup, L);
+	utils_lua_close(L);
+	return test_result;
+}
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit][v2] Handle all stack layouts in (delayed) TRACE vmevent.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing a bunch of nits below.

On 06.02.25, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 

<snipped>

> ---
> Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-1087-vm-handler-call
> 
> Related issues:
> - https://github.com/LuaJIT/LuaJIT/issues/1087
> - https://github.com/tarantool/tarantool/issues/10709
> 
> Changes in the v2:
> * Addressed comments by Sergey Kaplun
> 

<snipped>

> diff --git a/src/lj_trace.c b/src/lj_trace.c
> index 94cb27e5..6b97cc13 100644
> --- a/src/lj_trace.c
> +++ b/src/lj_trace.c

<snipped>

> diff --git a/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c b/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c
> new file mode 100644
> index 00000000..ebdf63e1
> --- /dev/null
> +++ b/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c

<snipped>

> +/* XXX: Still need normal assert inside writer functions. */

Minor: Since there is no writer function, I would rather use the
following wording:
| Still need normal assert for sanity checks.

> +#undef NDEBUG
> +#include <assert.h>
> +
> +/*
> + * Test file to demonstrate a segmentation fault, when C function

Typo: s/C function/a C function/

> + * is used as a VM handler in LuaJIT:

Minor: `as a VM handler for trace events` is a little bit more verbose.
Feel free to ignore.

> + *
> + * Program received signal SIGSEGV, Segmentation fault.
> + * 0x000055555557e77d in trace_abort (J=0x7ffff7f9b6b8) at lj_trace.c:615
> + * 615         lj_vmevent_send(L, TRACE,
> + * (gdb) bt
> + *
> + * See details in https://github.com/LuaJIT/LuaJIT/issues/1087.
> + */
> +

<snipped>

> +	/*
> +	 * The number 32767 is `REF_DROP - REF_BIAS`.

It will be better to rephrase the next sentence.

>                                                  See the commit
> +	 * 0fdf06b456e6 ("test: relax JIT setup in misc.getmetrics
> +	 * test") for the details.
> +	 */

I suggest the following comment.
| This is the maximum possible IR amount, so the trace is always
| aborted.

The amount of text to read is approximately the same, but we don't need
to look into the mysterious commit anymore. :)

> +	int res = luaL_dostring(L, "jit.opt.start('minstitch=32767')");

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit][v2] Handle all stack layouts in (delayed) TRACE vmevent.

[Sergey Bronnikov via Tarantool-patches]

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

Bonjour, Sergey,

fixed and force-pushed.

Sergey


On 07.02.2025 13:50, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, after fixing a bunch of nits below.
>
> On 06.02.25, Sergey Bronnikov wrote:
>> From: Mike Pall <mike>
>>
> <snipped>
>
>> ---
>> Branch:https://github.com/tarantool/luajit/tree/ligurio/lj-1087-vm-handler-call
>>
>> Related issues:
>> -https://github.com/LuaJIT/LuaJIT/issues/1087
>> -https://github.com/tarantool/tarantool/issues/10709
>>
>> Changes in the v2:
>> * Addressed comments by Sergey Kaplun
>>
> <snipped>
>
>> diff --git a/src/lj_trace.c b/src/lj_trace.c
>> index 94cb27e5..6b97cc13 100644
>> --- a/src/lj_trace.c
>> +++ b/src/lj_trace.c
> <snipped>
>
>> diff --git a/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c b/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c
>> new file mode 100644
>> index 00000000..ebdf63e1
>> --- /dev/null
>> +++ b/test/tarantool-c-tests/lj-1087-vm-handler-call.test.c
> <snipped>
>
>> +/* XXX: Still need normal assert inside writer functions. */
> Minor: Since there is no writer function, I would rather use the
> following wording:
> | Still need normal assert for sanity checks.
Fixed.
>> +#undef NDEBUG
>> +#include <assert.h>
>> +
>> +/*
>> + * Test file to demonstrate a segmentation fault, when C function
> Typo: s/C function/a C function/
Fixed.
>
>> + * is used as a VM handler in LuaJIT:
> Minor: `as a VM handler for trace events` is a little bit more verbose.
> Feel free to ignore.

Fixed.

  /*
- * Test file to demonstrate a segmentation fault, when C function
- * is used as a VM handler in LuaJIT:
+ * Test file to demonstrate a segmentation fault, when a C
+ * function is used as a VM handler for trace events in LuaJIT:
   *
   * Program received signal SIGSEGV, Segmentation fault.
   * 0x000055555557e77d in trace_abort (J=0x7ffff7f9b6b8) at lj_trace.c:615

>> + *
>> + * Program received signal SIGSEGV, Segmentation fault.
>> + * 0x000055555557e77d in trace_abort (J=0x7ffff7f9b6b8) at lj_trace.c:615
>> + * 615         lj_vmevent_send(L, TRACE,
>> + * (gdb) bt
>> + *
>> + * See details inhttps://github.com/LuaJIT/LuaJIT/issues/1087.
>> + */
>> +
> <snipped>
>
>> +	/*
>> +	 * The number 32767 is `REF_DROP - REF_BIAS`.
> It will be better to rephrase the next sentence.
>
>>                                                   See the commit
>> +	 * 0fdf06b456e6 ("test: relax JIT setup in misc.getmetrics
>> +	 * test") for the details.
>> +	 */
> I suggest the following comment.
> | This is the maximum possible IR amount, so the trace is always
> | aborted.
>
> The amount of text to read is approximately the same, but we don't need
> to look into the mysterious commit anymore. :)

Fixed.

@@ -103,9 +103,9 @@ static int handle_cont_frame(void *test_state)
         jit_attach(L, nop, "trace");

         /*
-        * The number 32767 is `REF_DROP - REF_BIAS`. See the commit
-        * 0fdf06b456e6 ("test: relax JIT setup in misc.getmetrics
-        * test") for the details.
+        * The number 32767 is `REF_DROP - REF_BIAS`. This is the
+        * maximum possible IR amount, so the trace is always
+        * aborted.
          */

>
>> +	int res = luaL_dostring(L, "jit.opt.start('minstitch=32767')");
> <snipped>
>

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

Re: [Tarantool-patches] [PATCH luajit][v2] Handle all stack layouts in (delayed) TRACE vmevent.

[Sergey Kaplun via Tarantool-patches]

Sergey,

I've applied the patch into all long-term branches in tarantool/luajit
and bumped a new version in master [1], release/3.3 [2], release/3.2 [3]
and release/2.11 [4].

[1]: https://github.com/tarantool/tarantool/pull/11128
[2]: https://github.com/tarantool/tarantool/pull/11029
[3]: https://github.com/tarantool/tarantool/pull/11030
[4]: https://github.com/tarantool/tarantool/pull/11031

-- 
Best regards,
Sergey Kaplun