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

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

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.

> | 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.

> ---
>  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).

>      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.

>  test:ok(type(errno) == "number", "errno with not running")
>  
>  -- Bad path.
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun