Re: [Tarantool-patches] [PATCH luajit 4/8][v3] sysprof: introduce specific errors and default mode

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

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

Hi, Sergey,

changes applied and force-pushed.

Sergey

On 24.02.2025 15:46, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, after the final polishing, please consider my comments below.
>
> On 20.02.25, Sergey Bronnikov wrote:
>> sysprof has a number of options and with any incorrect option it
>> returns `false` and error message "profiler misuse". This message
>> discourages sysprof users and makes using sysprof more
>> complicated. The patch sets the default profiling mode ("D", that
>> shows only virtual machine state counters) if it was not passed
>> and adds details to the error message with possible reasons of
>> misuse.
> Please add the following notes.
>
> | Also, it fixes the bug when the error isn't raised if the given argument
> | type is incorrect.
> | Also, the profiler can start if it has extra arguments given.
> | They are just ignored now.
Added.
>
>> ---
>>   src/lib_misc.c                                | 77 ++++++++++++-------
>>   src/lj_errmsg.h                               |  5 ++
>>   .../profilers/misclib-sysprof-lapi.test.lua   | 68 ++++++++++++++--
>>   3 files changed, 114 insertions(+), 36 deletions(-)
>>
>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>> index 5b7a4b62..1fd06dd1 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -14,6 +14,7 @@
>>   
>>   #include "lj_obj.h"
>>   #include "lj_str.h"
>> +#include "lj_strfmt.h"
> Please add the corresponding include (after lj_str.h) in <src/Makefile.dep.original> to
> avoid conflicts when we use this file (for crossbuilds or whatever).
>
>>   #include "lj_tab.h"
>>   #include "lj_lib.h"
>>   #include "lj_gc.h" @@ -163,6 +164,7 @@ static int on_stop_cb_default(void *opt, 
>> uint8_t *buf) /* The default profiling interval equals to 10 ms. */ 
>> #define SYSPROF_DEFAULT_INTERVAL 10 +#define SYSPROF_DEFAULT_MODE "D"
>>   #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
>>   
>>   static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {
>> @@ -177,21 +179,36 @@ static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {
>>     return PROFILE_SUCCESS;
>>   }
>>   
>> -static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, int idx) {
>> -  GCtab *options = lj_lib_checktab(L, idx);
>> +static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,
>> +	                      const char **err_details) {
> Nit: Please, replace every 8 spaces with one tab.

Fixed:

--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -180,7 +180,7 @@ static int set_output_path(const char *path, struct 
luam_Sysprof_Options *opt) {
  }

  static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
-                             const char **err_details) {
+                             const char **err_details) {
    int n = (int)(L->top - L->base);
    if (n == 0) {
      opt->mode = LUAM_SYSPROF_DEFAULT;

>
>> +  int n = (int)(L->top - L->base);
>> +  if (n == 0) {
>> +    opt->mode = LUAM_SYSPROF_DEFAULT;
>> +    opt->interval = SYSPROF_DEFAULT_INTERVAL;
>> +    return PROFILE_SUCCESS;
>> +  }
>> +
>> +  GCtab *options = lj_lib_checktab(L, 1);
> Please, declare (not initialize) `options` before `if` branch to avoid
> -Wdeclaration-after-statement warnings.

Fixed:

    int n = (int)(L->top - L->base);
+  GCtab *options;
    if (n == 0) {
      opt->mode = LUAM_SYSPROF_DEFAULT;
      opt->interval = SYSPROF_DEFAULT_INTERVAL;
      return PROFILE_SUCCESS;
    }

-  GCtab *options = lj_lib_checktab(L, 1);
+  options = lj_lib_checktab(L, 1);

    /* Get profiling mode. */
    {

>
> Please, also add the comment that we ignore all other arguments given to
> this function.

Added:

    if (n == 0) {
      opt->mode = LUAM_SYSPROF_DEFAULT;
      opt->interval = SYSPROF_DEFAULT_INTERVAL;
      return PROFILE_SUCCESS;
    }

-  GCtab *options = lj_lib_checktab(L, 1);
+  /* All other arguments given to this function are ignored. */
+  options = lj_lib_checktab(L, 1);

    /* Get profiling mode. */
    {

>
>>   
>>     /* Get profiling mode. */
>>     {
> <snipped>
>
>>     }
>> @@ -214,8 +232,10 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>>       opt->interval = SYSPROF_DEFAULT_INTERVAL;
>>       if (interval && tvisnumber(interval)) {
>>         int32_t signed_interval = numberVint(interval);
>> -      if (signed_interval < 1)
>> +      if (signed_interval < 1) {
>> +        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL);
>>           return PROFILE_ERRUSE;
>> +      }
>>         opt->interval = signed_interval;
>>       }
>>     }
>> @@ -230,9 +250,10 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>>       cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path"));
>>       if (!pathtv)
> Please, add the `{` braces here too.

Added:

@@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
      int status = 0;

      cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path"));
-    if (!pathtv)
+    if (!pathtv) {
        path = SYSPROF_DEFAULT_OUTPUT;
-    else if (!tvisstr(pathtv)) {
+    } else if (!tvisstr(pathtv)) {
        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);
        return PROFILE_ERRUSE;
-    } else
+    } else {
        path = strVdata(pathtv);
+    }

      ctx = lj_mem_new(L, sizeof(*ctx));
      ctx->g = G(L);

>
>>         path = SYSPROF_DEFAULT_OUTPUT;
>> -    else if (!tvisstr(pathtv))
>> +    else if (!tvisstr(pathtv)) {
>> +      *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);
>>         return PROFILE_ERRUSE;
>> -    else
>> +    } else
> Ditto.
@@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
      int status = 0;

      cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path"));
-    if (!pathtv)
+    if (!pathtv) {
        path = SYSPROF_DEFAULT_OUTPUT;
-    else if (!tvisstr(pathtv)) {
+    } else if (!tvisstr(pathtv)) {
        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);
        return PROFILE_ERRUSE;
-    } else
+    } else {
        path = strVdata(pathtv);
+    }

      ctx = lj_mem_new(L, sizeof(*ctx));
      ctx->g = G(L);
>
>>         path = strVdata(pathtv);
>>   
>>       ctx = lj_mem_new(L, sizeof(*ctx));
>> @@ -251,29 +272,26 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>>     return PROFILE_SUCCESS;
>>   }
> <snipped>
>
>> +static int sysprof_error(lua_State *L, int status, const char *err_details)
>>   {
>>     switch (status) {
>>       case PROFILE_ERRUSE:
>>         lua_pushnil(L);
>> -      lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
>> +      if (err_details) {
> Nit: OTOH, there is no need for them here and below.
> Feel free to ignore.

Removed:

@@ -277,21 +280,16 @@ static int sysprof_error(lua_State *L, int status, 
const char *err_details)
    switch (status) {
      case PROFILE_ERRUSE:
        lua_pushnil(L);
-      if (err_details) {
+      if (err_details)
          lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), 
err_details);
-      } else {
+      else
          lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
-      }
        lua_pushinteger(L, EINVAL);
        return 3;

>> +        lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
>> +      } else {
>> +        lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
>> +      }
>>         lua_pushinteger(L, EINVAL);
>>         return 3;
>>   #if LJ_HASSYSPROF
>>       case PROFILE_ERRRUN:
>>         lua_pushnil(L);
>> -      lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
>> +      if (err_details) {
> Side note: `err_deails` is always `NULL` here. I suppose we don't need
> this branch for now (to avoid the dead code), so these changes may be
> avoided.

Removed:

  #if LJ_HASSYSPROF
      case PROFILE_ERRRUN:
        lua_pushnil(L);
-      if (err_details) {
-        lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), 
err_details);
-      } else {
-        lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
-      }
+      lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
        lua_pushinteger(L, EINVAL);
        return 3;
      case PROFILE_ERRIO:

>> +        lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
> Typo: s/LJ_ERR_PROF_MISUSE/LJ_ERR_PROF_ISRUNNING/
> Irrelevant, after fixing the comment above.
Yep, irrelevant now.
>
>> +      } else {
>> +        lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
>> +      }
>>         lua_pushinteger(L, EINVAL);
>>         return 3;
>>       case PROFILE_ERRIO:
>> @@ -291,15 +309,16 @@ LJLIB_CF(misc_sysprof_start)
> <snipped>
>
>> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h
>> index 19c41f0b..b5c3a275 100644
>> --- a/src/lj_errmsg.h
>> +++ b/src/lj_errmsg.h
>> @@ -188,6 +188,11 @@ ERRDEF(PROF_ISRUNNING,	"profiler is running already")
>>   ERRDEF(PROF_NOTRUNNING,	"profiler is not running")
>>   #endif
>>   
>> +ERRDEF(PROF_DETAILS_BADMODE, "profiler mode must be 'D', 'L' or 'C'")
>> +ERRDEF(PROF_DETAILS_BADINTERVAL, "profiler interval must be greater than 1")
>> +ERRDEF(PROF_DETAILS_BADPATH, "profiler path does not exist")
> This should be something like the following:
> | "profiler path should be a string"

Fixed:

--- a/src/lj_errmsg.h
+++ b/src/lj_errmsg.h
@@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING,     "profiler is not running")

  ERRDEF(PROF_DETAILS_BADMODE, "profiler mode must be 'D', 'L' or 'C'")
  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_BADPATH, "profiler path should be a string")

  #undef ERRDEF

>
>> +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
> This errmsg is unused now and can be deleted.

Thanks, fixed:

--- a/src/lj_errmsg.h
+++ b/src/lj_errmsg.h
@@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING,     "profiler is not running")

  ERRDEF(PROF_DETAILS_BADMODE, "profiler mode must be 'D', 'L' or 'C'")
  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_BADPATH, "profiler path should be a string")

  #undef ERRDEF

@@ -127,6 +127,13 @@ test:ok(res == nil and err:match("No such file or 
directory"),
          "result status and error with bad path")
test:ok(type(errno) == "number", "errno with bad path")

+-- Path is not a string.
+res, err, errno = misc.sysprof.start({ mode = 'C', path = 190 })
+test:ok(res == nil, "result status with path is not a string")
+test:ok(err:match("profiler path should be a string"),
+        "err with path is not a string")
+test:ok(type(errno) == "number", "errno with path is not a string")
+
  -- Bad interval.
  res, err, errno = misc.sysprof.start{ mode = "C", interval = 
BAD_INTERVAL }
test:is(res, nil, "result status and error with bad interval")

>
>> +
>>   #undef ERRDEF
>>   
>>   /* Detecting unused error messages:
>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> index 32fa384c..9915f565 100644
>> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> <snipped>
>
>> @@ -24,6 +24,8 @@ local profilename = require("utils").tools.profilename
>>   
>>   local TMP_BINFILE = profilename("sysprofdata.tmp.bin")
>>   local BAD_PATH = profilename("sysprofdata/tmp.bin")
>> +local BAD_MODE = "A"
>> +local BAD_INTERVAL = -1
> Why do we need this definition for only one type of bad interval?
> Let's consistently declare both of them like:
> | local BAD_INTERVAL_M1 = -1
> | local BAD_INTERVAL_0  = 0
> Or don't declare them at all.
>
> I'm OK with both variants.

BAD_INTERVAL is used in two testcases therefore it is declared as constant.

>>   
>>   local function payload()
>>     local function fib(n)
>> @@ -64,11 +66,44 @@ end
> <snipped>
>
>>   test:ok(type(errno) == "number", "errno with wrong profiling mode")
>>   
>> +-- Missed profiling mode.
>> +res, err, errno = misc.sysprof.start{}
>> +test:is(res, true, "res with missed profiling mode")
>> +test:is(err, nil, "no error with missed profiling mode")
>> +test:is(errno, nil, "no errno with missed profiling mode")
>> +assert(misc.sysprof.stop())
> Minor: It would be nice to check that there is no default file created,
> i.e., that we actually run the default profiler mode without stack
> inspection. Feel free to ignore, through.

Added:

@@ -77,6 +79,9 @@ res, err, errno = misc.sysprof.start{}
test:is(res, true, "res with missed profiling mode")
test:is(err, nil, "no error with missed profiling mode")
test:is(errno, nil, "no errno with missed profiling mode")
+local ok, err_msg = pcall(read_file, SYSPROF_DEFAULT_OUTPUT_FILE)
+test:ok(ok == false and err_msg:match("cannot open a file"),
+        "default output file is empty")
  assert(misc.sysprof.stop())

  -- Not a table.

>> +
>> +-- Not a table.
>> +res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")
>> +print(res, err, errno)
> Excess debug print.
Removed, thanks!
>
>> +test:is(res, false, "res with not a table")
>> +test:ok(err:match("table expected, got string"),
>> +        "error with not a table")
>> +test:is(errno, nil, "errno with not a table")
> There is no need to check or declare the `errno` variable. It's always
> `nil` for the case when the error is raised.

The check is cheap, why not?


>
>> +
>> +-- All parameters are invalid.
>> +res, err, errno = misc.sysprof.start({
>> +  mode = BAD_MODE, path = BAD_PATH, interval = BAD_INTERVAL })
>> +test:ok(res == nil, "res with all invalid parameters")
>> +test:ok(err:match("profiler misuse: profiler mode must be 'D', 'L' or 'C'"),
>> +        "error with all invalid parameters")
>> +test:ok(type(errno) == "number", "errno with all invalid parameters")
>> +
>> +-- All parameters are invalid, except the first one.
>> +res, err, errno = misc.sysprof.start({
>> +  mode = "C", path = BAD_PATH, interval = BAD_INTERVAL })
>> +test:ok(res == nil, "res with all invalid parameters except the first one")
>> +test:ok(err:match("profiler misuse: profiler interval must be greater than 1"),
>> +        "error with all invalid parameters except the first one")
>> +test:ok(type(errno) == "number",
>> +        "errno with all invalid parameters except the first one")
> This is not exactly what I mean by invalid/partially invalid parameters
> (sorry, for the bad wording):
> I mean to add tests that cover the following calls:
> | pcall(misc.sysprof.start, '', '', '') -- shouldn't started -- error
> | -- (expected table got nil)
> | misc.sysprof.start({}, '', '') -- should started with the default options
>
Verbally decided to left these testcases with a comment:

test:is(res, false, "res with not a table")
test:ok(err:match("table expected, got string"),
          "error with not a table")
test:is(errno, nil, "errno with not a table")

--- All parameters are invalid.
+-- All parameters are invalid. Check parameter validation order
+-- (not strict documented behaviour).
  res, err, errno = misc.sysprof.start({
    mode = BAD_MODE, path = BAD_PATH, interval = BAD_INTERVAL })
test:ok(res == nil, "res with all invalid parameters")
@@ -95,7 +100,8 @@ test:ok(err:match("profiler misuse: profiler mode 
must be 'D', 'L' or 'C'"),
          "error with all invalid parameters")
test:ok(type(errno) == "number", "errno with all invalid parameters")

--- All parameters are invalid, except the first one.
+-- All parameters are invalid, except the first one. Check
+-- parameter validation order (not strict documented behaviour).
  res, err, errno = misc.sysprof.start({
    mode = "C", path = BAD_PATH, interval = BAD_INTERVAL })
test:ok(res == nil, "res with all invalid parameters except the first one")


>> +
>>   -- Already running.
>>   res, err = misc.sysprof.start{ mode = "D" }
>>   assert(res, err)
>> @@ -93,11 +128,30 @@test:ok(res == nil anderr:match("No such file or directory"),
>>   test:ok(type(errno) == "number", "errno with bad path")
>>   
>>   -- Bad interval.
>> -res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
>> -test:ok(res == nil anderr:match("profiler misuse"),
>> -        "result status and error with bad interval")
>> +res, err, errno = misc.sysprof.start{ mode = "C", interval = BAD_INTERVAL }
> Side note: See my comments about this constant above.
decided to left as is
>
>> +test:is(res, nil, "result status and error with bad interval")
>> +test:ok(err:match("profiler interval must be greater than 1"),
>> +        "error with bad interval")
>>   test:ok(type(errno) == "number", "errno with bad interval")
>>   
>> +-- Bad interval (0).
>> +res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 }
>> +test:ok(res == nil, "res with bad interval 0")
>> +test:ok(err:match("profiler interval must be greater than 1"),
>> +        "error with bad interval 0")
>> +test:ok(type(errno) == "number", "errno with bad interval 0")
>> +
>> +-- Good interval (1).
>> +res, err, errno = misc.sysprof.start{
>> +    mode = "C",
>> +    interval = 1,
>> +    path = "/dev/null",
>> +}
>> +test:is(res, true, "res with good interval 1")
>> +test:is(err, nil, "no error with good interval 1")
>> +test:is(errno, nil, "no errno with good interval 1")
>> +misc.sysprof.stop()
>> +
>>   -- DEFAULT MODE
>>   
>>   if not pcall(generate_output, { mode = "D", interval = 100 }) then
>> -- 
>> 2.43.0
>>

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