[Tarantool-patches] [PATCH luajit 0/8][v3] Fix profilers issues

[Tarantool-patches] [PATCH luajit 0/8][v3] Fix profilers issues

[Sergey Bronnikov via Tarantool-patches]

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

- Make an error messages more specific and descriptive.
- Introduces default profiling mode in sysprof and default output
  file in memprof.
- Added fixes of descriptions in sysprof testcases.
- Set default path to memprof output file.
- Add an specific error message for disabled profilers.

Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-fix-sysprof-opts-processing

Changes v3:

- Added fixes according to comments by Sergey Kaplun.

Changes v2:

- The patch that fixes a problem with wrong error message on stop
  not running profiler has been moved to a separate patch series.
- Added patch that align test title with sysprof test filename.
- Added patch that set default path to memprof output file.
- Added patch that adds a workflow with disabled profilers.
- Added patch that changes a error message for disabled profilers.
- Added fixes according to comments by Sergey Kaplun.

Sergey Bronnikov (8):
  test: add descriptions to sysprof testcases
  test: align test title with test filename
  sysprof: fix typo in the comment
  sysprof: introduce specific errors and default mode
  test: introduce flag LUAJIT_DISABLE_MEMPROF
  ci: add workflow with disabled profilers
  misc: specific message for disabled profilers
  memprof: set default path to profiling output file

 .github/workflows/exotic-builds-testing.yml   |   6 +-
 src/lib_misc.c                                | 107 ++++++++++++------
 src/lj_errmsg.h                               |   6 +
 test/tarantool-tests/CMakeLists.txt           |   4 +
 .../gh-5994-memprof-human-readable.test.lua   |   1 +
 ...misclib-memprof-lapi-default-file.test.lua |  37 ++++++
 .../misclib-memprof-lapi-disabled.test.lua    |  22 ++++
 .../profilers/misclib-memprof-lapi.test.lua   |  17 +--
 .../misclib-sysprof-lapi-disabled.test.lua    |  29 +++++
 .../profilers/misclib-sysprof-lapi.test.lua   | 105 +++++++++++++----
 10 files changed, 273 insertions(+), 61 deletions(-)
 create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
 create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
 create mode 100644 test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua

-- 
2.43.0

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

[Sergey Bronnikov via Tarantool-patches]

The patch adds descriptions to sysprof testcases to make the TAP
report more usable.
---
 .../profilers/misclib-sysprof-lapi.test.lua   | 43 ++++++++++++-------
 1 file changed, 27 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..237143ad 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -65,34 +65,38 @@ 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"),
+        "result status with wrong profiling mode")
+test:ok(type(errno) == "number", "errno with wrong profiling mode")
 
 -- 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, "result status 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"),
+        "result status 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"),
+        "result status and error with bad interval")
+test:ok(type(errno) == "number", "errno with bad interval")
 
 -- DEFAULT MODE
 
@@ -102,20 +106,26 @@ 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 for the default payload")
 -- 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 " ..
+        "for the default payload")
 -- 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 " ..
+        "for the default payload")
 -- Check all JIT-related VM states are not sampled.
+local msg = "total number of VM state %s is equal to 0 for the default payload"
 for _, vmstate in pairs({ 'TRACE', 'RECORD', 'OPT', 'ASM', 'EXIT' }) do
-  test:ok(report.vmstate[vmstate] == 0)
+  test:ok(report.vmstate[vmstate] == 0, msg:format(vmstate))
 end
 
 -- With very big interval.
@@ -124,7 +134,8 @@ 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 for " ..
+        "the too big sampling interval")
 
 -- LEAF MODE
 check_mode("L", 100)
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 2/8][v3] test: align test title with test filename

[Sergey Bronnikov via Tarantool-patches]

The patch fixes TAP test title to make it aligned with test
filename. Inconsistency introduced in commit
efd7e1a67de24464e1e83793e16eac6df443e3c0
("test: make skipcond helper more convenient").
---
 test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua | 2 +-
 test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua
index ce41e4d5..077a7e44 100644
--- a/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua
@@ -1,7 +1,7 @@
 -- XXX: This comment is a reminder to reimplement memprof tests
 -- assertions to make them more independent to the changes made.
 local tap = require("tap")
-local test = tap.test("misc-memprof-lapi"):skipcond({
+local test = tap.test("misclib-memprof-lapi"):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
   ["Memprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index 237143ad..32fa384c 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -1,5 +1,5 @@
 local tap = require("tap")
-local test = tap.test("misc-sysprof-lapi"):skipcond({
+local test = tap.test("misclib-sysprof-lapi"):skipcond({
   ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
                                                jit.arch ~= "x64",
   ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 3/8][v3] 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
2593a9f83072ca999d5f528e1da481e8ba51d5b8 ("misc: introduce Lua
API for new sampling profiler").

Follows up tarantool/tarantool#781
---
 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.43.0

[Tarantool-patches] [PATCH luajit 4/8][v3] 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
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

[Tarantool-patches] [PATCH luajit 5/8][v3] test: introduce flag LUAJIT_DISABLE_MEMPROF

[Sergey Bronnikov via Tarantool-patches]

The patch introduce an environment variable LUAJIT_DISABLE_MEMPROF
needed by tests in suite `tarantool-tests` for skip testcases when
memprof is disabled in build.

Needed for the following commit.
---
 test/tarantool-tests/CMakeLists.txt               |  4 ++++
 .../gh-5994-memprof-human-readable.test.lua       |  1 +
 .../profilers/misclib-memprof-lapi.test.lua       | 15 ++++++++-------
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 9bacac88..a6d09dc7 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -97,6 +97,10 @@ if(LUAJIT_DISABLE_SYSPROF)
   list(APPEND LUA_TEST_ENV_MORE LUAJIT_DISABLE_SYSPROF=1)
 endif()
 
+if(LUAJIT_DISABLE_MEMPROF)
+  list(APPEND LUA_TEST_ENV_MORE LUAJIT_DISABLE_MEMPROF=1)
+endif()
+
 # Needed to skip some long tests or tests using signals.
 # See also https://github.com/tarantool/tarantool/issues/10803.
 if(LUAJIT_USE_VALGRIND)
diff --git a/test/tarantool-tests/profilers/gh-5994-memprof-human-readable.test.lua b/test/tarantool-tests/profilers/gh-5994-memprof-human-readable.test.lua
index f3041779..3dc0db6b 100644
--- a/test/tarantool-tests/profilers/gh-5994-memprof-human-readable.test.lua
+++ b/test/tarantool-tests/profilers/gh-5994-memprof-human-readable.test.lua
@@ -7,6 +7,7 @@ local test = tap.test('gh-5994-memprof-human-readable'):skipcond({
   ['No profile tools CLI option integration'] = _TARANTOOL,
   -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
   ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
+  ['Memprof is disabled'] = os.getenv('LUAJIT_DISABLE_MEMPROF'),
 })
 
 local utils = require('utils')
diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua
index 077a7e44..cd675864 100644
--- a/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-memprof-lapi.test.lua
@@ -8,6 +8,7 @@ local test = tap.test("misclib-memprof-lapi"):skipcond({
                                                jit.arch ~= "x64",
   -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
   ["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"),
+  ["Memprof is disabled"] = os.getenv("LUAJIT_DISABLE_MEMPROF"),
 })
 
 test:plan(5)
@@ -164,9 +165,9 @@ test:test("output", function(subtest)
   -- one is the number of allocations. 1 event - allocation of
   -- table by itself + 1 allocation of array part as far it is
   -- bigger than LJ_MAX_COLOSIZE (16).
-  subtest:ok(check_alloc_report(alloc, { line = 39, linedefined = 37 }, 2))
+  subtest:ok(check_alloc_report(alloc, { line = 40, linedefined = 38 }, 2))
   -- 20 strings allocations.
-  subtest:ok(check_alloc_report(alloc, { line = 44, linedefined = 37 }, 20))
+  subtest:ok(check_alloc_report(alloc, { line = 45, linedefined = 38 }, 20))
 
   -- Collect all previous allocated objects.
   subtest:ok(free.INTERNAL == 22)
@@ -174,8 +175,8 @@ test:test("output", function(subtest)
   -- Tests for leak-only option.
   -- See also https://github.com/tarantool/tarantool/issues/5812.
   local heap_delta = process.form_heap_delta(events)
-  local tab_alloc_stats = heap_delta[form_source_line(39)]
-  local str_alloc_stats = heap_delta[form_source_line(44)]
+  local tab_alloc_stats = heap_delta[form_source_line(40)]
+  local str_alloc_stats = heap_delta[form_source_line(45)]
   subtest:ok(tab_alloc_stats.nalloc == tab_alloc_stats.nfree)
   subtest:ok(tab_alloc_stats.dbytes == 0)
   subtest:ok(str_alloc_stats.nalloc == str_alloc_stats.nfree)
@@ -260,10 +261,10 @@ test:test("jit-output", function(subtest)
   -- 10 allocations in interpreter mode, 1 allocation for a trace
   -- recording and assembling and next 9 allocations will happen
   -- while running the trace.
-  subtest:ok(check_alloc_report(alloc, { line = 44, linedefined = 37 }, 11))
-  subtest:ok(check_alloc_report(alloc, { traceno = 1, line = 42 }, 9))
+  subtest:ok(check_alloc_report(alloc, { line = 45, linedefined = 38 }, 11))
+  subtest:ok(check_alloc_report(alloc, { traceno = 1, line = 43 }, 9))
   -- See same checks with jit.off().
-  subtest:ok(check_alloc_report(alloc, { line = 39, linedefined = 37 }, 2))
+  subtest:ok(check_alloc_report(alloc, { line = 40, linedefined = 38 }, 2))
 
   -- Restore default JIT settings.
   jit.opt.start(unpack(jit_opt_default))
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 6/8][v3] ci: add workflow with disabled profilers

[Sergey Bronnikov via Tarantool-patches]

Needed for the following commit.
---
 .github/workflows/exotic-builds-testing.yml | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
index 374c879b..0c575404 100644
--- a/.github/workflows/exotic-builds-testing.yml
+++ b/.github/workflows/exotic-builds-testing.yml
@@ -34,7 +34,7 @@ jobs:
         BUILDTYPE: [Debug, Release]
         ARCH: [ARM64, x86_64]
         GC64: [ON, OFF]
-        FLAVOR: [checkhook, dualnum, dynamic, gdbjit, nojit, nounwind, tablebump]
+        FLAVOR: [checkhook, dualnum, dynamic, gdbjit, nojit, noprof, nounwind, tablebump]
         include:
           - BUILDTYPE: Debug
             CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
@@ -48,6 +48,8 @@ jobs:
             FLAVORFLAGS: -DLUAJIT_ENABLE_CHECKHOOK=ON
           - FLAVOR: nojit
             FLAVORFLAGS: -DLUAJIT_DISABLE_JIT=ON
+          - FLAVOR: noprof
+            FLAVORFLAGS: -DLUAJIT_DISABLE_MEMPROF=ON -DLUAJIT_DISABLE_SYSPROF=ON
           - FLAVOR: gdbjit
             FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
           - FLAVOR: nounwind
@@ -66,6 +68,8 @@ jobs:
             ARCH: ARM64
           - FLAVOR: dynamic
             BUILDTYPE: Release
+          - FLAVOR: noprof
+            ARCH: ARM64
           # With table bump optimization enabled (and due to our modification
           # related to metrics), some offsets in GG_State stop fitting in 12bit
           # immediate. Hence, the build failed due to the DASM error
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 7/8][v3] misc: specific message for disabled profilers

[Sergey Bronnikov via Tarantool-patches]

sysprof and memprof Lua API functions return an error message
"profiler misuse", when the corresponding profiler is disabled in
the build. It is not possible to easily distinguish whether it is
really misuse or if the profiler was not enabled in the build. The
patch changes error messages, so when profiler is not enabled in
the build, the message is the following: "profiler misuse:
profiler is disabled".
---
 src/lib_misc.c                                | 25 ++++++++++++++--
 src/lj_errmsg.h                               |  1 +
 .../misclib-memprof-lapi-disabled.test.lua    | 22 ++++++++++++++
 .../misclib-sysprof-lapi-disabled.test.lua    | 29 +++++++++++++++++++
 4 files changed, 75 insertions(+), 2 deletions(-)
 create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
 create mode 100644 test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 1fd06dd1..d98cf3f0 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -306,10 +306,15 @@ static int sysprof_error(lua_State *L, int status, const char *err_details)
 /* local res, err, errno = sysprof.start(options) */
 LJLIB_CF(misc_sysprof_start)
 {
+  const char *err_details = NULL;
   int status = PROFILE_SUCCESS;
 
   struct luam_Sysprof_Options opt = {};
-  const char *err_details = NULL;
+
+  if (!LJ_HASSYSPROF) {
+    err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+    return sysprof_error(L, PROFILE_ERRUSE, err_details);
+  }
 
   status = parse_sysprof_opts(L, &opt, &err_details);
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
@@ -328,6 +333,10 @@ LJLIB_CF(misc_sysprof_start)
 LJLIB_CF(misc_sysprof_stop)
 {
   int status = luaM_sysprof_stop(L);
+  if (!LJ_HASSYSPROF) {
+    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+    return sysprof_error(L, PROFILE_ERRUSE, err_details);
+  }
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
     return sysprof_error(L, status, NULL);
 
@@ -341,8 +350,12 @@ LJLIB_CF(misc_sysprof_report)
   struct luam_Sysprof_Counters counters = {};
   GCtab *data_tab = NULL;
   GCtab *count_tab = NULL;
-
   int status = luaM_sysprof_report(&counters);
+  if (!LJ_HASSYSPROF) {
+    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+    return sysprof_error(L, PROFILE_ERRUSE, err_details);
+  }
+
   if (status != PROFILE_SUCCESS)
     return sysprof_error(L, status, NULL);
 
@@ -381,6 +394,10 @@ LJLIB_CF(misc_memprof_start)
   const char *fname = strdata(lj_lib_checkstr(L, 1));
   struct profile_ctx *ctx;
   int memprof_status;
+  if (!LJ_HASMEMPROF) {
+    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+    return sysprof_error(L, PROFILE_ERRUSE, err_details);
+  }
 
   /*
   ** FIXME: more elegant solution with ctx.
@@ -432,6 +449,10 @@ LJLIB_CF(misc_memprof_start)
 LJLIB_CF(misc_memprof_stop)
 {
   int status = lj_memprof_stop(L);
+  if (!LJ_HASMEMPROF) {
+    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+    return sysprof_error(L, PROFILE_ERRUSE, err_details);
+  }
   if (status != PROFILE_SUCCESS) {
     switch (status) {
     case PROFILE_ERRUSE:
diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h
index b5c3a275..e26f5e38 100644
--- a/src/lj_errmsg.h
+++ b/src/lj_errmsg.h
@@ -193,6 +193,7 @@ 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")
 
+ERRDEF(PROF_DETAILS_DISABLED, "profiler is disabled")
 #undef ERRDEF
 
 /* Detecting unused error messages:
diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
new file mode 100644
index 00000000..de0aa136
--- /dev/null
+++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
@@ -0,0 +1,22 @@
+local tap = require('tap')
+local test = tap.test('misclib-memprof-lapi-disabled'):skipcond({
+  ['Memprof is enabled'] = not os.getenv('LUAJIT_DISABLE_MEMPROF'),
+})
+
+test:plan(6)
+
+-- Attempt to start memprof when it is disabled.
+local res, err, errno = misc.memprof.start()
+test:is(res, nil, 'result status on start when memprof is disabled')
+test:ok(err:match('profiler is disabled'),
+        'error on start when memprof is disabled')
+test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
+
+-- Attempt to stop memprof when it is disabled.
+res, err, errno = misc.memprof.stop()
+test:is(res, nil, 'result status on stop when memprof is disabled')
+test:ok(err:match('profiler is disabled'),
+        'error on stop when memprof is disabled')
+test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
+
+test:done(true)
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
new file mode 100644
index 00000000..2a9ce796
--- /dev/null
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
@@ -0,0 +1,29 @@
+local tap = require('tap')
+local test = tap.test('misclib-sysprof-lapi-disabled'):skipcond({
+  ['Sysprof is enabled'] = not os.getenv('LUAJIT_DISABLE_SYSPROF'),
+})
+
+test:plan(9)
+
+-- Attempt to start sysprof when sysprof is disabled.
+local res, err, errno = misc.sysprof.start()
+test:is(res, nil, 'result status on start when sysprof is disabled')
+test:ok(err:match('profiler is disabled'),
+        'error on start when sysprof is disabled')
+test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
+
+-- Attempt to stop sysprof when sysprof is disabled.
+res, err, errno = misc.sysprof.stop()
+test:is(res, nil, 'result status on stop when sysprof is disabled')
+test:ok(err:match('profiler is disabled'),
+        'error on stop when sysprof is disabled')
+test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
+
+-- Attempt to report when sysprof is disabled.
+res, err, errno = misc.sysprof.report()
+test:is(res, nil, 'result status on report when sysprof is disabled')
+test:ok(err:match('profiler is disabled'),
+        'error on stop when sysprof is disabled')
+test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
+
+test:done(true)
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 8/8][v3] memprof: set default path to profiling output file

[Sergey Bronnikov via Tarantool-patches]

sysprof has an optional parameter `path`, that sets a path to
the profiling output file. By default the path is `sysprof.bin`.
`misc.memprof.start()` requires to set a path to profiling output
file. The patch fixes this inconsistency by introducing a default
path to the memprof profiling output file - `memprof.bin`.
---
 src/lib_misc.c                                |  5 ++-
 ...misclib-memprof-lapi-default-file.test.lua | 37 +++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua

diff --git a/src/lib_misc.c b/src/lib_misc.c
index d98cf3f0..92dc6e2a 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -387,11 +387,14 @@ LJLIB_CF(misc_sysprof_report)
 
 #define LJLIB_MODULE_misc_memprof
 
+#define MEMPROF_DEFAULT_OUTPUT "memprof.bin"
+
 /* local started, err, errno = misc.memprof.start(fname) */
 LJLIB_CF(misc_memprof_start)
 {
   struct lj_memprof_options opt = {0};
-  const char *fname = strdata(lj_lib_checkstr(L, 1));
+  GCstr *s = lj_lib_optstr(L, 1);
+  const char *fname = s ? strdata(s) : MEMPROF_DEFAULT_OUTPUT;
   struct profile_ctx *ctx;
   int memprof_status;
   if (!LJ_HASMEMPROF) {
diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
new file mode 100644
index 00000000..ae8a73c9
--- /dev/null
+++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
@@ -0,0 +1,37 @@
+local tap = require('tap')
+local test = tap.test('misc-memprof-lapi-default-file'):skipcond({
+  ['Memprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
+                                               jit.arch ~= 'x64',
+  ['Memprof is disabled'] = os.getenv('LUAJIT_DISABLE_MEMPROF'),
+})
+
+test:plan(1)
+
+local tools = require('utils.tools')
+
+test:test('default-output-file', function(subtest)
+
+  subtest:plan(1)
+
+  local default_output_file = 'memprof.bin'
+  os.remove(default_output_file)
+
+  local _, _ = misc.memprof.start()
+
+  local res, err = misc.memprof.stop()
+
+  -- Want to cleanup carefully if something went wrong.
+  if not res then
+    os.remove(default_output_file)
+    error(err)
+  end
+
+  local profile_buf = tools.read_file(default_output_file)
+  subtest:ok(profile_buf ~= nil and #profile_buf ~= 0,
+             'default output file is not empty')
+
+  -- We don't need it any more.
+  os.remove(default_output_file)
+end)
+
+test:done(true)
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit 2/8][v3] test: align test title with test filename

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, except a single nit regarding the commit message.

On 20.02.25, Sergey Bronnikov wrote:
> The patch fixes TAP test title to make it aligned with test
> filename. Inconsistency introduced in commit
> efd7e1a67de24464e1e83793e16eac6df443e3c0
> ("test: make skipcond helper more convenient").

I am afraid that this naming was introduced in the first commit related
to the tests (sysprof and memprof). So we either should mention 2
initial commits or just omit this part.

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 5/8][v3] test: introduce flag LUAJIT_DISABLE_MEMPROF

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, except a few nits regarding the commit message.

On 20.02.25, Sergey Bronnikov wrote:
> The patch introduce an environment variable LUAJIT_DISABLE_MEMPROF

Typo: s/introduce/introduces/


> needed by tests in suite `tarantool-tests` for skip testcases when

Typo: s/suite/the suite/
Typo: s/for skip/to skip/

> memprof is disabled in build.

Typo: s/build/the build/

> 
> Needed for the following commit.
> ---

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 6/8][v3] ci: add workflow with disabled profilers

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes.
LGTM, after fixing a nit regarding the commit message below.

Minor: Please provide a minor sentence that mentions that this workflow
runs our test suite with both profilers (memprof and sysprof) disabled.

On 20.02.25, Sergey Bronnikov wrote:
> Needed for the following commit.
> ---

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 8/8][v3] memprof: set default path to profiling output file

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, with a few small nits below.

On 20.02.25, Sergey Bronnikov wrote:
> sysprof has an optional parameter `path`, that sets a path to
> the profiling output file. By default the path is `sysprof.bin`.

Typo: s/default/default,/

> `misc.memprof.start()` requires to set a path to profiling output

Typo: s/to set/setting/
Typo: s/profiling/the profiling/

> file. The patch fixes this inconsistency by introducing a default
> path to the memprof profiling output file - `memprof.bin`.
> ---
>  src/lib_misc.c                                |  5 ++-
>  ...misclib-memprof-lapi-default-file.test.lua | 37 +++++++++++++++++++
>  2 files changed, 41 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
> 
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index d98cf3f0..92dc6e2a 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c

<snipped>

> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
> new file mode 100644
> index 00000000..ae8a73c9
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
> @@ -0,0 +1,37 @@
> +local tap = require('tap')
> +local test = tap.test('misc-memprof-lapi-default-file'):skipcond({
> +  ['Memprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
> +                                               jit.arch ~= 'x64',
> +  ['Memprof is disabled'] = os.getenv('LUAJIT_DISABLE_MEMPROF'),
> +})
> +
> +test:plan(1)
> +
> +local tools = require('utils.tools')
> +
> +test:test('default-output-file', function(subtest)
> +
> +  subtest:plan(1)
> +
> +  local default_output_file = 'memprof.bin'
> +  os.remove(default_output_file)
> +
> +  local _, _ = misc.memprof.start()

Minor: Do we want to check that the profiler starts successfully?
I suppose we should, since this is the expected behaviour for this
feature. In case the profiler is not started (old behaviour) we would
get an error from the branch below: profiler not running. This isn't a
verbose definition of what goes wrong.
I suppose we may use `goto` here like the following:

|   local res, err = misc.memprof.start()
|   -- Should start successfully.
|   if not res then
|     goto err_handling
|   end
|
|   res, err = misc.memprof.stop()
|
| ::err_handling::
|   -- Want to cleanup carefully if something went wrong.
|   if not res then

> +
> +  local res, err = misc.memprof.stop()
> +
> +  -- Want to cleanup carefully if something went wrong.
> +  if not res then
> +    os.remove(default_output_file)
> +    error(err)
> +  end
> +
> +  local profile_buf = tools.read_file(default_output_file)
> +  subtest:ok(profile_buf ~= nil and #profile_buf ~= 0,
> +             'default output file is not empty')
> +
> +  -- We don't need it any more.
> +  os.remove(default_output_file)
> +end)
> +
> +test:done(true)
> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
LGTM, since there are no changes in this version.

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/8][v3] sysprof: fix typo in the comment

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
LGTM, since there are no changes in this version.

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 7/8][v3] misc: specific message for disabled profilers

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing my comments below.

On 20.02.25, Sergey Bronnikov wrote:
> sysprof and memprof Lua API functions return an error message
> "profiler misuse", when the corresponding profiler is disabled in
> the build. It is not possible to easily distinguish whether it is
> really misuse or if the profiler was not enabled in the build. The
> patch changes error messages, so when profiler is not enabled in
> the build, the message is the following: "profiler misuse:
> profiler is disabled".
> ---
>  src/lib_misc.c                                | 25 ++++++++++++++--
>  src/lj_errmsg.h                               |  1 +
>  .../misclib-memprof-lapi-disabled.test.lua    | 22 ++++++++++++++
>  .../misclib-sysprof-lapi-disabled.test.lua    | 29 +++++++++++++++++++
>  4 files changed, 75 insertions(+), 2 deletions(-)
>  create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>  create mode 100644 test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
> 
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index 1fd06dd1..d98cf3f0 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -306,10 +306,15 @@ static int sysprof_error(lua_State *L, int status, const char *err_details)
>  /* local res, err, errno = sysprof.start(options) */
>  LJLIB_CF(misc_sysprof_start)
>  {

As we discussed ofline we may use 2 approaches here:

1) Use branching:
a) Main branch first.
| {
|    if (LJ_HASSYSPROF) {
|      /* ... */
|    } else {
|      const char *err_details = //
|      /* ... */
|    }
| }

b) Early return first
| {
|    if (!LJ_HASSYSPROF) {
|      const char *err_details = //
|      /* ... */
|    } else {
|      /* ... */
|    }
| }

2) Use macros directives instead:
a) Main branch first.
| {
| #if LJ_HASSYSPROF
|      /* ... */
| #else
|      /* ... */
|    }
| #endif

b) Early return first
| {
| #if !LJ_HASSYSPROF
|      /* ... */
| #else
|      /* ... */
|    }
| #endif

We decided to use the second approach to avoid huge diff changes and
make code more readable.
It's up to you to use an approach a) or b).
This helps us to avoid warnings with `-Wdeclaration-after-statement`
enabled.

Here and below.

Also, it helps to avoid an excess call to `lj_{sysprof,memprof}_stop()`.

> +  const char *err_details = NULL;
>    int status = PROFILE_SUCCESS;
>  
>    struct luam_Sysprof_Options opt = {};
> -  const char *err_details = NULL;
> +
> +  if (!LJ_HASSYSPROF) {
> +    err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
>  
>    status = parse_sysprof_opts(L, &opt, &err_details);
>    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
> @@ -328,6 +333,10 @@ LJLIB_CF(misc_sysprof_start)
>  LJLIB_CF(misc_sysprof_stop)
>  {
>    int status = luaM_sysprof_stop(L);
> +  if (!LJ_HASSYSPROF) {
> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
>    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>      return sysprof_error(L, status, NULL);
>  
> @@ -341,8 +350,12 @@ LJLIB_CF(misc_sysprof_report)
>    struct luam_Sysprof_Counters counters = {};
>    GCtab *data_tab = NULL;
>    GCtab *count_tab = NULL;
> -
>    int status = luaM_sysprof_report(&counters);
> +  if (!LJ_HASSYSPROF) {
> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
> +
>    if (status != PROFILE_SUCCESS)
>      return sysprof_error(L, status, NULL);
>  
> @@ -381,6 +394,10 @@ LJLIB_CF(misc_memprof_start)
>    const char *fname = strdata(lj_lib_checkstr(L, 1));
>    struct profile_ctx *ctx;
>    int memprof_status;
> +  if (!LJ_HASMEMPROF) {
> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
>  
>    /*
>    ** FIXME: more elegant solution with ctx.
> @@ -432,6 +449,10 @@ LJLIB_CF(misc_memprof_start)
>  LJLIB_CF(misc_memprof_stop)
>  {
>    int status = lj_memprof_stop(L);
> +  if (!LJ_HASMEMPROF) {
> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +  }
>    if (status != PROFILE_SUCCESS) {
>      switch (status) {
>      case PROFILE_ERRUSE:
> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h
> index b5c3a275..e26f5e38 100644
> --- a/src/lj_errmsg.h
> +++ b/src/lj_errmsg.h
> @@ -193,6 +193,7 @@ 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")
>  
> +ERRDEF(PROF_DETAILS_DISABLED, "profiler is disabled")
>  #undef ERRDEF
>  
>  /* Detecting unused error messages:
> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
> new file mode 100644
> index 00000000..de0aa136
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
> @@ -0,0 +1,22 @@
> +local tap = require('tap')
> +local test = tap.test('misclib-memprof-lapi-disabled'):skipcond({
> +  ['Memprof is enabled'] = not os.getenv('LUAJIT_DISABLE_MEMPROF'),
> +})
> +
> +test:plan(6)
> +
> +-- Attempt to start memprof when it is disabled.
> +local res, err, errno = misc.memprof.start()
> +test:is(res, nil, 'result status on start when memprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on start when memprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
> +
> +-- Attempt to stop memprof when it is disabled.
> +res, err, errno = misc.memprof.stop()
> +test:is(res, nil, 'result status on stop when memprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on stop when memprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
> new file mode 100644
> index 00000000..2a9ce796
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
> @@ -0,0 +1,29 @@
> +local tap = require('tap')
> +local test = tap.test('misclib-sysprof-lapi-disabled'):skipcond({
> +  ['Sysprof is enabled'] = not os.getenv('LUAJIT_DISABLE_SYSPROF'),
> +})
> +
> +test:plan(9)
> +
> +-- Attempt to start sysprof when sysprof is disabled.
> +local res, err, errno = misc.sysprof.start()
> +test:is(res, nil, 'result status on start when sysprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on start when sysprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
> +
> +-- Attempt to stop sysprof when sysprof is disabled.
> +res, err, errno = misc.sysprof.stop()
> +test:is(res, nil, 'result status on stop when sysprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on stop when sysprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
> +
> +-- Attempt to report when sysprof is disabled.
> +res, err, errno = misc.sysprof.report()
> +test:is(res, nil, 'result status on report when sysprof is disabled')
> +test:ok(err:match('profiler is disabled'),
> +        'error on stop when sysprof is disabled')
> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
> +
> +test:done(true)
> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, after the final polishing, please consider my comments below.

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

Please add the following notes.

| Also, it fixes the bug when the error isn't raised if the given argument
| type is incorrect.
| Also, the profiler can start if it has extra arguments given.
| They are just ignored now.

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

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

Nit: Please, replace every 8 spaces with one tab.

> +  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);

Please, declare (not initialize) `options` before `if` branch to avoid
-Wdeclaration-after-statement warnings.

Please, also add the comment that we ignore all other arguments given to
this function.

>  
>    /* Get profiling mode. */
>    {

<snipped>

>    }
> @@ -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)

Please, add the `{` braces here too.

>        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

Ditto.

>        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;
>  }

<snipped>

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

Nit: OTOH, there is no need for them here and below.
Feel free to ignore.

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

Side note: `err_deails` is always `NULL` here. I suppose we don't need
this branch for now (to avoid the dead code), so these changes may be
avoided.

> +        lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);

Typo: s/LJ_ERR_PROF_MISUSE/LJ_ERR_PROF_ISRUNNING/
Irrelevant, after fixing the comment above.

> +      } 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)

<snipped>

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

This should be something like the following:
| "profiler path should be a string"

> +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")

This errmsg is unused now and can be deleted.

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

<snipped>

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

Why do we need this definition for only one type of bad interval?
Let's consistently declare both of them like:
| local BAD_INTERVAL_M1 = -1
| local BAD_INTERVAL_0  = 0
Or don't declare them at all.

I'm OK with both variants.

>  
>  local function payload()
>    local function fib(n)
> @@ -64,11 +66,44 @@ end

<snipped>

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

Minor: It would be nice to check that there is no default file created,
i.e., that we actually run the default profiler mode without stack
inspection. Feel free to ignore, through.

> +
> +-- Not a table.
> +res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")
> +print(res, err, errno)

Excess debug print.

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

> +
> +-- 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")

This is not exactly what I mean by invalid/partially invalid parameters
(sorry, for the bad wording):
I mean to add tests that cover the following calls:
| pcall(misc.sysprof.start, '', '', '') -- shouldn't started -- error
| -- (expected table got nil)
| misc.sysprof.start({}, '', '') -- should started with the default options

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

Side note: See my comments about this constant above.

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

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/8][v3] test: align test title with test filename

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

On 24.02.2025 12:40, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, except a single nit regarding the commit message.
>
> On 20.02.25, Sergey Bronnikov wrote:
>> The patch fixes TAP test title to make it aligned with test
>> filename. Inconsistency introduced in commit
>> efd7e1a67de24464e1e83793e16eac6df443e3c0
>> ("test: make skipcond helper more convenient").
> I am afraid that this naming was introduced in the first commit related
> to the tests (sysprof and memprof). So we either should mention 2
> initial commits or just omit this part.
>
> <snipped>
>
Sorry, it was really introduced earlier.

Removed this sentence at all.

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

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

[Sergey Bronnikov via Tarantool-patches]

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

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:
>> 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.
> Please add the following notes.
>
> | Also, it fixes the bug when the error isn't raised if the given argument
> | type is incorrect.
> | Also, the profiler can start if it has extra arguments given.
> | They are just ignored now.
Added.
>
>> ---
>>   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).
>
>>   #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) {
> 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) {
    int n = (int)(L->top - L->base);
    if (n == 0) {
      opt->mode = LUAM_SYSPROF_DEFAULT;

>
>> +  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);
> Please, declare (not initialize) `options` before `if` branch to avoid
> -Wdeclaration-after-statement warnings.

Fixed:

    int n = (int)(L->top - L->base);
+  GCtab *options;
    if (n == 0) {
      opt->mode = LUAM_SYSPROF_DEFAULT;
      opt->interval = SYSPROF_DEFAULT_INTERVAL;
      return PROFILE_SUCCESS;
    }

-  GCtab *options = lj_lib_checktab(L, 1);
+  options = lj_lib_checktab(L, 1);

    /* Get profiling mode. */
    {

>
> Please, also add the comment that we ignore all other arguments given to
> this function.

Added:

    if (n == 0) {
      opt->mode = LUAM_SYSPROF_DEFAULT;
      opt->interval = SYSPROF_DEFAULT_INTERVAL;
      return PROFILE_SUCCESS;
    }

-  GCtab *options = lj_lib_checktab(L, 1);
+  /* All other arguments given to this function are ignored. */
+  options = lj_lib_checktab(L, 1);

    /* Get profiling mode. */
    {

>
>>   
>>     /* Get profiling mode. */
>>     {
> <snipped>
>
>>     }
>> @@ -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)
> Please, add the `{` braces here too.

Added:

@@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
      int status = 0;

      cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path"));
-    if (!pathtv)
+    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));
      ctx->g = G(L);

>
>>         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
> Ditto.
@@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
      int status = 0;

      cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path"));
-    if (!pathtv)
+    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));
      ctx->g = G(L);
>
>>         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;
>>   }
> <snipped>
>
>> +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) {
> Nit: OTOH, there is no need for them here and below.
> Feel free to ignore.

Removed:

@@ -277,21 +280,16 @@ static int sysprof_error(lua_State *L, int status, 
const char *err_details)
    switch (status) {
      case PROFILE_ERRUSE:
        lua_pushnil(L);
-      if (err_details) {
+      if (err_details)
          lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), 
err_details);
-      } else {
+      else
          lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
-      }
        lua_pushinteger(L, EINVAL);
        return 3;

>> +        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) {
> Side note: `err_deails` is always `NULL` here. I suppose we don't need
> this branch for now (to avoid the dead code), so these changes may be
> avoided.

Removed:

  #if LJ_HASSYSPROF
      case PROFILE_ERRRUN:
        lua_pushnil(L);
-      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_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
        lua_pushinteger(L, EINVAL);
        return 3;
      case PROFILE_ERRIO:

>> +        lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
> Typo: s/LJ_ERR_PROF_MISUSE/LJ_ERR_PROF_ISRUNNING/
> Irrelevant, after fixing the comment above.
Yep, irrelevant now.
>
>> +      } 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)
> <snipped>
>
>> 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")
> This should be something like the following:
> | "profiler path should be a string"

Fixed:

--- a/src/lj_errmsg.h
+++ b/src/lj_errmsg.h
@@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING,     "profiler is not running")

  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")
+ERRDEF(PROF_DETAILS_BADPATH, "profiler path should be a string")

  #undef ERRDEF

>
>> +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
> This errmsg is unused now and can be deleted.

Thanks, fixed:

--- a/src/lj_errmsg.h
+++ b/src/lj_errmsg.h
@@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING,     "profiler is not running")

  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")
+ERRDEF(PROF_DETAILS_BADPATH, "profiler path should be a string")

  #undef ERRDEF

@@ -127,6 +127,13 @@ test:ok(res == nil and err:match("No such file or 
directory"),
          "result status and error with bad path")
test:ok(type(errno) == "number", "errno with bad path")

+-- Path is not a string.
+res, err, errno = misc.sysprof.start({ mode = 'C', path = 190 })
+test:ok(res == nil, "result status with path is not a string")
+test:ok(err:match("profiler path should be a string"),
+        "err with path is not a string")
+test:ok(type(errno) == "number", "errno with path is not a string")
+
  -- Bad interval.
  res, err, errno = misc.sysprof.start{ mode = "C", interval = 
BAD_INTERVAL }
test:is(res, nil, "result status and error with bad interval")

>
>> +
>>   #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
> <snipped>
>
>> @@ -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
> Why do we need this definition for only one type of bad interval?
> Let's consistently declare both of them like:
> | local BAD_INTERVAL_M1 = -1
> | local BAD_INTERVAL_0  = 0
> Or don't declare them at all.
>
> I'm OK with both variants.

BAD_INTERVAL is used in two testcases therefore it is declared as constant.

>>   
>>   local function payload()
>>     local function fib(n)
>> @@ -64,11 +66,44 @@ end
> <snipped>
>
>>   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())
> Minor: It would be nice to check that there is no default file created,
> i.e., that we actually run the default profiler mode without stack
> inspection. Feel free to ignore, through.

Added:

@@ -77,6 +79,9 @@ 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")
+local ok, err_msg = pcall(read_file, SYSPROF_DEFAULT_OUTPUT_FILE)
+test:ok(ok == false and err_msg:match("cannot open a file"),
+        "default output file is empty")
  assert(misc.sysprof.stop())

  -- Not a table.

>> +
>> +-- Not a table.
>> +res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")
>> +print(res, err, errno)
> Excess debug print.
Removed, thanks!
>
>> +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?


>
>> +
>> +-- 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")
> This is not exactly what I mean by invalid/partially invalid parameters
> (sorry, for the bad wording):
> I mean to add tests that cover the following calls:
> | pcall(misc.sysprof.start, '', '', '') -- shouldn't started -- error
> | -- (expected table got nil)
> | misc.sysprof.start({}, '', '') -- should started with the default options
>
Verbally decided to left these testcases with a comment:

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.
+-- All parameters are invalid. Check parameter validation order
+-- (not strict documented behaviour).
  res, err, errno = misc.sysprof.start({
    mode = BAD_MODE, path = BAD_PATH, interval = BAD_INTERVAL })
test:ok(res == nil, "res with all invalid parameters")
@@ -95,7 +100,8 @@ 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.
+-- All parameters are invalid, except the first one. Check
+-- parameter validation order (not strict documented behaviour).
  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")


>> +
>>   -- Already running.
>>   res, err = misc.sysprof.start{ mode = "D" }
>>   assert(res, err)
>> @@ -93,11 +128,30 @@test:ok(res == nil anderr: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 anderr:match("profiler misuse"),
>> -        "result status and error with bad interval")
>> +res, err, errno = misc.sysprof.start{ mode = "C", interval = BAD_INTERVAL }
> Side note: See my comments about this constant above.
decided to left as is
>
>> +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
>>

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

Re: [Tarantool-patches] [PATCH luajit 5/8][v3] test: introduce flag LUAJIT_DISABLE_MEMPROF

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review!

Updated and force-pushed.

Sergey

On 24.02.2025 12:45, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, except a few nits regarding the commit message.
>
> On 20.02.25, Sergey Bronnikov wrote:
>> The patch introduce an environment variable LUAJIT_DISABLE_MEMPROF
> Typo: s/introduce/introduces/
Fixed.
>
>
>> needed by tests in suite `tarantool-tests` for skip testcases when
> Typo: s/suite/the suite/
> Typo: s/for skip/to skip/
Fixed.
>> memprof is disabled in build.
> Typo: s/build/the build/
Fixed.
>
>> Needed for the following commit.
>> ---
> <snipped>
>

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

Re: [Tarantool-patches] [PATCH luajit 6/8][v3] ci: add workflow with disabled profilers

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review!

On 24.02.2025 12:48, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes.
> LGTM, after fixing a nit regarding the commit message below.
>
> Minor: Please provide a minor sentence that mentions that this workflow
> runs our test suite with both profilers (memprof and sysprof) disabled.

Updated version:

Author: Sergey Bronnikov <sergeyb@tarantool.org>
Date:   Tue Feb 18 17:08:17 2025 +0300

     ci: add workflow with disabled profilers

     The patch adds a workflow that runs our test suite with both
     profilers (memprof and sysprof) disabled.

     Needed for the following commit.

> On 20.02.25, Sergey Bronnikov wrote:
>> Needed for the following commit.
>> ---
> <snipped>
>

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

Re: [Tarantool-patches] [PATCH luajit 7/8][v3] misc: specific message for disabled profilers

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey!

thanks for review!

Updated and force-pushed.

Sergey

On 24.02.2025 14:28, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, after fixing my comments below.
>
> On 20.02.25, Sergey Bronnikov wrote:
>> sysprof and memprof Lua API functions return an error message
>> "profiler misuse", when the corresponding profiler is disabled in
>> the build. It is not possible to easily distinguish whether it is
>> really misuse or if the profiler was not enabled in the build. The
>> patch changes error messages, so when profiler is not enabled in
>> the build, the message is the following:"profiler misuse: profiler is disabled".
>> ---
>>   src/lib_misc.c                                | 25 ++++++++++++++--
>>   src/lj_errmsg.h                               |  1 +
>>   .../misclib-memprof-lapi-disabled.test.lua    | 22 ++++++++++++++
>>   .../misclib-sysprof-lapi-disabled.test.lua    | 29 +++++++++++++++++++
>>   4 files changed, 75 insertions(+), 2 deletions(-)
>>   create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>>   create mode 100644 test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
>>
>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>> index 1fd06dd1..d98cf3f0 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -306,10 +306,15 @@ static int sysprof_error(lua_State *L, int status, const char *err_details)
>>   /* local res, err, errno = sysprof.start(options) */
>>   LJLIB_CF(misc_sysprof_start)
>>   {
> As we discussed ofline we may use 2 approaches here:
>
> 1) Use branching:
> a) Main branch first.
> | {
> |    if (LJ_HASSYSPROF) {
> |      /* ... */
> |    } else {
> |      const char *err_details = //
> |      /* ... */
> |    }
> | }
>
> b) Early return first
> | {
> |    if (!LJ_HASSYSPROF) {
> |      const char *err_details = //
> |      /* ... */
> |    } else {
> |      /* ... */
> |    }
> | }
>
> 2) Use macros directives instead:
> a) Main branch first.
> | {
> | #if LJ_HASSYSPROF
> |      /* ... */
> | #else
> |      /* ... */
> |    }
> | #endif
>
> b) Early return first
> | {
> | #if !LJ_HASSYSPROF
> |      /* ... */
> | #else
> |      /* ... */
> |    }
> | #endif
>
> We decided to use the second approach to avoid huge diff changes and
> make code more readable.
> It's up to you to use an approach a) or b).
> This helps us to avoid warnings with `-Wdeclaration-after-statement`
> enabled.

Updated (approach b)):

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 62d0597e..d5122665 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -304,15 +304,14 @@ static int sysprof_error(lua_State *L, int status, 
const char *err_details)
  /* local res, err, errno = sysprof.start(options) */
  LJLIB_CF(misc_sysprof_start)
  {
-  const char *err_details = NULL;
+#if !LJ_HASSYSPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    int status = PROFILE_SUCCESS;

    struct luam_Sysprof_Options opt = {};
-
-  if (!LJ_HASSYSPROF) {
-    err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-    return sysprof_error(L, PROFILE_ERRUSE, err_details);
-  }
+  const char *err_details = NULL;

    status = parse_sysprof_opts(L, &opt, &err_details);
    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
@@ -325,35 +324,36 @@ LJLIB_CF(misc_sysprof_start)

    lua_pushboolean(L, 1);
    return 1;
+#endif /* !LJ_HASSYSPROF */
  }

  /* local res, err, errno = profile.sysprof_stop() */
  LJLIB_CF(misc_sysprof_stop)
  {
+#if !LJ_HASSYSPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    int status = luaM_sysprof_stop(L);
-  if (!LJ_HASSYSPROF) {
-    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-    return sysprof_error(L, PROFILE_ERRUSE, err_details);
-  }
    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
      return sysprof_error(L, status, NULL);

    lua_pushboolean(L, 1);
    return 1;
+#endif /* !LJ_HASSYSPROF */
  }

  /* local counters, err, errno = sysprof.report() */
  LJLIB_CF(misc_sysprof_report)
  {
+#if !LJ_HASSYSPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    struct luam_Sysprof_Counters counters = {};
    GCtab *data_tab = NULL;
    GCtab *count_tab = NULL;
    int status = luaM_sysprof_report(&counters);
-  if (!LJ_HASSYSPROF) {
-    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-    return sysprof_error(L, PROFILE_ERRUSE, err_details);
-  }
-
    if (status != PROFILE_SUCCESS)
      return sysprof_error(L, status, NULL);

@@ -379,6 +379,7 @@ LJLIB_CF(misc_sysprof_report)
    lua_setfield(L, -2, "vmstate");

    return 1;
+#endif /* !LJ_HASSYSPROF */
  }

  /* ----- misc.memprof module 
---------------------------------------------- */
@@ -388,14 +389,14 @@ LJLIB_CF(misc_sysprof_report)
  /* local started, err, errno = misc.memprof.start(fname) */
  LJLIB_CF(misc_memprof_start)
  {
+#if !LJ_HASMEMPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    struct lj_memprof_options opt = {0};
    const char *fname = strdata(lj_lib_checkstr(L, 1));
    struct profile_ctx *ctx;
    int memprof_status;
-  if (!LJ_HASMEMPROF) {
-    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-    return sysprof_error(L, PROFILE_ERRUSE, err_details);
-  }

    /*
    ** FIXME: more elegant solution with ctx.
@@ -441,16 +442,17 @@ LJLIB_CF(misc_memprof_start)
    }
    lua_pushboolean(L, 1);
    return 1;
+#endif /* !LJ_HASMEMPROF */
  }

  /* local stopped, err, errno = misc.memprof.stop() */
  LJLIB_CF(misc_memprof_stop)
  {
+#if !LJ_HASMEMPROF
+  const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
    int status = lj_memprof_stop(L);
-  if (!LJ_HASMEMPROF) {
-    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-    return sysprof_error(L, PROFILE_ERRUSE, err_details);
-  }
    if (status != PROFILE_SUCCESS) {
      switch (status) {
      case PROFILE_ERRUSE:
@@ -474,6 +476,7 @@ LJLIB_CF(misc_memprof_stop)
    }
    lua_pushboolean(L, 1);
    return 1;
+#endif /* !LJ_HASMEMPROF */
  }
  #endif /* !LJ_TARGET_WINDOWS */

> Here and below.
>
> Also, it helps to avoid an excess call to `lj_{sysprof,memprof}_stop()`.
>
>> +  const char *err_details = NULL;
>>     int status = PROFILE_SUCCESS;
>>   
>>     struct luam_Sysprof_Options opt = {};
>> -  const char *err_details = NULL;
>> +
>> +  if (!LJ_HASSYSPROF) {
>> +    err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +  }
>>   
>>     status = parse_sysprof_opts(L, &opt, &err_details);
>>     if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>> @@ -328,6 +333,10 @@ LJLIB_CF(misc_sysprof_start)
>>   LJLIB_CF(misc_sysprof_stop)
>>   {
>>     int status = luaM_sysprof_stop(L);
>> +  if (!LJ_HASSYSPROF) {
>> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +  }
>>     if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>>       return sysprof_error(L, status, NULL);
>>   
>> @@ -341,8 +350,12 @@ LJLIB_CF(misc_sysprof_report)
>>     struct luam_Sysprof_Counters counters = {};
>>     GCtab *data_tab = NULL;
>>     GCtab *count_tab = NULL;
>> -
>>     int status = luaM_sysprof_report(&counters);
>> +  if (!LJ_HASSYSPROF) {
>> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +  }
>> +
>>     if (status != PROFILE_SUCCESS)
>>       return sysprof_error(L, status, NULL);
>>   
>> @@ -381,6 +394,10 @@ LJLIB_CF(misc_memprof_start)
>>     const char *fname = strdata(lj_lib_checkstr(L, 1));
>>     struct profile_ctx *ctx;
>>     int memprof_status;
>> +  if (!LJ_HASMEMPROF) {
>> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +  }
>>   
>>     /*
>>     ** FIXME: more elegant solution with ctx.
>> @@ -432,6 +449,10 @@ LJLIB_CF(misc_memprof_start)
>>   LJLIB_CF(misc_memprof_stop)
>>   {
>>     int status = lj_memprof_stop(L);
>> +  if (!LJ_HASMEMPROF) {
>> +    const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +    return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +  }
>>     if (status != PROFILE_SUCCESS) {
>>       switch (status) {
>>       case PROFILE_ERRUSE:
>> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h
>> index b5c3a275..e26f5e38 100644
>> --- a/src/lj_errmsg.h
>> +++ b/src/lj_errmsg.h
>> @@ -193,6 +193,7 @@ 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")
>>   
>> +ERRDEF(PROF_DETAILS_DISABLED, "profiler is disabled")
>>   #undef ERRDEF
>>   
>>   /* Detecting unused error messages:
>> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>> new file mode 100644
>> index 00000000..de0aa136
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>> @@ -0,0 +1,22 @@
>> +local tap = require('tap')
>> +local test = tap.test('misclib-memprof-lapi-disabled'):skipcond({
>> +  ['Memprof is enabled'] = not os.getenv('LUAJIT_DISABLE_MEMPROF'),
>> +})
>> +
>> +test:plan(6)
>> +
>> +-- Attempt to start memprof when it is disabled.
>> +local res, err, errno = misc.memprof.start()
>> +test:is(res, nil, 'result status on start when memprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> +        'error on start when memprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
>> +
>> +-- Attempt to stop memprof when it is disabled.
>> +res, err, errno = misc.memprof.stop()
>> +test:is(res, nil, 'result status on stop when memprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> +        'error on stop when memprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
>> +
>> +test:done(true)
>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
>> new file mode 100644
>> index 00000000..2a9ce796
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
>> @@ -0,0 +1,29 @@
>> +local tap = require('tap')
>> +local test = tap.test('misclib-sysprof-lapi-disabled'):skipcond({
>> +  ['Sysprof is enabled'] = not os.getenv('LUAJIT_DISABLE_SYSPROF'),
>> +})
>> +
>> +test:plan(9)
>> +
>> +-- Attempt to start sysprof when sysprof is disabled.
>> +local res, err, errno = misc.sysprof.start()
>> +test:is(res, nil, 'result status on start when sysprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> +        'error on start when sysprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
>> +
>> +-- Attempt to stop sysprof when sysprof is disabled.
>> +res, err, errno = misc.sysprof.stop()
>> +test:is(res, nil, 'result status on stop when sysprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> +        'error on stop when sysprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
>> +
>> +-- Attempt to report when sysprof is disabled.
>> +res, err, errno = misc.sysprof.report()
>> +test:is(res, nil, 'result status on report when sysprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> +        'error on stop when sysprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
>> +
>> +test:done(true)
>> -- 
>> 2.43.0
>>

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

Re: [Tarantool-patches] [PATCH luajit 8/8][v3] memprof: set default path to profiling output file

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review!

On 24.02.2025 14:14, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, with a few small nits below.
>
> On 20.02.25, Sergey Bronnikov wrote:
>> sysprof has an optional parameter `path`, that sets a path to
>> the profiling output file. By default the path is `sysprof.bin`.
> Typo: s/default/default,/
Fixed.
>
>> `misc.memprof.start()` requires to set a path to profiling output
> Typo: s/to set/setting/
> Typo: s/profiling/the profiling/
Fixed.
>
>> file. The patch fixes this inconsistency by introducing a default
>> path to the memprof profiling output file - `memprof.bin`.
>> ---
>>   src/lib_misc.c                                |  5 ++-
>>   ...misclib-memprof-lapi-default-file.test.lua | 37 +++++++++++++++++++
>>   2 files changed, 41 insertions(+), 1 deletion(-)
>>   create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
>>
>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>> index d98cf3f0..92dc6e2a 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
> <snipped>
>
>> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
>> new file mode 100644
>> index 00000000..ae8a73c9
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
>> @@ -0,0 +1,37 @@
>> +local tap = require('tap')
>> +local test = tap.test('misc-memprof-lapi-default-file'):skipcond({
>> +  ['Memprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
>> +                                               jit.arch ~= 'x64',
>> +  ['Memprof is disabled'] = os.getenv('LUAJIT_DISABLE_MEMPROF'),
>> +})
>> +
>> +test:plan(1)
>> +
>> +local tools = require('utils.tools')
>> +
>> +test:test('default-output-file', function(subtest)
>> +
>> +subtest:plan(1)
>> +
>> +  local default_output_file = 'memprof.bin'
>> +  os.remove(default_output_file)
>> +
>> +  local _, _ = misc.memprof.start()
> Minor: Do we want to check that the profiler starts successfully?
> I suppose we should, since this is the expected behaviour for this
> feature. In case the profiler is not started (old behaviour) we would
> get an error from the branch below: profiler not running. This isn't a
> verbose definition of what goes wrong.

I don't get why we should check that profiler is started in a test for 
default output file.


> I suppose we may use `goto` here like the following:
>
> |   local res, err = misc.memprof.start()
> |   -- Should start successfully.
> |   if not res then
> |     goto err_handling
> |   end
> |
> |   res, err = misc.memprof.stop()
> |
> | ::err_handling::
> |   -- Want to cleanup carefully if something went wrong.
> |   if not res then
>
>> +
>> +  local res, err = misc.memprof.stop()
>> +
>> +  -- Want to cleanup carefully if something went wrong.
>> +  if not res then
>> +    os.remove(default_output_file)
>> +    error(err)
>> +  end
>> +
>> +  local profile_buf = tools.read_file(default_output_file)
>> +subtest:ok(profile_buf ~= nil and #profile_buf ~= 0,
>> +             'default output file is not empty')
>> +
>> +  -- We don't need it any more.
>> +  os.remove(default_output_file)
>> +end)
>> +
>> +test:done(true)
>> -- 
>> 2.43.0
>>

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

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

[Sergey Kaplun via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH luajit 7/8][v3] misc: specific message for disabled profilers

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey!

Changes applied and force-pushed.

Sergey

On 05.03.2025 10:55, Sergey Kaplun wrote:
> 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.

Sorry, missed this comment.

Fixed:

--- a/src/Makefile.dep.original
+++ b/src/Makefile.dep.original
@@ -30,7 +30,7 @@ lib_jit.o: lib_jit.c lua.h luaconf.h lauxlib.h 
lualib.h lj_obj.h lj_def.h \
  lib_math.o: lib_math.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \
   lj_def.h lj_arch.h lj_lib.h lj_vm.h lj_libdef.h
  lib_misc.o: lib_misc.c lua.h luaconf.h lmisclib.h lauxlib.h lj_obj.h \
- lj_def.h lj_arch.h lj_str.h lj_tab.h lj_lib.h lj_gc.h lj_err.h \
+ lj_def.h lj_arch.h lj_str.h lj_strfmt.h lj_tab.h lj_lib.h lj_gc.h 
lj_err.h \
   lj_errmsg.h lj_memprof.h lj_wbuf.h lj_libdef.h
  lib_os.o: lib_os.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h lj_def.h \
   lj_arch.h lj_gc.h lj_err.h lj_errmsg.h lj_buf.h lj_str.h lj_lib.h \

>
> <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.
Applied.
>
> ===================================================================
> 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 :)).

okay, removed:

@@ -85,11 +85,10 @@ test:ok(ok == false and err_msg:match("cannot open a 
file"),
  assert(misc.sysprof.stop())

  -- Not a table.
-res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")
+res, err = pcall(misc.sysprof.start, "NOT A TABLE")
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. Check parameter validation order
  -- (not strict documented behaviour).


>
> <snipped>
>

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

Re: [Tarantool-patches] [PATCH luajit 8/8][v3] memprof: set default path to profiling output file

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
See my answer below.

On 24.02.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review!

<snipped>

> >> ---
> >> +
> >> +  local default_output_file = 'memprof.bin'
> >> +  os.remove(default_output_file)
> >> +
> >> +  local _, _ = misc.memprof.start()
> > Minor: Do we want to check that the profiler starts successfully?
> > I suppose we should, since this is the expected behaviour for this
> > feature. In case the profiler is not started (old behaviour) we would
> > get an error from the branch below: profiler not running. This isn't a
> > verbose definition of what goes wrong.
> 
> I don't get why we should check that profiler is started in a test for 
> default output file.

If the `memprof.start()` call will fail in the test (for any reason),
the next call of `memprof.stop()` will return `false, "profiler is not
running"`. So, when debugging the test, we have no clue about the reason
for the not-started profiler, which is not very convenient.

> > I suppose we may use `goto` here like the following:
> >
> > |   local res, err = misc.memprof.start()
> > |   -- Should start successfully.
> > |   if not res then
> > |     goto err_handling
> > |   end
> > |
> > |   res, err = misc.memprof.stop()
> > |
> > | ::err_handling::
> > |   -- Want to cleanup carefully if something went wrong.
> > |   if not res then
> >
> >> +
> >> +  local res, err = misc.memprof.stop()
> >> +
> >> +  -- Want to cleanup carefully if something went wrong.
> >> +  if not res then
> >> +    os.remove(default_output_file)
> >> +    error(err)
> >> +  end

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 8/8][v3] memprof: set default path to profiling output file

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey!

Updated and force-pushed.

Sergey

On 05.03.2025 13:57, Sergey Kaplun wrote:
> Hi, Sergey!
> See my answer below.
>
> On 24.02.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for review!
> <snipped>
>
>>>> ---
>>>> +
>>>> +  local default_output_file = 'memprof.bin'
>>>> +  os.remove(default_output_file)
>>>> +
>>>> +  local _, _ = misc.memprof.start()
>>> Minor: Do we want to check that the profiler starts successfully?
>>> I suppose we should, since this is the expected behaviour for this
>>> feature. In case the profiler is not started (old behaviour) we would
>>> get an error from the branch below: profiler not running. This isn't a
>>> verbose definition of what goes wrong.
>> I don't get why we should check that profiler is started in a test for
>> default output file.
> If the `memprof.start()` call will fail in the test (for any reason),
> the next call of `memprof.stop()` will return `false, "profiler is not
> running"`. So, when debugging the test, we have no clue about the reason
> for the not-started profiler, which is not very convenient.

The patch below handles error from memprof start and stop:

diff --git 
a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua 
b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
index ae8a73c9..55b5c60b 100644
--- 
a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
+++ 
b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
@@ -16,21 +16,26 @@ test:test('default-output-file', function(subtest)
    local default_output_file = 'memprof.bin'
    os.remove(default_output_file)

-  local _, _ = misc.memprof.start()
-
-  local res, err = misc.memprof.stop()
+  local res, err = misc.memprof.start()
+  -- Want to cleanup carefully if something went wrong.
+  if not res then
+ test:fail('sysprof was not started: ' .. err)
+    os.remove(default_output_file)
+  end

+  res, err = misc.memprof.stop()
    -- Want to cleanup carefully if something went wrong.
    if not res then
+ test:fail('sysprof was not started: ' .. err)
      os.remove(default_output_file)
-    error(err)
    end

+
    local profile_buf = tools.read_file(default_output_file)
subtest:ok(profile_buf ~= nil and #profile_buf ~= 0,
               'default output file is not empty')

-  -- We don't need it any more.
+  -- We don't need it anymore.
    os.remove(default_output_file)
  end)


>
>>> I suppose we may use `goto` here like the following:
>>>
>>> |   local res, err = misc.memprof.start()
>>> |   -- Should start successfully.
>>> |   if not res then
>>> |     goto err_handling
>>> |   end
>>> |
>>> |   res, err = misc.memprof.stop()
>>> |
>>> | ::err_handling::
>>> |   -- Want to cleanup carefully if something went wrong.
>>> |   if not res then
>>>
>>>> +
>>>> +  local res, err = misc.memprof.stop()
>>>> +
>>>> +  -- Want to cleanup carefully if something went wrong.
>>>> +  if not res then
>>>> +    os.remove(default_output_file)
>>>> +    error(err)
>>>> +  end
> <snipped>
>

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

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

also added err_details to processing of PROFILE_ERRIO as discussed:

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 79cfaf7b..83669268 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -267,6 +267,7 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,

      status = set_output_path(path, opt);
      if (status != PROFILE_SUCCESS) {
+      *err_details = path;
        lj_mem_free(ctx->g, ctx, sizeof(*ctx));
        return status;
      }
@@ -293,7 +294,7 @@ static int sysprof_error(lua_State *L, int status, 
const char *err_details)
        lua_pushinteger(L, EINVAL);
        return 3;
      case PROFILE_ERRIO:
-      return luaL_fileresult(L, 0, NULL);
+      return luaL_fileresult(L, 0, err_details);
  #endif
      default:
        lj_assertL(0, "bad sysprof error %d", status);
diff --git 
a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua 
b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index 32ce75bc..9fa779a7 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -128,8 +128,9 @@ 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"),
-        "result status and error with bad path")
+test:ok(res == nil, "result status with bad path")
+local error_msg = ("%s: No such file or directory"):format(BAD_PATH)
+test:ok(err == error_msg, "error with bad path")
test:ok(type(errno) == "number", "errno with bad path")

  -- Path is not a string.

On 05.03.2025 10:55, Sergey Kaplun wrote:
> 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>
>

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

Re: [Tarantool-patches] [PATCH luajit 8/8][v3] memprof: set default path to profiling output file

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, with a final nit below.

On 05.03.25, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> Updated and force-pushed.
> 
> Sergey
> 
> On 05.03.2025 13:57, Sergey Kaplun wrote:

<snipped>

> -  local _, _ = misc.memprof.start()
> -
> -  local res, err = misc.memprof.stop()
> +  local res, err = misc.memprof.start()
> +  -- Want to cleanup carefully if something went wrong.
> +  if not res then
> + test:fail('sysprof was not started: ' .. err)
> +    os.remove(default_output_file)
> +  end
> 
> +  res, err = misc.memprof.stop()
>     -- Want to cleanup carefully if something went wrong.
>     if not res then
> + test:fail('sysprof was not started: ' .. err)

Looks like it should be: "was not stopped successfully".

>       os.remove(default_output_file)
> -    error(err)
>     end
> 
> +
>     local profile_buf = tools.read_file(default_output_file)
> subtest:ok(profile_buf ~= nil and #profile_buf ~= 0,
>                'default output file is not empty')
> 
> -  -- We don't need it any more.
> +  -- We don't need it anymore.
>     os.remove(default_output_file)
>   end)
> 

<snipped>


-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Sergey,
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 8/8][v3] memprof: set default path to profiling output file

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

On 05.03.2025 18:12, Sergey Kaplun wrote:
>> <snipped>
>>
>> +  res, err = misc.memprof.stop()
>>      -- Want to cleanup carefully if something went wrong.
>>      if not res then
>> +test:fail('sysprof was not started: ' .. err)
> Looks like it should be: "was not stopped successfully".

Fixed, thanks!

--- 
a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
+++ 
b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
@@ -26,7 +26,7 @@ test:test('default-output-file', function(subtest)
    res, err = misc.memprof.stop()
    -- Want to cleanup carefully if something went wrong.
    if not res then
- test:fail('sysprof was not started: ' .. err)
+ test:fail('sysprof was not stopped: ' .. err)
      os.remove(default_output_file)
    end

>>        os.remove(default_output_file)
>> -    error(err)
>>      end
>>
>> +
>>      local profile_buf = tools.read_file(default_output_file)
>> subtest:ok(profile_buf ~= nil and #profile_buf ~= 0,
>>                 'default output file is not empty')
>>
>> -  -- We don't need it any more.
>> +  -- We don't need it anymore.
>>      os.remove(default_output_file)
>>    end)
>>
> <snipped>
>
>

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

Re: [Tarantool-patches] [PATCH luajit 0/8][v3] Fix profilers issues

[Sergey Kaplun via Tarantool-patches]

Sergey,

I've applied the patch set into all long-term branches in
tarantool/luajit and bumped a new version in master [1],
release/3.3 [2], release/3.2 [3] and release/2.11 [4].

[1]: https://github.com/tarantool/tarantool/pull/11235
[2]: https://github.com/tarantool/tarantool/pull/11236
[3]: https://github.com/tarantool/tarantool/pull/11237
[4]: https://github.com/tarantool/tarantool/pull/11238

-- 
Best regards,
Sergey Kaplun