[Tarantool-patches] [PATCH luajit 4/8][v3] 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
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.
---
 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"
 #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) {
+  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);
 
   /* Get profiling mode. */
   {
     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)) {
+        *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;
+      }
     }
 
-    mode = strVdata(mode_opt);
-    if (mode[1] != '\0')
-      return PROFILE_ERRUSE;
+    if (!mode)
+      mode = SYSPROF_DEFAULT_MODE;
 
     switch (*mode) {
       case 'D':
@@ -204,6 +221,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
         opt->mode = LUAM_SYSPROF_CALLGRAPH;
         break;
       default:
+        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
         return PROFILE_ERRUSE;
     }
   }
@@ -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)
       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));
@@ -251,29 +272,26 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
   return PROFILE_SUCCESS;
 }
 
-static int parse_options(lua_State *L, struct luam_Sysprof_Options *opt)
-{
-  if (lua_gettop(L) != 1)
-    return PROFILE_ERRUSE;
-
-  if (!lua_istable(L, 1))
-    return PROFILE_ERRUSE;
-
-  return parse_sysprof_opts(L, opt, 1);
-}
-
-static int sysprof_error(lua_State *L, int status)
+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) {
+        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) {
+        lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
+      } 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)
   int status = PROFILE_SUCCESS;
 
   struct luam_Sysprof_Options opt = {};
+  const char *err_details = NULL;
 
-  status = parse_options(L, &opt);
+  status = parse_sysprof_opts(L, &opt, &err_details);
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
-    return sysprof_error(L, status);
+    return sysprof_error(L, status, err_details);
 
   status = luaM_sysprof_start(L, &opt);
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
     /* Allocated memory will be freed in on_stop callback. */
-    return sysprof_error(L, status);
+    return sysprof_error(L, status, err_details);
 
   lua_pushboolean(L, 1);
   return 1;
@@ -310,7 +329,7 @@ LJLIB_CF(misc_sysprof_stop)
 {
   int status = luaM_sysprof_stop(L);
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
-    return sysprof_error(L, status);
+    return sysprof_error(L, status, NULL);
 
   lua_pushboolean(L, 1);
   return 1;
@@ -325,7 +344,7 @@ LJLIB_CF(misc_sysprof_report)
 
   int status = luaM_sysprof_report(&counters);
   if (status != PROFILE_SUCCESS)
-    return sysprof_error(L, status);
+    return sysprof_error(L, status, NULL);
 
   lua_createtable(L, 0, 3);
   data_tab = tabV(L->top - 1);
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")
+ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
+
 #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
@@ -10,7 +10,7 @@ local test = tap.test("misclib-sysprof-lapi"):skipcond({
   ["Disabled due to #10803"] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
 })
 
-test:plan(19)
+test:plan(39)
 
 jit.off()
 -- XXX: Run JIT tuning functions in a safe frame to avoid errors
@@ -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
 
 local function payload()
   local function fib(n)
@@ -64,11 +66,44 @@ end
 -- GENERAL
 
 -- Wrong profiling mode.
-local res, err, errno = misc.sysprof.start{ mode = "A" }
-test:ok(res == nil and err:match("profiler misuse"),
-        "result status with wrong profiling mode")
+local res, err, errno = misc.sysprof.start{ mode = BAD_MODE }
+test:ok(res == nil, "result status with wrong profiling mode")
+test:ok(err:match("profiler mode must be 'D', 'L' or 'C'"),
+        "error with wrong profiling mode")
 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())
+
+-- Not a table.
+res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")
+print(res, err, errno)
+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.
+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")
+
 -- Already running.
 res, err = misc.sysprof.start{ mode = "D" }
 assert(res, err)
@@ -93,11 +128,30 @@ test:ok(res == nil and err: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 and err:match("profiler misuse"),
-        "result status and error with bad interval")
+res, err, errno = misc.sysprof.start{ mode = "C", interval = BAD_INTERVAL }
+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