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

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

[-- 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 --]