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

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

Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing 3 minor nits below.

On 24.02.25, Sergey Bronnikov wrote:
> 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:

<snipped>

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

There is no corresponding update on the branch.

> >

<snipped>

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

Thanks!
Please apply the following patch. It fixes all patchset-related places.

===================================================================
diff --git a/src/lib_misc.c b/src/lib_misc.c
index 94ec6564..79cfaf7b 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -199,13 +199,13 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,
     cTValue *mode_opt = lj_tab_getstr(options, lj_str_newlit(L, "mode"));
     if (mode_opt) {
       if (!tvisstr(mode_opt)) {
-        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
-        return PROFILE_ERRUSE;
+	*err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
+	return PROFILE_ERRUSE;
       }
       mode = strVdata(mode_opt);
       if (strlen(mode) == 0 || mode[1] != '\0') {
-        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
-        return PROFILE_ERRUSE;
+	*err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
+	return PROFILE_ERRUSE;
       }
     }
 
@@ -223,8 +223,8 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,
         opt->mode = LUAM_SYSPROF_CALLGRAPH;
         break;
       default:
-        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
-        return PROFILE_ERRUSE;
+	*err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
+	return PROFILE_ERRUSE;
     }
   }
 
@@ -235,8 +235,8 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,
     if (interval && tvisnumber(interval)) {
       int32_t signed_interval = numberVint(interval);
       if (signed_interval < 1) {
-        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL);
-        return PROFILE_ERRUSE;
+	*err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL);
+	return PROFILE_ERRUSE;
       }
       opt->interval = signed_interval;
     }
@@ -281,9 +281,9 @@ static int sysprof_error(lua_State *L, int status, const char *err_details)
     case PROFILE_ERRUSE:
       lua_pushnil(L);
       if (err_details)
-        lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
+	lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
       else
-        lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
+	lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
       lua_pushinteger(L, EINVAL);
       return 3;
 #if LJ_HASSYSPROF
===================================================================

>     int n = (int)(L->top - L->base);

<snipped>

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

This is kind of misleading, plus this is a dead check. It is __always__
`nil`, or we will get an error in the first test and the second test.
Since we are not testing `pcall()` implementation here, let's just drop
it (it is even cheaper :)).

<snipped>

-- 
Best regards,
Sergey Kaplun