Re: [Tarantool-patches] [PATCH luajit 4/4] sysprof: fix a message with stop without run

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

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

Hi, Sergey!

thanks for valuable comments!

Sergey

On 10.02.2025 17:52, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On 04.02.25, Sergey Bronnikov wrote:
>> The function `misc.sysprof.stop()` reports that profiler is
>> already running:
>>
>> | $ ./build/src/luajit -e 'print(misc.sysprof.stop())'
> Minor: You may omit a ./build here, but this is a matter of taste, so
> feel free to ignore.
Removed 'build'.
>
>> | nil     profiler is running already     22
>>
>> The patch fixes that.
> Unfortunatelly not:
>
> | $ git --no-pager log --oneline -n1 --no-decorate && ./src/luajit -e 'print(misc.sysprof.stop())'
> | b49c5fac sysprof: fix a message with stop without run
> | nil     profiler is running already     22
>
> See my comments below.
>
> Please add the follow-up to the tarantool/tarantool#781.
Added.
>
>> ---
>>   src/lj_sysprof.c                                             | 2 +-
>>   test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 +++--
>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
>> index 88c7a41b..b76f503c 100644
>> --- a/src/lj_sysprof.c
>> +++ b/src/lj_sysprof.c
>> @@ -493,7 +493,7 @@ int lj_sysprof_stop(lua_State *L)
>>     global_State *g = sp->g;
>>     struct lj_wbuf *out = &sp->out;
>>   
>> -  if (SPS_IDLE == sp->state)
>> +  if (SPS_PROFILE != sp->state)
> This check is correct. It checks that the profiler has no error or is
> not running (i.e., that it is stopped). With the new check, a case with
> an error during profiling will be masked.
>
> It looks like we should add this case in the `misc.sysprof.stop()` (it
> uses `sysprof_error()` which handles start cases).
Fixed.
>
>>       return PROFILE_ERRRUN;
>>     else if (G(L) != g)
>>       return PROFILE_ERRUSE;
>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> index 68a4b72f..91f9ca5c 100644
>> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> @@ -10,7 +10,7 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({
>>     ["Disabled due to #10803"] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
>>   })
>>   
>> -test:plan(33)
>> +test:plan(34)
>>   
>>   jit.off()
>>   -- XXX: Run JIT tuning functions in a safe frame to avoid errors
>> @@ -98,7 +98,8 @@ assert(res, err)
>>   
>>   -- Not running.
>>   res, err, errno = misc.sysprof.stop()
>> -test:ok(res == nil and err, "res and error with not running")
>> +test:is(res, nil, "res with not running")
>> +test:ok(err:match("profiler is running already"), "error with not running")
> I suppose there is bad copy-pasting here. It should be: 'profiler is not
> running'. This is why the test passes.
Fixed.
>
>>   test:ok(type(errno) == "number", "errno with not running")
>>   
>>   -- Bad path.
>> -- 
>> 2.34.1
>>

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