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

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

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

Hi, Sergey,

thanks for review!

On 19.02.2025 11:06, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, after fixing minor nits below.
>
> On 13.02.25, Sergey Bronnikov wrote:
>> sysprof and memprof Lua API functions returns an error message
> Typo: s/returns/return/
Fixed.
>
>> "profiler misuse", when appropriate profiler is disabled in build.
> Typo: s/appropriate/the corresponding/
> Typo: s/build/the build/
Fixed.
>
>> It is not possible to easily distinquish whether it is really
> Typo: s/distinquish/distinguish/
Fixed.
>
>> misuse or profiler was not enabled in build. The patch changes
> Typo: s/profiler/or if the profiler/
> Typo: s/build/the build/
Fixed.
>
>> errors messages, so when profiler was not enabled in build message
> Typo: s/errors/error/
> Typo: s/profiler/a profiler/
> Typo: s/was/is/
> Typo: s/in build message/in the build, the message/
Fixed.
>
>> is the following: "profiler misuse: profiler is disabled".
>> ---
>>   src/lib_misc.c                                | 27 ++++++++++++++++-
>>   src/lj_errmsg.h                               |  1 +
>>   .../misclib-memprof-lapi-disabled.test.lua    | 22 ++++++++++++++
>>   .../misclib-sysprof-lapi-disabled.test.lua    | 29 +++++++++++++++++++
>>   4 files changed, 78 insertions(+), 1 deletion(-)
>>   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 d71904e4..7666d85f 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -315,10 +315,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)
>>   {
>> +  const char *err_details = NULL;
>> +#if !LJ_HASSYSPROF
> It's more LuaJIT-way to use something like the following:
> | if (!LJ_HASSYSPROF) {
> |   /* ... */
> | }
>
> This helps to avoid strange early return.
Fixed.
> Also, please be avaired to declare all variables (stataus, opt,
> err_details) in the beginning of the block.

  It is a requirement for the code that strictly follows c89, when you 
must declare all of your variables at the beginning of a scope block.

AFAIK, we have no such requirement, and also there is no option 
"-std=c89" in CI and no any mentions in the contribution guide.

>
> Here and below.

>
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASSYSPROF */
>> +
>>     int status = PROFILE_SUCCESS;
>>   
>>     struct luam_Sysprof_Options opt = {};
>> -  const char *err_details = NULL;
>>   
>>     status = parse_sysprof_opts(L, &opt, &err_details);
>>     if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>> @@ -336,6 +341,11 @@ LJLIB_CF(misc_sysprof_start)
>>   /* local res, err, errno = profile.sysprof_stop() */
>>   LJLIB_CF(misc_sysprof_stop)
>>   {
>> +#if !LJ_HASSYSPROF
>> +  const char *err_details = NULL;
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> Looks like we just can do the following for this block:
>
> | const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>
> Here and below.
Fixed.
>
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASSYSPROF */
>>     int status = luaM_sysprof_stop(L);
>>     if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>>       return sysprof_error(L, status, NULL);
>> @@ -347,6 +357,11 @@ LJLIB_CF(misc_sysprof_stop)
>>   /* local counters, err, errno = sysprof.report() */
>>   LJLIB_CF(misc_sysprof_report)
>>   {
>> +#if !LJ_HASSYSPROF
>> +  const char *err_details = NULL;
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASSYSPROF */
>>     struct luam_Sysprof_Counters counters = {};
>>     GCtab *data_tab = NULL;
>>     GCtab *count_tab = NULL;
>> @@ -386,6 +401,11 @@ 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 = NULL;
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASMEMPROF */
>>     struct lj_memprof_options opt = {0};
>>     const char *fname = strdata(lj_lib_checkstr(L, 1));
>>     struct profile_ctx *ctx;
>> @@ -440,6 +460,11 @@ LJLIB_CF(misc_memprof_start)
>>   /* local stopped, err, errno = misc.memprof.stop() */
>>   LJLIB_CF(misc_memprof_stop)
>>   {
>> +#if !LJ_HASMEMPROF
>> +  const char *err_details = NULL;
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASMEMPROF */
>>     int status = lj_memprof_stop(L);
>>     if (status != PROFILE_SUCCESS) {
>>       switch (status) {
>> 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")
> I suppose this should be declared under `#else` directive (i.e. if we
> have at least one profiler disabled).

err message is required by code for checking disabled profilers (see above)

>
>>   #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..a0669cc6
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>> @@ -0,0 +1,22 @@
>> +local tap = require("tap")
> Please use single quotes everywhere across this file for the
> consistency.

Fixed.


>> +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 memprof is disabled.
> Minor: s/start memprof when memprof/start memprof when it/
>
Fixed.
>> +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 memprof is disabled.
> Minor: s/stop memprof when memprof/stop memprof when it/
Fixed.
>
>> +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..79707434
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
>> @@ -0,0 +1,29 @@
>> +local tap = require("tap")
> Please use single quotes everywhere across this file for the
> consistency.
Fixed.
>> +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.34.1
>>

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