[Tarantool-patches] [PATCH luajit 0/4] Fix sysprof issues

[Tarantool-patches] [PATCH luajit 0/4] Fix sysprof issues

[Sergey Bronnikov via Tarantool-patches]

The patch series fixes a number of issues related to sysprof:

- Fixes a problem with wrong error message on stop not running
  profiler.
- Make error messages more specific and descriptive.
- Introduces default profiling mode.
- Fixes descriptions in sysprof testcases.

Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-fix-sysprof-opts-processing
(aarch64 jobs are failed, it is a known issue.)

Sergey Bronnikov (4):
  test: add descriptions to sysprof testcases
  sysprof: fix typo in the comment
  sysprof: introduce specific errors and default mode
  sysprof: fix a message with stop without run

 src/lib_misc.c                                | 49 +++++++++---
 src/lj_errmsg.h                               |  4 +
 src/lj_sysprof.c                              |  2 +-
 src/lmisclib.h                                |  5 ++
 .../profilers/misclib-sysprof-lapi.test.lua   | 76 ++++++++++++++-----
 5 files changed, 105 insertions(+), 31 deletions(-)

-- 
2.34.1

[Tarantool-patches] [PATCH luajit 1/4] test: add descriptions to sysprof testcases

[Sergey Bronnikov via Tarantool-patches]

The patch add descriptions to sysprof testcases to make TAP report
more usable.
---
 .../profilers/misclib-sysprof-lapi.test.lua   | 36 ++++++++++---------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index b2c38c49..581fb7fd 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -65,34 +65,35 @@ end
 
 -- Wrong profiling mode.
 local res, err, errno = misc.sysprof.start{ mode = "A" }
-test:ok(res == nil and err:match("profiler misuse"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler misuse"), "res with no parameters")
+test:ok(type(errno) == "number", "errno with no parameters")
 
 -- Already running.
 res, err = misc.sysprof.start{ mode = "D" }
 assert(res, err)
 
 res, err, errno = misc.sysprof.start{ mode = "D" }
-test:ok(res == nil and err:match("profiler is running already"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler is running already"),
+        "ok with already running")
+test:ok(type(errno) == "number", "errno with already running")
 
 res, err = misc.sysprof.stop()
 assert(res, err)
 
 -- Not running.
 res, err, errno = misc.sysprof.stop()
-test:ok(res == nil and err)
-test:ok(type(errno) == "number")
+test:ok(res == nil and err, "res and error with not running")
+test:ok(type(errno) == "number", "errno with not running")
 
 -- Bad path.
 res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
-test:ok(res == nil and err:match("No such file or directory"))
-test:ok(type(errno) == "number")
+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.
 res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
-test:ok(res == nil and err:match("profiler misuse"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval")
+test:ok(type(errno) == "number", "errno with bad interval")
 
 -- DEFAULT MODE
 
@@ -102,20 +103,23 @@ end
 
 local report = misc.sysprof.report()
 
--- Check the profile is not empty
-test:ok(report.samples > 0)
+-- Check the profile is not empty.
+test:ok(report.samples > 0, "number of samples is greater than 0")
 -- There is a Lua function with FNEW bytecode in it. Hence there
 -- are only three possible sample types:
 -- * LFUNC -- Lua payload is sampled.
 -- * GC -- Lua GC machinery triggered in scope of FNEW bytecode
 --   is sampled.
 -- * INTERP -- VM is in a specific state when the sample is taken.
-test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0)
+test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0,
+        "total number of LFUNC, GC and INTERP samples is greater than 0")
 -- There is no fast functions and C function in default payload.
-test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0)
+test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0,
+        "total number of FFUNC and CFUNC samples is equal to 0")
 -- Check all JIT-related VM states are not sampled.
 for _, vmstate in pairs({ 'TRACE', 'RECORD', 'OPT', 'ASM', 'EXIT' }) do
-  test:ok(report.vmstate[vmstate] == 0)
+  local msg = ("total number of VM state %s is equal to 0"):format(vmstate)
+  test:ok(report.vmstate[vmstate] == 0, msg)
 end
 
 -- With very big interval.
@@ -124,7 +128,7 @@ if not pcall(generate_output, { mode = "D", interval = 1000 }) then
 end
 
 report = misc.sysprof.report()
-test:ok(report.samples == 0)
+test:ok(report.samples == 0, "total number of samples is equal to 0")
 
 -- LEAF MODE
 check_mode("L", 100)
-- 
2.34.1

[Tarantool-patches] [PATCH luajit 2/4] sysprof: fix typo in the comment

[Sergey Bronnikov via Tarantool-patches]

The patch fixes inconsistency in comment and source code below
the comment. The typo was added in the initial commit
2593a9f83072 ("misc: introduce Lua API for new sampling profiler").
---
 src/lib_misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index ca1d1c75..5b7a4b62 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -161,7 +161,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf)
 
 #define LJLIB_MODULE_misc_sysprof
 
-/* The default profiling interval equals to 11 ms. */
+/* The default profiling interval equals to 10 ms. */
 #define SYSPROF_DEFAULT_INTERVAL 10
 #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
 
-- 
2.34.1

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

[Sergey Bronnikov via Tarantool-patches]

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

[Tarantool-patches] [PATCH luajit 4/4] sysprof: fix a message with stop without run

[Sergey Bronnikov via Tarantool-patches]

The function `misc.sysprof.stop()` reports that profiler is
already running:

| $ ./build/src/luajit -e 'print(misc.sysprof.stop())'
| nil     profiler is running already     22

The patch fixes that.
---
 src/lj_sysprof.c                                             | 2 +-
 test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index 88c7a41b..b76f503c 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -493,7 +493,7 @@ int lj_sysprof_stop(lua_State *L)
   global_State *g = sp->g;
   struct lj_wbuf *out = &sp->out;
 
-  if (SPS_IDLE == sp->state)
+  if (SPS_PROFILE != sp->state)
     return PROFILE_ERRRUN;
   else if (G(L) != g)
     return PROFILE_ERRUSE;
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index 68a4b72f..91f9ca5c 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(33)
+test:plan(34)
 
 jit.off()
 -- XXX: Run JIT tuning functions in a safe frame to avoid errors
@@ -98,7 +98,8 @@ assert(res, err)
 
 -- Not running.
 res, err, errno = misc.sysprof.stop()
-test:ok(res == nil and err, "res and error with not running")
+test:is(res, nil, "res with not running")
+test:ok(err:match("profiler is running already"), "error with not running")
 test:ok(type(errno) == "number", "errno with not running")
 
 -- Bad path.
-- 
2.34.1