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

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

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