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

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

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

Hi, Sergey!

thanks for review!

Updated and force-pushed.

Sergey

On 24.02.2025 14:28, Sergey Kaplun via Tarantool-patches wrote:
> 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.

Updated (approach b)):

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 62d0597e..d5122665 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -304,15 +304,14 @@ 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)
  {
-  const char *err_details = NULL;
+#if !LJ_HASSYSPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    int status = PROFILE_SUCCESS;

    struct luam_Sysprof_Options opt = {};
-
-  if (!LJ_HASSYSPROF) {
-    err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-    return sysprof_error(L, PROFILE_ERRUSE, err_details);
-  }
+  const char *err_details = NULL;

    status = parse_sysprof_opts(L, &opt, &err_details);
    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
@@ -325,35 +324,36 @@ LJLIB_CF(misc_sysprof_start)

    lua_pushboolean(L, 1);
    return 1;
+#endif /* !LJ_HASSYSPROF */
  }

  /* local res, err, errno = profile.sysprof_stop() */
  LJLIB_CF(misc_sysprof_stop)
  {
+#if !LJ_HASSYSPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    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);

    lua_pushboolean(L, 1);
    return 1;
+#endif /* !LJ_HASSYSPROF */
  }

  /* local counters, err, errno = sysprof.report() */
  LJLIB_CF(misc_sysprof_report)
  {
+#if !LJ_HASSYSPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    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);

@@ -379,6 +379,7 @@ LJLIB_CF(misc_sysprof_report)
    lua_setfield(L, -2, "vmstate");

    return 1;
+#endif /* !LJ_HASSYSPROF */
  }

  /* ----- misc.memprof module 
---------------------------------------------- */
@@ -388,14 +389,14 @@ LJLIB_CF(misc_sysprof_report)
  /* local started, err, errno = misc.memprof.start(fname) */
  LJLIB_CF(misc_memprof_start)
  {
+#if !LJ_HASMEMPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    struct lj_memprof_options opt = {0};
    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.
@@ -441,16 +442,17 @@ LJLIB_CF(misc_memprof_start)
    }
    lua_pushboolean(L, 1);
    return 1;
+#endif /* !LJ_HASMEMPROF */
  }

  /* local stopped, err, errno = misc.memprof.stop() */
  LJLIB_CF(misc_memprof_stop)
  {
+#if !LJ_HASMEMPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    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:
@@ -474,6 +476,7 @@ LJLIB_CF(misc_memprof_stop)
    }
    lua_pushboolean(L, 1);
    return 1;
+#endif /* !LJ_HASMEMPROF */
  }
  #endif /* !LJ_TARGET_WINDOWS */

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

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