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

[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:
> sysprof has a number of options and with any incorrect option it
> returns `false` and error message "profiler misuse". This message
> does not descriptive for sysprof users and make using sysprof
> more complicated. The patch splits error `PROFILE_ERRUSE` to four
> specific errors: `PROFILE_ERRBADMODE`, `PROFILE_ERRBADINTERVAL`,
> `PROFILE_ERRBADPATH` and `PROFILE_ERRBADTABLE`, and use default
> profiling mode ("C", callgraph) if it was not passed.

I don't like this approach:
Now we have profiler-specific statuses (valid only for sysprof) in the
public API. They have no meaning for memprof. Also, that makes the
definition of errors more twisted. Plus, the error message is a little
bit incompatible with the previous version (you may notice this in test
changes).

I suggest the following approach:
We append all additional details in the error message, so it looks
like:
| profiler misuse: sysprof mode must be 'D', 'L' or 'C'

The implementation is also simple. We don't need any additional statuses
for error messages, just to return in the `char **` pointer to string as
additional details.

So the usage is the following:
`parse_sysprof_opts(L, &opts, &err_details);`
`sysprof_error(L, &opts, err_details); /*details may be NULL here */`

Pros:
* No custom statuses for sysprof in the public API (or reserved for
  internal usage).
* More backward compatible with previous errors -- error message
  still matches with 'profiler misuse'.
* No additional custom error definitions.
* More consistent behaviour between Lua and C API -- as we operate only
  `PROFILER_MISUSE` status everywhere.
* It simplifies error handling in `sysprof_error()` -- we just need to
  check if the given argument is NULL.

Cons:
* Additional argument to the internal functions `sysprof_error()`,
  `parse_sysprof_opts()`.

Side note: I believe, since we have no valuable logic in the
`parse_opts()`, its body can be merged in the `parse_sysprof_opts()`).
See my comment below.

Friendly reminder: Also, I got the following error when building LuaJIT
with sysprof disabled (we discussed it online):

| src/luajit -e 'misc.sysprof.start()'
| LuaJIT ASSERT /home/burii/reviews/luajit/sysprof-fixes/src/lj_state.c:214: close_state: memory leak of 8388624 bytes
| Aborted (core dumped)

It seems that this is an old problem unrelated to your patchset. This
happens for a non-default mode if we use the build with disabled
sysprof. Looks like we still allocated the write buffer for sysprof for
some reason.

> ---
>  src/lib_misc.c                                | 47 ++++++++++++++-----
>  src/lj_errmsg.h                               |  4 ++
>  src/lmisclib.h                                |  5 ++
>  .../profilers/misclib-sysprof-lapi.test.lua   | 45 ++++++++++++++++--
>  4 files changed, 85 insertions(+), 16 deletions(-)
> 
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index 5b7a4b62..a28e4a3c 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -163,6 +163,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 "C"

Why is the default mode C and not D?

>  #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
>  
>  static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {
> @@ -185,13 +186,16 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>      const char *mode = NULL;
>  
>      cTValue *mode_opt = lj_tab_getstr(options, lj_str_newlit(L, "mode"));
> -    if (!mode_opt || !tvisstr(mode_opt)) {
> -      return PROFILE_ERRUSE;
> +    if (mode_opt) {
> +      if (!tvisstr(mode_opt))
> +        return PROFILE_ERRBADMODE;
> +      mode = strVdata(mode_opt);
> +      if (mode[1] != '\0')

If we have `mode = ''`, this will read the byte from `g->hookmask`. The
result is always `PROFILER_ERRBADMODE` (in the current terms), but it is
still dirty reads.

> +        return PROFILE_ERRBADMODE;
>      }
>  
> -    mode = strVdata(mode_opt);
> -    if (mode[1] != '\0')
> -      return PROFILE_ERRUSE;
> +    if (!mode)
> +      mode = SYSPROF_DEFAULT_MODE;
>  
>      switch (*mode) {
>        case 'D':
> @@ -204,7 +208,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>          opt->mode = LUAM_SYSPROF_CALLGRAPH;
>          break;
>        default:
> -        return PROFILE_ERRUSE;
> +        return PROFILE_ERRBADMODE;
>      }
>    }
>  
> @@ -215,7 +219,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>      if (interval && tvisnumber(interval)) {
>        int32_t signed_interval = numberVint(interval);
>        if (signed_interval < 1)
> -        return PROFILE_ERRUSE;
> +        return PROFILE_ERRBADINTERVAL;
>        opt->interval = signed_interval;
>      }
>    }
> @@ -231,7 +235,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>      if (!pathtv)
>        path = SYSPROF_DEFAULT_OUTPUT;
>      else if (!tvisstr(pathtv))
> -      return PROFILE_ERRUSE;
> +      return PROFILE_ERRBADPATH;
>      else
>        path = strVdata(pathtv);
>  
> @@ -253,11 +257,12 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>  
>  static int parse_options(lua_State *L, struct luam_Sysprof_Options *opt)

I suppose this helper function isn't needed. It is not related to
memprof, so its name is misleading. Its body may be merged with
`parse_sysprof_options()` to avoid these problems.

>  {
> -  if (lua_gettop(L) != 1)
> -    return PROFILE_ERRUSE;
> +  if (lua_gettop(L) != 1) {

If there are 2+ arguments with different content, we still create a
table. I suppose there is no need for table creation. We may just set
the default options at once without additional processing and creation
of the table.

Minor: Also, maybe it is better to use `(int)(L->top - L->base)` instead
of the call to the `lua_gettop()` for consistency with the internal
LuaJIT code base. I know that this function does the same thing, but
since it is the public API, this call isn't inlined.

> +    lua_createtable(L, 0, 0);
> +  }
>  
>    if (!lua_istable(L, 1))
> -    return PROFILE_ERRUSE;
> +    return PROFILE_ERRBADTABLE;
>  
>    return parse_sysprof_opts(L, opt, 1);
>  }
> @@ -270,6 +275,26 @@ static int sysprof_error(lua_State *L, int status)
>        lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
>        lua_pushinteger(L, EINVAL);
>        return 3;
> +    case PROFILE_ERRBADMODE:
> +      lua_pushnil(L);
> +      lua_pushstring(L, err2msg(LJ_ERR_PROF_BADMODE));
> +      lua_pushinteger(L, EINVAL);
> +      return 3;
> +    case PROFILE_ERRBADINTERVAL:
> +      lua_pushnil(L);
> +      lua_pushstring(L, err2msg(LJ_ERR_PROF_BADINTERVAL));
> +      lua_pushinteger(L, EINVAL);
> +      return 3;
> +    case PROFILE_ERRBADPATH:
> +      lua_pushnil(L);
> +      lua_pushstring(L, err2msg(LJ_ERR_PROF_BADPATH));
> +      lua_pushinteger(L, EINVAL);
> +      return 3;
> +    case PROFILE_ERRBADTABLE:
> +      lua_pushnil(L);
> +      lua_pushstring(L, err2msg(LJ_ERR_PROF_BADTABLE));
> +      lua_pushinteger(L, EINVAL);
> +      return 3;
>  #if LJ_HASSYSPROF
>      case PROFILE_ERRRUN:
>        lua_pushnil(L);
> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h
> index 19c41f0b..9713d6c7 100644
> --- a/src/lj_errmsg.h
> +++ b/src/lj_errmsg.h
> @@ -183,6 +183,10 @@ ERRDEF(FFI_NYICALL,	"NYI: cannot call this C function (yet)")
>  
>  /* Profiler errors. */
>  ERRDEF(PROF_MISUSE,	"profiler misuse")
> +ERRDEF(PROF_BADMODE, "profiler mode must be 'D', 'L' or 'C'")
> +ERRDEF(PROF_BADINTERVAL, "profiler interval must be greater than 1")
> +ERRDEF(PROF_BADPATH, "profiler path does not exist")
> +ERRDEF(PROF_BADTABLE, "profiler expects a table with parameters")

The errors are sysprof-specific, so they should be defined under the
corresponding ifdef. The comment is irrelevant if you consider my
suggestion above.

>  #if LJ_HASMEMPROF || LJ_HASSYSPROF
>  ERRDEF(PROF_ISRUNNING,	"profiler is running already")
>  ERRDEF(PROF_NOTRUNNING,	"profiler is not running")
> diff --git a/src/lmisclib.h b/src/lmisclib.h
> index 9084319c..b4eb0b57 100644
> --- a/src/lmisclib.h
> +++ b/src/lmisclib.h
> @@ -142,6 +142,11 @@ struct luam_Sysprof_Options {
>  #define PROFILE_ERRMEM  3
>  #define PROFILE_ERRIO   4
>  
> +#define PROFILE_ERRBADMODE   5
> +#define PROFILE_ERRBADINTERVAL 6
> +#define PROFILE_ERRBADPATH 7
> +#define PROFILE_ERRBADTABLE 8

The errors are sysprof-specific, so they should be defined under the
corresponding ifdef. Also, they are blocking the future introduction of
profiler-general errors. The comment is irrelevant if you consider my
suggestion above.

> +
>  LUAMISC_API int luaM_sysprof_set_writer(luam_Sysprof_writer writer);
>  
>  LUAMISC_API int luaM_sysprof_set_on_stop(luam_Sysprof_on_stop on_stop);
> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> index 581fb7fd..68a4b72f 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(19)
> +test:plan(33)
>  
>  jit.off()
>  -- XXX: Run JIT tuning functions in a safe frame to avoid errors
> @@ -65,9 +65,25 @@ end
>  
>  -- Wrong profiling mode.
>  local res, err, errno = misc.sysprof.start{ mode = "A" }
> -test:ok(res == nil and err:match("profiler misuse"), "res with no parameters")
> +test:ok(res == nil, "res with no parameters")
> +test:ok(err:match("profiler mode must be 'D', 'L' or 'C'"),
> +        "error with no parameters")
>  test:ok(type(errno) == "number", "errno with no parameters")
>  
> +-- Missed profiling mode.
> +res, err, errno = misc.sysprof.start{}
> +test:is(res, true, "res with missed profiling mode")
> +test:is(err, nil, "error with missed profiling mode")

Typo: s/error/no error/

> +test:is(errno, nil, "errno with missed profiling mode")

Typo: s/errno/no errno/

> +assert(misc.sysprof.stop(), "sysprof is not stopped")

I would rather drop the "sysprof is not stopped" -- in that case we will
see the reason _why_ it is not stopped successfully instead.

> +
> +-- Not a table.
> +res, err, errno = misc.sysprof.start("NOT A TABLE")
> +test:ok(res == nil, "res with not a table")
> +test:ok(err:match("profiler expects a table with parameters"),
> +        "error with not a table")
> +test:ok(type(errno) == "number", "errno with not a table")
> +
>  -- Already running.
>  res, err = misc.sysprof.start{ mode = "D" }
>  assert(res, err)
> @@ -90,10 +106,29 @@ res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
>  test:ok(res == nil and err:match("No such file or directory"), "res and error with bad path")
>  test:ok(type(errno) == "number", "errno with bad path")
>  
> --- Bad interval.
> +-- Bad interval (-1).
>  res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
> -test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval")
> -test:ok(type(errno) == "number", "errno with bad interval")
> +test:ok(res == nil, "res with bad interval -1")
> +test:ok(err:match("profiler interval must be greater than 1"),
> +        "error with bad interval -1")
> +test:ok(type(errno) == "number", "errno with bad interval -1")
> +
> +-- 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{

Here we created a file and don't delete it after the test ends. Maybe it
is better to use '/dev/null', since we don't care about the content of
the file.

> +    mode = "C",
> +    interval = 1,
> +}
> +test:is(res, true, "res with good interval 1")
> +test:is(err, nil, "error with good interval 1")
> +test:is(errno, nil, "errno with good interval 1")
> +misc.sysprof.stop()
>  
>  -- DEFAULT MODE
>  
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun