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

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

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.
---
 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"
 #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')
+        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)
 {
-  if (lua_gettop(L) != 1)
-    return PROFILE_ERRUSE;
+  if (lua_gettop(L) != 1) {
+    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")
 #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
+
 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")
+test:is(errno, nil, "errno with missed profiling mode")
+assert(misc.sysprof.stop(), "sysprof is not stopped")
+
+-- 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{
+    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