Re: [Tarantool-patches] [PATCH luajit 7/8][v3] misc: specific message for disabled profilers

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

Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing my comments below.

On 20.02.25, Sergey Bronnikov wrote:
> sysprof and memprof Lua API functions return an error message
> "profiler misuse", when the corresponding profiler is disabled in
> the build. It is not possible to easily distinguish whether it is
> really misuse or if the profiler was not enabled in the build. The
> patch changes error messages, so when profiler is not enabled in
> the build, the message is the following: "profiler misuse:
> profiler is disabled".
> ---
>  src/lib_misc.c                                | 25 ++++++++++++++--
>  src/lj_errmsg.h                               |  1 +
>  .../misclib-memprof-lapi-disabled.test.lua    | 22 ++++++++++++++
>  .../misclib-sysprof-lapi-disabled.test.lua    | 29 +++++++++++++++++++
>  4 files changed, 75 insertions(+), 2 deletions(-)
>  create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>  create mode 100644 test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
> 
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index 1fd06dd1..d98cf3f0 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -306,10 +306,15 @@ static int sysprof_error(lua_State *L, int status, const char *err_details)
>  /* local res, err, errno = sysprof.start(options) */
>  LJLIB_CF(misc_sysprof_start)
>  {

As we discussed ofline we may use 2 approaches here:

1) Use branching:
a) Main branch first.
| {
|    if (LJ_HASSYSPROF) {
|      /* ... */
|    } else {
|      const char *err_details = //
|      /* ... */
|    }
| }

b) Early return first
| {
|    if (!LJ_HASSYSPROF) {
|      const char *err_details = //
|      /* ... */
|    } else {
|      /* ... */
|    }
| }

2) Use macros directives instead:
a) Main branch first.
| {
| #if LJ_HASSYSPROF
|      /* ... */
| #else
|      /* ... */
|    }
| #endif

b) Early return first
| {
| #if !LJ_HASSYSPROF
|      /* ... */
| #else
|      /* ... */
|    }
| #endif

We decided to use the second approach to avoid huge diff changes and
make code more readable.
It's up to you to use an approach a) or b).
This helps us to avoid warnings with `-Wdeclaration-after-statement`
enabled.

Here and below.

Also, it helps to avoid an excess call to `lj_{sysprof,memprof}_stop()`.

> +  const char *err_details = NULL;
>    int status = PROFILE_SUCCESS;
>  
>    struct luam_Sysprof_Options opt = {};
> -  const char *err_details = NULL;
> +
> +  if (!LJ_HASSYSPROF) {
> +    err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
>  
>    status = parse_sysprof_opts(L, &opt, &err_details);
>    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
> @@ -328,6 +333,10 @@ LJLIB_CF(misc_sysprof_start)
>  LJLIB_CF(misc_sysprof_stop)
>  {
>    int status = luaM_sysprof_stop(L);
> +  if (!LJ_HASSYSPROF) {
> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
>    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>      return sysprof_error(L, status, NULL);
>  
> @@ -341,8 +350,12 @@ LJLIB_CF(misc_sysprof_report)
>    struct luam_Sysprof_Counters counters = {};
>    GCtab *data_tab = NULL;
>    GCtab *count_tab = NULL;
> -
>    int status = luaM_sysprof_report(&counters);
> +  if (!LJ_HASSYSPROF) {
> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
> +
>    if (status != PROFILE_SUCCESS)
>      return sysprof_error(L, status, NULL);
>  
> @@ -381,6 +394,10 @@ LJLIB_CF(misc_memprof_start)
>    const char *fname = strdata(lj_lib_checkstr(L, 1));
>    struct profile_ctx *ctx;
>    int memprof_status;
> +  if (!LJ_HASMEMPROF) {
> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
>  
>    /*
>    ** FIXME: more elegant solution with ctx.
> @@ -432,6 +449,10 @@ LJLIB_CF(misc_memprof_start)
>  LJLIB_CF(misc_memprof_stop)
>  {
>    int status = lj_memprof_stop(L);
> +  if (!LJ_HASMEMPROF) {
> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
>    if (status != PROFILE_SUCCESS) {
>      switch (status) {
>      case PROFILE_ERRUSE:
> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h
> index b5c3a275..e26f5e38 100644
> --- a/src/lj_errmsg.h
> +++ b/src/lj_errmsg.h
> @@ -193,6 +193,7 @@ ERRDEF(PROF_DETAILS_BADINTERVAL, "profiler interval must be greater than 1")
>  ERRDEF(PROF_DETAILS_BADPATH, "profiler path does not exist")
>  ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
>  
> +ERRDEF(PROF_DETAILS_DISABLED, "profiler is disabled")
>  #undef ERRDEF
>  
>  /* Detecting unused error messages:
> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
> new file mode 100644
> index 00000000..de0aa136
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
> @@ -0,0 +1,22 @@
> +local tap = require('tap')
> +local test = tap.test('misclib-memprof-lapi-disabled'):skipcond({
> +  ['Memprof is enabled'] = not os.getenv('LUAJIT_DISABLE_MEMPROF'),
> +})
> +
> +test:plan(6)
> +
> +-- Attempt to start memprof when it is disabled.
> +local res, err, errno = misc.memprof.start()
> +test:is(res, nil, 'result status on start when memprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on start when memprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
> +
> +-- Attempt to stop memprof when it is disabled.
> +res, err, errno = misc.memprof.stop()
> +test:is(res, nil, 'result status on stop when memprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on stop when memprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
> new file mode 100644
> index 00000000..2a9ce796
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
> @@ -0,0 +1,29 @@
> +local tap = require('tap')
> +local test = tap.test('misclib-sysprof-lapi-disabled'):skipcond({
> +  ['Sysprof is enabled'] = not os.getenv('LUAJIT_DISABLE_SYSPROF'),
> +})
> +
> +test:plan(9)
> +
> +-- Attempt to start sysprof when sysprof is disabled.
> +local res, err, errno = misc.sysprof.start()
> +test:is(res, nil, 'result status on start when sysprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on start when sysprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
> +
> +-- Attempt to stop sysprof when sysprof is disabled.
> +res, err, errno = misc.sysprof.stop()
> +test:is(res, nil, 'result status on stop when sysprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on stop when sysprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
> +
> +-- Attempt to report when sysprof is disabled.
> +res, err, errno = misc.sysprof.report()
> +test:is(res, nil, 'result status on report when sysprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on stop when sysprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
> +
> +test:done(true)
> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun