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

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

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

Hi, Sergey,

thanks for the comments!

On 10.02.2025 17:51, Sergey Kaplun via Tarantool-patches wrote:
> 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.

Also, sysprof has options that have no meaning for memprof:

mode and interval, and sysprof accepts a table when memprof accepts

a string. It is not considered as a problem for you, but specific messages

is a problem. Seems there is an inconsistency.


> Plus, the error message is a little
> bit incompatible with the previous version (you may notice this in test
> changes).

We discussed this verbally (you, Sergos and me) and decided that it is

not a problem.

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

Honestly, it becomes more complicated in code, because error messages 
are not error messages,

but error prefixes, and now you should care about two things: error 
message and details.

> 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.
Anyway, I agree that introducing error details is a compromise in a 
current situtation.
> 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()`).
Merged.
> 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.
It is not reproduced with updated patchset.
>
>> ---
>>   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?
Fixed. (I remember you told me that would be good to set "C" as default 
mode)
>>   #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.


Fixed by checking a string len:

        }
        mode = strVdata(mode_opt);
-      if (mode[1] != '\0') {
+      if (strlen(mode) > 0 && mode[1] != '\0') {
          *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
          return PROFILE_ERRUSE;
        }
(1/1) Stage this h

>> +        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.
Removed parse_options.
>
>>   {
>> -  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.

Good advice! Thanks, fixed.

@@ -180,8 +180,11 @@ 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) {
-  if (lua_gettop(L) != 1) {
-    lua_createtable(L, 0, 0);
+  int n = (int)(L->top - L->base);
+  if (n != 1) {
+    opt->mode = LUAM_SYSPROF_DEFAULT;
+    opt->interval = SYSPROF_DEFAULT_INTERVAL;
+    goto set_path;
    }

    if (!lua_istable(L, 1)) {
@@ -241,6 +244,8 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
      }
    }

+set_path:
+
    /* Get output path. */
    if (opt->mode != LUAM_SYSPROF_DEFAULT)
    {

>
> 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.
Fixed, thanks!
>
>> +    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.

Fixed.

>
>>   #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.
these additional err codes were removed
>
>> +
>>   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/
Fixed.
>
>> +test:is(errno, nil, "errno with missed profiling mode")
> Typo: s/errno/no errno/
Fixed.
>> +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.
Removed.
>
>> +
>> +-- 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.
Fixed.
>
>> +    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
>>

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