[Tarantool-patches] [PATCH luajit 0/7][v2] Fix profilers issues

[Tarantool-patches] [PATCH luajit 0/7][v2] 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.

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.

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

Sergey Bronnikov (7):
  test: add descriptions to sysprof testcases
  sysprof: align test title with test filename
  sysprof: fix typo in the comment
  sysprof: introduce specific errors and default mode
  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                                | 112 +++++++++++++-----
 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 |  41 +++++++
 .../misclib-memprof-lapi-disabled.test.lua    |  22 ++++
 .../profilers/misclib-memprof-lapi.test.lua   |  15 +--
 .../misclib-sysprof-lapi-disabled.test.lua    |  29 +++++
 .../profilers/misclib-sysprof-lapi.test.lua   |  83 ++++++++++---
 10 files changed, 264 insertions(+), 55 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.34.1

[Tarantool-patches] [PATCH luajit 1/7][v2] 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.34.1

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

[Tarantool-patches] [PATCH luajit 2/7] sysprof: 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-sysprof-lapi.test.lua | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks fro the patch!
LGTM, except a minor nits below.

> sysprof: align test title with test filename

It should be `test:`, since we don't change the sysprof component
itself, only test for it.

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

> ---
>  test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 2 +-

Should it be fixed for the memprof too?

>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review.

On 18.02.2025 14:10, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks fro the patch!
> LGTM, except a minor nits below.
>
>> sysprof: align test title with test filename
> It should be `test:`, since we don't change the sysprof component
> itself, only test for it.
Fixed.
> On 13.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).

No. The test introduced in 2593a9f83072ca999d5f528e1da481e8ba51d5b8,

when test titles were not supported.

>> ---
>>   test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 2 +-
> Should it be fixed for the memprof too?

Fixed:

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

>
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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
> <snipped>
>
>> -- 
>> 2.34.1
>>

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

[Tarantool-patches] [PATCH luajit 3/7][v2] 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.34.1

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

[Tarantool-patches] [PATCH luajit 4/7][v2] 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
discourage sysprof users and make using sysprof more complicated.
The patch sets 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                                | 80 +++++++++++++------
 src/lj_errmsg.h                               |  5 ++
 .../profilers/misclib-sysprof-lapi.test.lua   | 48 +++++++++--
 3 files changed, 100 insertions(+), 33 deletions(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 5b7a4b62..d71904e4 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -163,6 +163,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf)
 
 /* The default profiling interval equals to 10 ms. */
 #define SYSPROF_DEFAULT_INTERVAL 10
+#define SYSPROF_DEFAULT_MODE "D"
 #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
 
 static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {
@@ -177,21 +178,41 @@ 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 != 1) {
+    opt->mode = LUAM_SYSPROF_DEFAULT;
+    opt->interval = SYSPROF_DEFAULT_INTERVAL;
+    goto set_path;
+  }
+
+  if (!lua_istable(L, 1)) {
+    *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADTABLE);
+    return PROFILE_ERRUSE;
+  }
+
+  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 +225,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,12 +236,16 @@ 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;
     }
   }
 
+set_path:
+
   /* Get output path. */
   if (opt->mode != LUAM_SYSPROF_DEFAULT)
   {
@@ -230,8 +256,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
       path = strVdata(pathtv);
 
@@ -251,29 +279,28 @@ 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) {
+        lua_pushstring(L, ": ");
+        lua_pushstring(L, err_details);
+        lua_concat(L, 3);
+      }
       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) {
+        lua_pushstring(L, ": ");
+        lua_pushstring(L, err_details);
+        lua_concat(L, 3);
+      }
       lua_pushinteger(L, EINVAL);
       return 3;
     case PROFILE_ERRIO:
@@ -291,15 +318,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 +338,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 +353,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..7622323a 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(33)
 
 jit.off()
 -- XXX: Run JIT tuning functions in a safe frame to avoid errors
@@ -65,10 +65,25 @@ end
 
 -- 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")
+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 = misc.sysprof.start("NOT A TABLE")
+test:ok(res == nil, "res with not a table")
+test:ok(err:match("profiler expects a table with parameters"),
+        "error with not a table")
+test:ok(type(errno) == "number", "errno with not a table")
+
 -- Already running.
 res, err = misc.sysprof.start{ mode = "D" }
 assert(res, err)
@@ -92,11 +107,30 @@ 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.
+-- Bad interval (-1).
 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")
-test:ok(type(errno) == "number", "errno with bad interval")
+test:is(res, nil, "result status and error with bad interval -1")
+test:ok(err:match("profiler interval must be greater than 1"),
+        "error with bad interval -1")
+test:ok(type(errno) == "number", "errno with bad interval -1")
+
+-- Bad interval (0).
+res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 }
+test:ok(res == nil, "res with bad interval 0")
+test:ok(err:match("profiler interval must be greater than 1"),
+        "error with bad interval 0")
+test:ok(type(errno) == "number", "errno with bad interval 0")
+
+-- Good interval (1).
+res, err, errno = misc.sysprof.start{
+    mode = "C",
+    interval = 1,
+    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
 
-- 
2.34.1

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
After refactoring the code become more readable, thanks!
Now I have a few ideas, see my comments below.

On 13.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
> discourage sysprof users and make using sysprof more complicated.

Typo: s/discourage/discourages/
Typo: s/make/makes/

> The patch sets default profiling mode ("D", that shows only

Typo: s/default/the default/

> 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                                | 80 +++++++++++++------
>  src/lj_errmsg.h                               |  5 ++
>  .../profilers/misclib-sysprof-lapi.test.lua   | 48 +++++++++--
>  3 files changed, 100 insertions(+), 33 deletions(-)
> 
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index 5b7a4b62..d71904e4 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -163,6 +163,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf)
>  
>  /* The default profiling interval equals to 10 ms. */
>  #define SYSPROF_DEFAULT_INTERVAL 10
> +#define SYSPROF_DEFAULT_MODE "D"
>  #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
>  
>  static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {
> @@ -177,21 +178,41 @@ 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 != 1) {

I suppose this should be `n == 0`. Otherwise we will observe the
following behaviour:
| src/luajit -e 'print(misc.sysprof.start(1, 2, 3))'
| true

> +    opt->mode = LUAM_SYSPROF_DEFAULT;
> +    opt->interval = SYSPROF_DEFAULT_INTERVAL;
> +    goto set_path;

I suppose it is better to set path explicitly here and goto ctx_allocate;

> +  }
> +
> +  if (!lua_istable(L, 1)) {

The more I think about it, the more it looks like a bug -- all library
functions raise an error when they get a bad parameter type.

Maybe we should just use `lj_lib_checktab()` here? This will help the
user to find an error and be consistent with memprof behaviour. So,
bugfix isn't breaking change, since something is working incorrectly.
Plus, as a bonus we don't need to introduce the new error with the same
meaning we have already.

OTOH, if you are against it, we may leave it as is, but check it via
`tvistab()` instead.

What do you think?

And the same should be done with the parameters in table content --
their type should be checked at once and the corresponding error should
be raised. I suppose we can introduce a local (inside this translation
unit) helper `key_opt_type()` here -- we will check a type for the keys
`path`, `mode`, `interval` and raise the user-friendly error like:

`bad key 'mode' in 'start' argument (string expected, got table)`.
See how it is done in `lj_lib_check*()` for inspiration.

> +    *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADTABLE);
> +    return PROFILE_ERRUSE;
> +  }
> +
> +  GCtab *options = lj_lib_checktab(L, 1);

This check is excess, let's move it above, as I suggested.

>  
>    /* 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') {

Minor: It's better to use `(strlen(mode) == 0) || mode[1] != '\0'` -- we
don't got further in code below, just early return here.

> +        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
> +        return PROFILE_ERRUSE;
> +      }
>      }
>  

<snipped>

> +set_path:
> +
>    /* Get output path. */
>    if (opt->mode != LUAM_SYSPROF_DEFAULT)
>    {
> @@ -230,8 +256,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)) {

It is better to wrap if and else branches too like the following:
| if () {
| } else if () {
| } else {
| }

> +      *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);
>        return PROFILE_ERRUSE;
> +    }
>      else
>        path = strVdata(pathtv);
>  
> @@ -251,29 +279,28 @@ 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) {
> +        lua_pushstring(L, ": ");

I suppose we may use string formatting here instead of concatenation Lua
C API call:
See usage of `lj_strfmt_pushf()` for the details.
Also, we may use nice helper to avoid code copy-pasting.

> +        lua_pushstring(L, err_details);
> +        lua_concat(L, 3);
> +      }
>        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) {
> +        lua_pushstring(L, ": ");
> +        lua_pushstring(L, err_details);
> +        lua_concat(L, 3);
> +      }
>        lua_pushinteger(L, EINVAL);
>        return 3;
>      case PROFILE_ERRIO:
> @@ -291,15 +318,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")
> +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
> +

These changes should be under the LJ_HASSYSPROF ifdef.
Also, I suggest the following naming:
| SYSPROF_BADMODE
| SYSPROF_BADINTERVAL
With this it will be obviour that they are sysprof-specific and not too
long.

>  #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..7622323a 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(33)
>  
>  jit.off()
>  -- XXX: Run JIT tuning functions in a safe frame to avoid errors
> @@ -65,10 +65,25 @@ end
>  
>  -- 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")
> +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")

I would rather still check matching with `profiler misuse:` error (as a
separate testcase). Here and below. But it's up to you. Feel free to
ignore.

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

This should be changed to the pcall() after the changes I suggested.

> +res, err, errno = misc.sysprof.start("NOT A TABLE")
> +test:ok(res == nil, "res with not a table")
> +test:ok(err:match("profiler expects a table with parameters"),
> +        "error with not a table")
> +test:ok(type(errno) == "number", "errno with not a table")

Please also check the case with multiple arguments (2 cases: all
invalid, and only first valid).

> +
>  -- Already running.
>  res, err = misc.sysprof.start{ mode = "D" }
>  assert(res, err)
> @@ -92,11 +107,30 @@ 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.
> +-- Bad interval (-1).
>  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")
> -test:ok(type(errno) == "number", "errno with bad interval")
> +test:is(res, nil, "result status and error with bad interval -1")
> +test:ok(err:match("profiler interval must be greater than 1"),
> +        "error with bad interval -1")
> +test:ok(type(errno) == "number", "errno with bad interval -1")
> +
> +-- Bad interval (0).
> +res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 }
> +test:ok(res == nil, "res with bad interval 0")
> +test:ok(err:match("profiler interval must be greater than 1"),
> +        "error with bad interval 0")
> +test:ok(type(errno) == "number", "errno with bad interval 0")
> +
> +-- Good interval (1).
> +res, err, errno = misc.sysprof.start{

Minor: Please use brackets for functions call.

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

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review!

On 18.02.2025 18:43, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> After refactoring the code become more readable, thanks!
> Now I have a few ideas, see my comments below.
>
> On 13.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
>> discourage sysprof users and make using sysprof more complicated.
> Typo: s/discourage/discourages/
> Typo: s/make/makes/
Fixed.
>
>> The patch sets default profiling mode ("D", that shows only
> Typo: s/default/the default/
Fixed.
>
>> 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                                | 80 +++++++++++++------
>>   src/lj_errmsg.h                               |  5 ++
>>   .../profilers/misclib-sysprof-lapi.test.lua   | 48 +++++++++--
>>   3 files changed, 100 insertions(+), 33 deletions(-)
>>
>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>> index 5b7a4b62..d71904e4 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -163,6 +163,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf)
>>   
>>   /* The default profiling interval equals to 10 ms. */
>>   #define SYSPROF_DEFAULT_INTERVAL 10
>> +#define SYSPROF_DEFAULT_MODE "D"
>>   #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
>>   
>>   static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {
>> @@ -177,21 +178,41 @@ 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 != 1) {
> I suppose this should be `n == 0`. Otherwise we will observe the
> following behaviour:
> | src/luajit -e 'print(misc.sysprof.start(1, 2, 3))'
> | true
Right, fixed.
>> +    opt->mode = LUAM_SYSPROF_DEFAULT;
>> +    opt->interval = SYSPROF_DEFAULT_INTERVAL;
>> +    goto set_path;
> I suppose it is better to set path explicitly here and goto ctx_allocate;

The code that make a jump to the middle of basic block smells bad.

It makes control flow more complicated, and benefits are not obvious.

>
>> +  }
>> +
>> +  if (!lua_istable(L, 1)) {
> The more I think about it, the more it looks like a bug -- all library
> functions raise an error when they get a bad parameter type.
>
> Maybe we should just use `lj_lib_checktab()` here? This will help the
> user to find an error and be consistent with memprof behaviour. So,
> bugfix isn't breaking change, since something is working incorrectly.
> Plus, as a bonus we don't need to introduce the new error with the same
> meaning we have already.
>
> OTOH, if you are against it, we may leave it as is, but check it via
> `tvistab()` instead.
>
> What do you think?
>
> And the same should be done with the parameters in table content --
> their type should be checked at once and the corresponding error should
> be raised. I suppose we can introduce a local (inside this translation
> unit) helper `key_opt_type()` here -- we will check a type for the keys
> `path`, `mode`, `interval` and raise the user-friendly error like:

What for? The reason of this patch series is bad error handling in 
profilers.

The value behind this refactoring is not obvious. Please elaborate.

This code was imperfect for about 2-3 years and this was ok for everyone

who added it initially. I don't want to fix all bad places here, just 
improve error messages visible by end users.

>
> `bad key 'mode' in 'start' argument (string expected, got table)`.
> See how it is done in `lj_lib_check*()` for inspiration.
>
>> +    *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADTABLE);
>> +    return PROFILE_ERRUSE;
>> +  }
>> +
>> +  GCtab *options = lj_lib_checktab(L, 1);
> This check is excess, let's move it above, as I suggested.

it is not a check, it's a function that retrieves a table from the stack.

>
>>   
>>     /* 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') {
> Minor: It's better to use `(strlen(mode) == 0) || mode[1] != '\0'` -- we
> don't got further in code below, just early return here.

Fixed.


>
>> +        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
>> +        return PROFILE_ERRUSE;
>> +      }
>>       }
>>   
> <snipped>
>
>> +set_path:
>> +
>>     /* Get output path. */
>>     if (opt->mode != LUAM_SYSPROF_DEFAULT)
>>     {
>> @@ -230,8 +256,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)) {
> It is better to wrap if and else branches too like the following:
> | if () {
> | } else if () {
> | } else {
> | }

It's funny, on the previous review brackets were excess [1].

1. https://lists.tarantool.org/tarantool-patches/Z6XlUsKqu92__8fL@root/T/#t

Brackets are not needed in the first and third blocks, if we're 
following a rule "brackets for blocks with more than one statements".
>
>> +      *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);
>>         return PROFILE_ERRUSE;
>> +    }
>>       else
>>         path = strVdata(pathtv);
>>   
>> @@ -251,29 +279,28 @@ 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) {
>> +        lua_pushstring(L, ": ");
> I suppose we may use string formatting here instead of concatenation Lua
> C API call:
> See usage of `lj_strfmt_pushf()` for the details.

Sure, we may, but what for?

Previous functions are a part of Lua C API (lua_pushnil, lua_pushstring),

why do we need to mix Lua C functions with this LJ-specific function?

> Also, we may use nice helper to avoid code copy-pasting.
>
>> +        lua_pushstring(L, err_details);
>> +        lua_concat(L, 3);
>> +      }
>>         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) {
>> +        lua_pushstring(L, ": ");
>> +        lua_pushstring(L, err_details);
>> +        lua_concat(L, 3);
>> +      }
>>         lua_pushinteger(L, EINVAL);
>>         return 3;
>>       case PROFILE_ERRIO:
>> @@ -291,15 +318,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")
>> +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
>> +
> These changes should be under the LJ_HASSYSPROF ifdef.
> Also, I suggest the following naming:
> | SYSPROF_BADMODE
> | SYSPROF_BADINTERVAL
> With this it will be obviour that they are sysprof-specific and not too
> long.

Imagine you then introduce a table and interval number for memprof.

Will you rename constants back? I dislike your suggestion.

>
>>   #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..7622323a 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(33)
>>   
>>   jit.off()
>>   -- XXX: Run JIT tuning functions in a safe frame to avoid errors
>> @@ -65,10 +65,25 @@ end
>>   
>>   -- Wrong profiling mode.
>>   local res, err, errno = misc.sysprof.start{ mode = "A" }
>> -test:ok(res == nil anderr:match("profiler misuse"),
>> -        "result status with wrong profiling 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")
> I would rather still check matching with `profiler misuse:` error (as a
> separate testcase). Here and below. But it's up to you. Feel free to
> ignore.
I think that checking specific part of error message is enough.
>
>>   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.
> This should be changed to the pcall() after the changes I suggested.
>
>> +res, err, errno = misc.sysprof.start("NOT A TABLE")
>> +test:ok(res == nil, "res with not a table")
>> +test:ok(err:match("profiler expects a table with parameters"),
>> +        "error with not a table")
>> +test:ok(type(errno) == "number", "errno with not a table")
> Please also check the case with multiple arguments (2 cases: all
> invalid, and only first valid).

Added:

254: ok - res with all invalid parameters
254: ok - error with all invalid parameters
254: ok - errno with all invalid parameters
254: ok - res with all invalid parameters except the first one
254: ok - error with all invalid parameters except the first one
254: ok - errno with all invalid parameters except the first one


>
>> +
>>   -- Already running.
>>   res, err = misc.sysprof.start{ mode = "D" }
>>   assert(res, err)
>> @@ -92,11 +107,30 @@test:ok(res == nil anderr:match("No such file or directory"),
>>           "result status and error with bad path")
>>   test:ok(type(errno) == "number", "errno with bad path")
>>   
>> --- Bad interval.
>> +-- Bad interval (-1).
>>   res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
>> -test:ok(res == nil anderr:match("profiler misuse"),
>> -        "result status and error with bad interval")
>> -test:ok(type(errno) == "number", "errno with bad interval")
>> +test:is(res, nil, "result status and error with bad interval -1")
>> +test:ok(err:match("profiler interval must be greater than 1"),
>> +        "error with bad interval -1")
>> +test:ok(type(errno) == "number", "errno with bad interval -1")
>> +
>> +-- Bad interval (0).
>> +res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 }
>> +test:ok(res == nil, "res with bad interval 0")
>> +test:ok(err:match("profiler interval must be greater than 1"),
>> +        "error with bad interval 0")
>> +test:ok(type(errno) == "number", "errno with bad interval 0")
>> +
>> +-- Good interval (1).
>> +res, err, errno = misc.sysprof.start{
> Minor: Please use brackets for functions call.

Case without brackets is more popular:

   local res, err = misc.sysprof.start(opts)
   res,err = misc.sysprof.stop()
local res, err, errno = misc.sysprof.start{ mode = "A" }
res, err, errno = misc.sysprof.start{}
assert(misc.sysprof.stop())
res, err, errno = misc.sysprof.start("NOT A TABLE")
res, err = misc.sysprof.start{ mode = "D" }
res, err, errno = misc.sysprof.start{ mode = "D" }
res, err = misc.sysprof.stop()
res, err, errno = misc.sysprof.stop()
res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 }
res, err, errno = misc.sysprof.start{
misc.sysprof.stop()
local report = misc.sysprof.report()
report = misc.sysprof.report()

proposed change will make coding style inconsistent in the file

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

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

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
Adds some comments related to our offline discussion below.

On 19.02.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review!
> 
> On 18.02.2025 18:43, Sergey Kaplun via Tarantool-patches wrote:
> > Hi, Sergey!
> > Thanks for the fixes!
> > After refactoring the code become more readable, thanks!
> > Now I have a few ideas, see my comments below.
> >
> > On 13.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
> >> discourage sysprof users and make using sysprof more complicated.
> > Typo: s/discourage/discourages/
> > Typo: s/make/makes/
> Fixed.
> >
> >> The patch sets default profiling mode ("D", that shows only
> > Typo: s/default/the default/
> Fixed.
> >
> >> 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                                | 80 +++++++++++++------
> >>   src/lj_errmsg.h                               |  5 ++
> >>   .../profilers/misclib-sysprof-lapi.test.lua   | 48 +++++++++--
> >>   3 files changed, 100 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/src/lib_misc.c b/src/lib_misc.c
> >> index 5b7a4b62..d71904e4 100644
> >> --- a/src/lib_misc.c
> >> +++ b/src/lib_misc.c
> >> @@ -163,6 +163,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf)
> >>   
> >>   /* The default profiling interval equals to 10 ms. */
> >>   #define SYSPROF_DEFAULT_INTERVAL 10
> >> +#define SYSPROF_DEFAULT_MODE "D"
> >>   #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
> >>   
> >>   static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {
> >> @@ -177,21 +178,41 @@ 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 != 1) {
> > I suppose this should be `n == 0`. Otherwise we will observe the
> > following behaviour:
> > | src/luajit -e 'print(misc.sysprof.start(1, 2, 3))'
> > | true
> Right, fixed.
> >> +    opt->mode = LUAM_SYSPROF_DEFAULT;
> >> +    opt->interval = SYSPROF_DEFAULT_INTERVAL;
> >> +    goto set_path;
> > I suppose it is better to set path explicitly here and goto ctx_allocate;
> 
> The code that make a jump to the middle of basic block smells bad.
> 
> It makes control flow more complicated, and benefits are not obvious.

I suppose we may just return here since we don't get into the if
condition under the label below.

> 
> >
> >> +  }
> >> +
> >> +  if (!lua_istable(L, 1)) {
> > The more I think about it, the more it looks like a bug -- all library
> > functions raise an error when they get a bad parameter type.
> >
> > Maybe we should just use `lj_lib_checktab()` here? This will help the
> > user to find an error and be consistent with memprof behaviour. So,
> > bugfix isn't breaking change, since something is working incorrectly.
> > Plus, as a bonus we don't need to introduce the new error with the same
> > meaning we have already.
> >
> > OTOH, if you are against it, we may leave it as is, but check it via
> > `tvistab()` instead.
> >
> > What do you think?
> >
> > And the same should be done with the parameters in table content --
> > their type should be checked at once and the corresponding error should
> > be raised. I suppose we can introduce a local (inside this translation
> > unit) helper `key_opt_type()` here -- we will check a type for the keys
> > `path`, `mode`, `interval` and raise the user-friendly error like:
> 
> What for? The reason of this patch series is bad error handling in 
> profilers.
> 
> The value behind this refactoring is not obvious. Please elaborate.
> 
> This code was imperfect for about 2-3 years and this was ok for everyone
> 
> who added it initially. I don't want to fix all bad places here, just 
> improve error messages visible by end users.

Discussed offline only to fix the bug with `lua_istable()` vs
`lj_lib_checktab()`. The options validation is OK it is the current
state.

> 
> >
> > `bad key 'mode' in 'start' argument (string expected, got table)`.
> > See how it is done in `lj_lib_check*()` for inspiration.
> >
> >> +    *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADTABLE);
> >> +    return PROFILE_ERRUSE;
> >> +  }
> >> +
> >> +  GCtab *options = lj_lib_checktab(L, 1);
> > This check is excess, let's move it above, as I suggested.
> 
> it is not a check, it's a function that retrieves a table from the stack.

See the comment above.

<snipped>

> >> +set_path:
> >> +
> >>     /* Get output path. */
> >>     if (opt->mode != LUAM_SYSPROF_DEFAULT)
> >>     {
> >> @@ -230,8 +256,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)) {
> > It is better to wrap if and else branches too like the following:
> > | if () {
> > | } else if () {
> > | } else {
> > | }
> 
> It's funny, on the previous review brackets were excess [1].
> 
> 1. https://lists.tarantool.org/tarantool-patches/Z6XlUsKqu92__8fL@root/T/#t

This was about single if usage not `if-else if-else` statements.

> 
> Brackets are not needed in the first and third blocks, if we're 
> following a rule "brackets for blocks with more than one statements".
> >
> >> +      *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);
> >>         return PROFILE_ERRUSE;
> >> +    }
> >>       else
> >>         path = strVdata(pathtv);
> >>   
> >> @@ -251,29 +279,28 @@ 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) {
> >> +        lua_pushstring(L, ": ");
> > I suppose we may use string formatting here instead of concatenation Lua
> > C API call:
> > See usage of `lj_strfmt_pushf()` for the details.
> 
> Sure, we may, but what for?
> 
> Previous functions are a part of Lua C API (lua_pushnil, lua_pushstring),
> 
> why do we need to mix Lua C functions with this LJ-specific function?

We don't bother the Lua stack with excess pushes and additional
concatenation, if we know exactly how many values on the stack we need.

<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")
> >> +
> > These changes should be under the LJ_HASSYSPROF ifdef.
> > Also, I suggest the following naming:
> > | SYSPROF_BADMODE
> > | SYSPROF_BADINTERVAL
> > With this it will be obviour that they are sysprof-specific and not too
> > long.
> 
> Imagine you then introduce a table and interval number for memprof.
> 
> Will you rename constants back? I dislike your suggestion.

OK, let's leave it as is.

> 
> >
> >>   #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..7622323a 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(33)
> >>   
> >>   jit.off()
> >>   -- XXX: Run JIT tuning functions in a safe frame to avoid errors
> >> @@ -65,10 +65,25 @@ end
> >>   
> >>   -- Wrong profiling mode.
> >>   local res, err, errno = misc.sysprof.start{ mode = "A" }
> >> -test:ok(res == nil anderr:match("profiler misuse"),
> >> -        "result status with wrong profiling 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")
> > I would rather still check matching with `profiler misuse:` error (as a
> > separate testcase). Here and below. But it's up to you. Feel free to
> > ignore.
> I think that checking specific part of error message is enough.

Ok.

<snipped>

> >> @@ -92,11 +107,30 @@test:ok(res == nil anderr:match("No such file or directory"),
> >>           "result status and error with bad path")
> >>   test:ok(type(errno) == "number", "errno with bad path")
> >>   
> >> --- Bad interval.
> >> +-- Bad interval (-1).
> >>   res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
> >> -test:ok(res == nil anderr:match("profiler misuse"),
> >> -        "result status and error with bad interval")
> >> -test:ok(type(errno) == "number", "errno with bad interval")
> >> +test:is(res, nil, "result status and error with bad interval -1")
> >> +test:ok(err:match("profiler interval must be greater than 1"),
> >> +        "error with bad interval -1")
> >> +test:ok(type(errno) == "number", "errno with bad interval -1")
> >> +
> >> +-- Bad interval (0).
> >> +res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 }
> >> +test:ok(res == nil, "res with bad interval 0")
> >> +test:ok(err:match("profiler interval must be greater than 1"),
> >> +        "error with bad interval 0")
> >> +test:ok(type(errno) == "number", "errno with bad interval 0")
> >> +
> >> +-- Good interval (1).
> >> +res, err, errno = misc.sysprof.start{
> > Minor: Please use brackets for functions call.
> 
> Case without brackets is more popular:

<snipped>

> proposed change will make coding style inconsistent in the file

OK, my bad.

> 
> >

<snipped>

> >> -- 
> >> 2.34.1
> >>

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

On 19.02.2025 12:34, Sergey Bronnikov via Tarantool-patches wrote:
>
> Hi, Sergey,
>
> thanks for review!
>
> On 18.02.2025 18:43, Sergey Kaplun via Tarantool-patches wrote:
>> Hi, Sergey!
>> Thanks for the fixes!
>> After refactoring the code become more readable, thanks!
>> Now I have a few ideas, see my comments below.
>>
>> On 13.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
>>> discourage sysprof users and make using sysprof more complicated.
>> Typo: s/discourage/discourages/
>> Typo: s/make/makes/
> Fixed.
>>> The patch sets default profiling mode ("D", that shows only
>> Typo: s/default/the default/
> Fixed.
>>> 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                                | 80 +++++++++++++------
>>>   src/lj_errmsg.h                               |  5 ++
>>>   .../profilers/misclib-sysprof-lapi.test.lua   | 48 +++++++++--
>>>   3 files changed, 100 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>>> index 5b7a4b62..d71904e4 100644
>>> --- a/src/lib_misc.c
>>> +++ b/src/lib_misc.c
>>> @@ -163,6 +163,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf)
>>>   
>>>   /* The default profiling interval equals to 10 ms. */
>>>   #define SYSPROF_DEFAULT_INTERVAL 10
>>> +#define SYSPROF_DEFAULT_MODE "D"
>>>   #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin"
>>>   
>>>   static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {
>>> @@ -177,21 +178,41 @@ 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 != 1) {
>> I suppose this should be `n == 0`. Otherwise we will observe the
>> following behaviour:
>> | src/luajit -e 'print(misc.sysprof.start(1, 2, 3))'
>> | true
> Right, fixed.
>>> +    opt->mode = LUAM_SYSPROF_DEFAULT;
>>> +    opt->interval = SYSPROF_DEFAULT_INTERVAL;
>>> +    goto set_path;
>> I suppose it is better to set path explicitly here and goto ctx_allocate;
>
> The code that make a jump to the middle of basic block smells bad.
>
> It makes control flow more complicated, and benefits are not obvious.
>
Verbally decided to fix as the following:

--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -184,7 +184,7 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
    if (n == 0) {
      opt->mode = LUAM_SYSPROF_DEFAULT;
      opt->interval = SYSPROF_DEFAULT_INTERVAL;
-    goto set_path;
+    return PROFILE_SUCCESS;
    }

    if (!lua_istable(L, 1)) {
@@ -244,8 +244,6 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
      }
    }

-set_path:
-
    /* Get output path. */
    if (opt->mode != LUAM_SYSPROF_DEFAULT)
    {

>>> +  }
>>> +
>>> +  if (!lua_istable(L, 1)) {
>> The more I think about it, the more it looks like a bug -- all library
>> functions raise an error when they get a bad parameter type.
>>
>> Maybe we should just use `lj_lib_checktab()` here? This will help the
>> user to find an error and be consistent with memprof behaviour. So,
>> bugfix isn't breaking change, since something is working incorrectly.
>> Plus, as a bonus we don't need to introduce the new error with the same
>> meaning we have already.
>>
>> OTOH, if you are against it, we may leave it as is, but check it via
>> `tvistab()` instead.
>>
>> What do you think?
>>
>> And the same should be done with the parameters in table content --
>> their type should be checked at once and the corresponding error should
>> be raised. I suppose we can introduce a local (inside this translation
>> unit) helper `key_opt_type()` here -- we will check a type for the keys
>> `path`, `mode`, `interval` and raise the user-friendly error like:
>
> What for? The reason of this patch series is bad error handling in 
> profilers.
>
> The value behind this refactoring is not obvious. Please elaborate.
>
> This code was imperfect for about 2-3 years and this was ok for everyone
>
> who added it initially. I don't want to fix all bad places here, just 
> improve error messages visible by end users.
>
Verbally discussed making as the following:


--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -188,11 +188,6 @@ static int parse_sysprof_opts(lua_State *L, struct 
luam_Sysprof_Options *opt,
      return PROFILE_SUCCESS;
    }

-  if (!lua_istable(L, 1)) {
-    *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADTABLE);
-    return PROFILE_ERRUSE;
-  }
-
    GCtab *options = lj_lib_checktab(L, 1);

    /* Get profiling mode. */

>> `bad key 'mode' in 'start' argument (string expected, got table)`.
>> See how it is done in `lj_lib_check*()` for inspiration.
>>
>>> +    *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADTABLE);
>>> +    return PROFILE_ERRUSE;
>>> +  }
>>> +
>>> +  GCtab *options = lj_lib_checktab(L, 1);
>> This check is excess, let's move it above, as I suggested.
>
> it is not a check, it's a function that retrieves a table from the stack.
>
>>>   
>>>     /* 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') {
>> Minor: It's better to use `(strlen(mode) == 0) || mode[1] != '\0'` -- we
>> don't got further in code below, just early return here.
>
> Fixed.
>
>
>>> +        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
>>> +        return PROFILE_ERRUSE;
>>> +      }
>>>       }
>>>   
>> <snipped>
>>
>>> +set_path:
>>> +
>>>     /* Get output path. */
>>>     if (opt->mode != LUAM_SYSPROF_DEFAULT)
>>>     {
>>> @@ -230,8 +256,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)) {
>> It is better to wrap if and else branches too like the following:
>> | if () {
>> | } else if () {
>> | } else {
>> | }
>
> It's funny, on the previous review brackets were excess [1].
>
> 1. 
> https://lists.tarantool.org/tarantool-patches/Z6XlUsKqu92__8fL@root/T/#t
>
> Brackets are not needed in the first and third blocks, if we're 
> following a rule "brackets for blocks with more than one statements".
>>> +      *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);
>>>         return PROFILE_ERRUSE;
>>> +    }
>>>       else
>>>         path = strVdata(pathtv);
>>>   
>>> @@ -251,29 +279,28 @@ 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) {
>>> +        lua_pushstring(L, ": ");
>> I suppose we may use string formatting here instead of concatenation Lua
>> C API call:
>> See usage of `lj_strfmt_pushf()` for the details.
>
> Sure, we may, but what for?
>
> Previous functions are a part of Lua C API (lua_pushnil, lua_pushstring),
>
> why do we need to mix Lua C functions with this LJ-specific function?
>
Verbally decided to do as the following:

@@ -283,22 +282,20 @@ 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) {
-        lua_pushstring(L, ": ");
-        lua_pushstring(L, err_details);
-        lua_concat(L, 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) {
-        lua_pushstring(L, ": ");
-        lua_pushstring(L, err_details);
-        lua_concat(L, 3);
+        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;

>> Also, we may use nice helper to avoid code copy-pasting.
>>
>>> +        lua_pushstring(L, err_details);
>>> +        lua_concat(L, 3);
>>> +      }
>>>         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) {
>>> +        lua_pushstring(L, ": ");
>>> +        lua_pushstring(L, err_details);
>>> +        lua_concat(L, 3);
>>> +      }
>>>         lua_pushinteger(L, EINVAL);
>>>         return 3;
>>>       case PROFILE_ERRIO:
>>> @@ -291,15 +318,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")
>>> +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
>>> +
>> These changes should be under the LJ_HASSYSPROF ifdef.
>> Also, I suggest the following naming:
>> | SYSPROF_BADMODE
>> | SYSPROF_BADINTERVAL
>> With this it will be obviour that they are sysprof-specific and not too
>> long.
>
> Imagine you then introduce a table and interval number for memprof.
>
> Will you rename constants back? I dislike your suggestion.
>
>>>   #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..7622323a 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(33)
>>>   
>>>   jit.off()
>>>   -- XXX: Run JIT tuning functions in a safe frame to avoid errors
>>> @@ -65,10 +65,25 @@ end
>>>   
>>>   -- Wrong profiling mode.
>>>   local res, err, errno = misc.sysprof.start{ mode = "A" }
>>> -test:ok(res == nil anderr:match("profiler misuse"),
>>> -        "result status with wrong profiling 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")
>> I would rather still check matching with `profiler misuse:` error (as a
>> separate testcase). Here and below. But it's up to you. Feel free to
>> ignore.
> I think that checking specific part of error message is enough.
>>>   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.
>> This should be changed to the pcall() after the changes I suggested.
>>
>>> +res, err, errno = misc.sysprof.start("NOT A TABLE")
>>> +test:ok(res == nil, "res with not a table")
>>> +test:ok(err:match("profiler expects a table with parameters"),
>>> +        "error with not a table")
>>> +test:ok(type(errno) == "number", "errno with not a table")
>> Please also check the case with multiple arguments (2 cases: all
>> invalid, and only first valid).
>
> Added:
>
> 254: ok - res with all invalid parameters
> 254: ok - error with all invalid parameters
> 254: ok - errno with all invalid parameters
> 254: ok - res with all invalid parameters except the first one
> 254: ok - error with all invalid parameters except the first one
> 254: ok - errno with all invalid parameters except the first one
>
>
>>> +
>>>   -- Already running.
>>>   res, err = misc.sysprof.start{ mode = "D" }
>>>   assert(res, err)
>>> @@ -92,11 +107,30 @@test:ok(res == nil anderr:match("No such file or directory"),
>>>           "result status and error with bad path")
>>>   test:ok(type(errno) == "number", "errno with bad path")
>>>   
>>> --- Bad interval.
>>> +-- Bad interval (-1).
>>>   res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
>>> -test:ok(res == nil anderr:match("profiler misuse"),
>>> -        "result status and error with bad interval")
>>> -test:ok(type(errno) == "number", "errno with bad interval")
>>> +test:is(res, nil, "result status and error with bad interval -1")
>>> +test:ok(err:match("profiler interval must be greater than 1"),
>>> +        "error with bad interval -1")
>>> +test:ok(type(errno) == "number", "errno with bad interval -1")
>>> +
>>> +-- Bad interval (0).
>>> +res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 }
>>> +test:ok(res == nil, "res with bad interval 0")
>>> +test:ok(err:match("profiler interval must be greater than 1"),
>>> +        "error with bad interval 0")
>>> +test:ok(type(errno) == "number", "errno with bad interval 0")
>>> +
>>> +-- Good interval (1).
>>> +res, err, errno = misc.sysprof.start{
>> Minor: Please use brackets for functions call.
>
> Case without brackets is more popular:
>
>   local res, err = misc.sysprof.start(opts)
>   res,err = misc.sysprof.stop()
> local res, err, errno = misc.sysprof.start{ mode = "A" }
> res, err, errno = misc.sysprof.start{}
> assert(misc.sysprof.stop())
> res, err, errno = misc.sysprof.start("NOT A TABLE")
> res, err = misc.sysprof.start{ mode = "D" }
> res, err, errno = misc.sysprof.start{ mode = "D" }
> res, err = misc.sysprof.stop()
> res, err, errno = misc.sysprof.stop()
> res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
> res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
> res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 }
> res, err, errno = misc.sysprof.start{
> misc.sysprof.stop()
> local report = misc.sysprof.report()
> report = misc.sysprof.report()
>
> proposed change will make coding style inconsistent in the file
>
>>> +    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
>>>   
>>> -- 
>>> 2.34.1
>>>

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

[Tarantool-patches] [PATCH luajit 5/7] ci: add workflow with disabled profilers

[Sergey Bronnikov via Tarantool-patches]

Needed for the following commit.
---
 .github/workflows/exotic-builds-testing.yml       |  6 +++++-
 test/tarantool-tests/CMakeLists.txt               |  4 ++++
 .../gh-5994-memprof-human-readable.test.lua       |  1 +
 .../profilers/misclib-memprof-lapi.test.lua       | 15 ++++++++-------
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
index 374c879b..70b55423 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, nounwind, tablebump, noprof]
         include:
           - BUILDTYPE: Debug
             CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
@@ -54,6 +54,8 @@ jobs:
             FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
           - FLAVOR: tablebump
             FLAVORFLAGS: -DLUAJIT_ENABLE_TABLE_BUMP=ON
+          - FLAVOR: noprof
+            FLAVORFLAGS: -DLUAJIT_DISABLE_MEMPROF=ON -DLUAJIT_DISABLE_SYSPROF=ON
         exclude:
           - ARCH: ARM64
             GC64: OFF
@@ -72,6 +74,8 @@ jobs:
           # (`DASM_S_RANGE_I`).
           - FLAVOR: tablebump
             ARCH: ARM64
+          - FLAVOR: noprof
+            ARCH: ARM64
     runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
     name: >
       LuaJIT ${{ matrix.FLAVOR }}
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..0ab9c57c 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 ce41e4d5..33925c7f 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("misc-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.34.1

Re: [Tarantool-patches] [PATCH luajit 5/7] ci: add workflow with disabled profilers

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, with a minor nits below.

On 13.02.25, Sergey Bronnikov wrote:
> Needed for the following commit.
> ---
>  .github/workflows/exotic-builds-testing.yml       |  6 +++++-
>  test/tarantool-tests/CMakeLists.txt               |  4 ++++
>  .../gh-5994-memprof-human-readable.test.lua       |  1 +
>  .../profilers/misclib-memprof-lapi.test.lua       | 15 ++++++++-------

I suggest to split this commit into 2 separate (both of them are LGTM
though):
1) Introduce `LUAJIT_DISABLE_MEMPROF` flag in cmake and tests.
2) Add the corresponding exotic workflow.

>  4 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> index 374c879b..70b55423 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, nounwind, tablebump, noprof]

Please sort entries alphabetically.
Here and below.

>          include:
>            - BUILDTYPE: Debug
>              CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> @@ -54,6 +54,8 @@ jobs:
>              FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
>            - FLAVOR: tablebump
>              FLAVORFLAGS: -DLUAJIT_ENABLE_TABLE_BUMP=ON
> +          - FLAVOR: noprof
> +            FLAVORFLAGS: -DLUAJIT_DISABLE_MEMPROF=ON -DLUAJIT_DISABLE_SYSPROF=ON
>          exclude:
>            - ARCH: ARM64
>              GC64: OFF
> @@ -72,6 +74,8 @@ jobs:
>            # (`DASM_S_RANGE_I`).
>            - FLAVOR: tablebump
>              ARCH: ARM64
> +          - FLAVOR: noprof
> +            ARCH: ARM64
>      runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
>      name: >
>        LuaJIT ${{ matrix.FLAVOR }}
> 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

<snipped>

> index f3041779..0ab9c57c 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'),

Please use single quotes for this file (for the consistency).

>  })
>  
>  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 ce41e4d5..33925c7f 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("misc-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'),

Please use dougle quotes for this file (for the consistency).

>  })
>  

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 5/7] ci: add workflow with disabled profilers

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review!

On 18.02.2025 15:10, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, with a minor nits below.
>
> On 13.02.25, Sergey Bronnikov wrote:
>> Needed for the following commit.
>> ---
>>   .github/workflows/exotic-builds-testing.yml       |  6 +++++-
>>   test/tarantool-tests/CMakeLists.txt               |  4 ++++
>>   .../gh-5994-memprof-human-readable.test.lua       |  1 +
>>   .../profilers/misclib-memprof-lapi.test.lua       | 15 ++++++++-------
> I suggest to split this commit into 2 separate (both of them are LGTM
> though):
> 1) Introduce `LUAJIT_DISABLE_MEMPROF` flag in cmake and tests.
> 2) Add the corresponding exotic workflow.

Separated to commits:

  -   ci: add workflow with disabled profilers
  -   test: introduce flag LUAJIT_DISABLE_MEMPROF

>
>>   4 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
>> index 374c879b..70b55423 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, nounwind, tablebump, noprof]
> Please sort entries alphabetically.
> Here and below.
Fixed.
>
>>           include:
>>             - BUILDTYPE: Debug
>>               CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
>> @@ -54,6 +54,8 @@ jobs:
>>               FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
>>             - FLAVOR: tablebump
>>               FLAVORFLAGS: -DLUAJIT_ENABLE_TABLE_BUMP=ON
>> +          - FLAVOR: noprof
>> +            FLAVORFLAGS: -DLUAJIT_DISABLE_MEMPROF=ON -DLUAJIT_DISABLE_SYSPROF=ON
>>           exclude:
>>             - ARCH: ARM64
>>               GC64: OFF
>> @@ -72,6 +74,8 @@ jobs:
>>             # (`DASM_S_RANGE_I`).
>>             - FLAVOR: tablebump
>>               ARCH: ARM64
>> +          - FLAVOR: noprof
>> +            ARCH: ARM64
>>       runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
>>       name: >
>>         LuaJIT ${{ matrix.FLAVOR }}
>> 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
> <snipped>
>
>> index f3041779..0ab9c57c 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 alsohttps://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'),
> Please use single quotes for this file (for the consistency).

Fixed, thanks.


>>   })
>>   
>>   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 ce41e4d5..33925c7f 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("misc-memprof-lapi"):skipcond({
>>                                                  jit.arch ~= "x64",
>>     -- See alsohttps://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'),
> Please use dougle quotes for this file (for the consistency).
Fixed.
>
>>   })
>>   
> <snipped>
>
>> -- 
>> 2.34.1
>>

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

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

[Sergey Bronnikov via Tarantool-patches]

sysprof and memprof Lua API functions returns an error message
"profiler misuse", when appropriate profiler is disabled in build.
It is not possible to easily distinquish whether it is really
misuse or profiler was not enabled in build. The patch changes
errors messages, so when profiler was not enabled in build message
is the following: "profiler misuse: profiler is disabled".
---
 src/lib_misc.c                                | 27 ++++++++++++++++-
 src/lj_errmsg.h                               |  1 +
 .../misclib-memprof-lapi-disabled.test.lua    | 22 ++++++++++++++
 .../misclib-sysprof-lapi-disabled.test.lua    | 29 +++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)
 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 d71904e4..7666d85f 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -315,10 +315,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;
+#if !LJ_HASSYSPROF
+  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#endif /* !LJ_HASSYSPROF */
+
   int status = PROFILE_SUCCESS;
 
   struct luam_Sysprof_Options opt = {};
-  const char *err_details = NULL;
 
   status = parse_sysprof_opts(L, &opt, &err_details);
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
@@ -336,6 +341,11 @@ LJLIB_CF(misc_sysprof_start)
 /* local res, err, errno = profile.sysprof_stop() */
 LJLIB_CF(misc_sysprof_stop)
 {
+#if !LJ_HASSYSPROF
+  const char *err_details = NULL;
+  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#endif /* !LJ_HASSYSPROF */
   int status = luaM_sysprof_stop(L);
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
     return sysprof_error(L, status, NULL);
@@ -347,6 +357,11 @@ LJLIB_CF(misc_sysprof_stop)
 /* local counters, err, errno = sysprof.report() */
 LJLIB_CF(misc_sysprof_report)
 {
+#if !LJ_HASSYSPROF
+  const char *err_details = NULL;
+  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#endif /* !LJ_HASSYSPROF */
   struct luam_Sysprof_Counters counters = {};
   GCtab *data_tab = NULL;
   GCtab *count_tab = NULL;
@@ -386,6 +401,11 @@ 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 = NULL;
+  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#endif /* !LJ_HASMEMPROF */
   struct lj_memprof_options opt = {0};
   const char *fname = strdata(lj_lib_checkstr(L, 1));
   struct profile_ctx *ctx;
@@ -440,6 +460,11 @@ LJLIB_CF(misc_memprof_start)
 /* local stopped, err, errno = misc.memprof.stop() */
 LJLIB_CF(misc_memprof_stop)
 {
+#if !LJ_HASMEMPROF
+  const char *err_details = NULL;
+  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#endif /* !LJ_HASMEMPROF */
   int status = lj_memprof_stop(L);
   if (status != PROFILE_SUCCESS) {
     switch (status) {
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..a0669cc6
--- /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 memprof 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 memprof 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..79707434
--- /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.34.1

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

[Sergey Kaplun via Tarantool-patches]

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

On 13.02.25, Sergey Bronnikov wrote:
> sysprof and memprof Lua API functions returns an error message

Typo: s/returns/return/

> "profiler misuse", when appropriate profiler is disabled in build.

Typo: s/appropriate/the corresponding/
Typo: s/build/the build/

> It is not possible to easily distinquish whether it is really

Typo: s/distinquish/distinguish/

> misuse or profiler was not enabled in build. The patch changes

Typo: s/profiler/or if the profiler/
Typo: s/build/the build/

> errors messages, so when profiler was not enabled in build message

Typo: s/errors/error/
Typo: s/profiler/a profiler/
Typo: s/was/is/
Typo: s/in build message/in the build, the message/

> is the following: "profiler misuse: profiler is disabled".
> ---
>  src/lib_misc.c                                | 27 ++++++++++++++++-
>  src/lj_errmsg.h                               |  1 +
>  .../misclib-memprof-lapi-disabled.test.lua    | 22 ++++++++++++++
>  .../misclib-sysprof-lapi-disabled.test.lua    | 29 +++++++++++++++++++
>  4 files changed, 78 insertions(+), 1 deletion(-)
>  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 d71904e4..7666d85f 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -315,10 +315,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;
> +#if !LJ_HASSYSPROF

It's more LuaJIT-way to use something like the following:
| if (!LJ_HASSYSPROF) {
|   /* ... */
| }

This helps to avoid strange early return.
Also, please be avaired to declare all variables (stataus, opt,
err_details) in the beginning of the block.

Here and below.

> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASSYSPROF */
> +
>    int status = PROFILE_SUCCESS;
>  
>    struct luam_Sysprof_Options opt = {};
> -  const char *err_details = NULL;
>  
>    status = parse_sysprof_opts(L, &opt, &err_details);
>    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
> @@ -336,6 +341,11 @@ LJLIB_CF(misc_sysprof_start)
>  /* local res, err, errno = profile.sysprof_stop() */
>  LJLIB_CF(misc_sysprof_stop)
>  {
> +#if !LJ_HASSYSPROF
> +  const char *err_details = NULL;
> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);

Looks like we just can do the following for this block:

| const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);

Here and below.

> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASSYSPROF */
>    int status = luaM_sysprof_stop(L);
>    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>      return sysprof_error(L, status, NULL);
> @@ -347,6 +357,11 @@ LJLIB_CF(misc_sysprof_stop)
>  /* local counters, err, errno = sysprof.report() */
>  LJLIB_CF(misc_sysprof_report)
>  {
> +#if !LJ_HASSYSPROF
> +  const char *err_details = NULL;
> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASSYSPROF */
>    struct luam_Sysprof_Counters counters = {};
>    GCtab *data_tab = NULL;
>    GCtab *count_tab = NULL;
> @@ -386,6 +401,11 @@ 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 = NULL;
> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASMEMPROF */
>    struct lj_memprof_options opt = {0};
>    const char *fname = strdata(lj_lib_checkstr(L, 1));
>    struct profile_ctx *ctx;
> @@ -440,6 +460,11 @@ LJLIB_CF(misc_memprof_start)
>  /* local stopped, err, errno = misc.memprof.stop() */
>  LJLIB_CF(misc_memprof_stop)
>  {
> +#if !LJ_HASMEMPROF
> +  const char *err_details = NULL;
> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASMEMPROF */
>    int status = lj_memprof_stop(L);
>    if (status != PROFILE_SUCCESS) {
>      switch (status) {
> 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")

I suppose this should be declared under `#else` directive (i.e. if we
have at least one profiler 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..a0669cc6
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
> @@ -0,0 +1,22 @@
> +local tap = require("tap")

Please use single quotes everywhere across this file for the
consistency.

> +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 memprof is disabled.

Minor: s/start memprof when memprof/start memprof when it/

> +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 memprof is disabled.

Minor: s/stop memprof when memprof/stop memprof when it/

> +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..79707434
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
> @@ -0,0 +1,29 @@
> +local tap = require("tap")

Please use single quotes everywhere across this file for the
consistency.

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

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review!

On 19.02.2025 11:06, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, after fixing minor nits below.
>
> On 13.02.25, Sergey Bronnikov wrote:
>> sysprof and memprof Lua API functions returns an error message
> Typo: s/returns/return/
Fixed.
>
>> "profiler misuse", when appropriate profiler is disabled in build.
> Typo: s/appropriate/the corresponding/
> Typo: s/build/the build/
Fixed.
>
>> It is not possible to easily distinquish whether it is really
> Typo: s/distinquish/distinguish/
Fixed.
>
>> misuse or profiler was not enabled in build. The patch changes
> Typo: s/profiler/or if the profiler/
> Typo: s/build/the build/
Fixed.
>
>> errors messages, so when profiler was not enabled in build message
> Typo: s/errors/error/
> Typo: s/profiler/a profiler/
> Typo: s/was/is/
> Typo: s/in build message/in the build, the message/
Fixed.
>
>> is the following: "profiler misuse: profiler is disabled".
>> ---
>>   src/lib_misc.c                                | 27 ++++++++++++++++-
>>   src/lj_errmsg.h                               |  1 +
>>   .../misclib-memprof-lapi-disabled.test.lua    | 22 ++++++++++++++
>>   .../misclib-sysprof-lapi-disabled.test.lua    | 29 +++++++++++++++++++
>>   4 files changed, 78 insertions(+), 1 deletion(-)
>>   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 d71904e4..7666d85f 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -315,10 +315,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;
>> +#if !LJ_HASSYSPROF
> It's more LuaJIT-way to use something like the following:
> | if (!LJ_HASSYSPROF) {
> |   /* ... */
> | }
>
> This helps to avoid strange early return.
Fixed.
> Also, please be avaired to declare all variables (stataus, opt,
> err_details) in the beginning of the block.

  It is a requirement for the code that strictly follows c89, when you 
must declare all of your variables at the beginning of a scope block.

AFAIK, we have no such requirement, and also there is no option 
"-std=c89" in CI and no any mentions in the contribution guide.

>
> Here and below.

>
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASSYSPROF */
>> +
>>     int status = PROFILE_SUCCESS;
>>   
>>     struct luam_Sysprof_Options opt = {};
>> -  const char *err_details = NULL;
>>   
>>     status = parse_sysprof_opts(L, &opt, &err_details);
>>     if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>> @@ -336,6 +341,11 @@ LJLIB_CF(misc_sysprof_start)
>>   /* local res, err, errno = profile.sysprof_stop() */
>>   LJLIB_CF(misc_sysprof_stop)
>>   {
>> +#if !LJ_HASSYSPROF
>> +  const char *err_details = NULL;
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> Looks like we just can do the following for this block:
>
> | const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>
> Here and below.
Fixed.
>
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASSYSPROF */
>>     int status = luaM_sysprof_stop(L);
>>     if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>>       return sysprof_error(L, status, NULL);
>> @@ -347,6 +357,11 @@ LJLIB_CF(misc_sysprof_stop)
>>   /* local counters, err, errno = sysprof.report() */
>>   LJLIB_CF(misc_sysprof_report)
>>   {
>> +#if !LJ_HASSYSPROF
>> +  const char *err_details = NULL;
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASSYSPROF */
>>     struct luam_Sysprof_Counters counters = {};
>>     GCtab *data_tab = NULL;
>>     GCtab *count_tab = NULL;
>> @@ -386,6 +401,11 @@ 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 = NULL;
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASMEMPROF */
>>     struct lj_memprof_options opt = {0};
>>     const char *fname = strdata(lj_lib_checkstr(L, 1));
>>     struct profile_ctx *ctx;
>> @@ -440,6 +460,11 @@ LJLIB_CF(misc_memprof_start)
>>   /* local stopped, err, errno = misc.memprof.stop() */
>>   LJLIB_CF(misc_memprof_stop)
>>   {
>> +#if !LJ_HASMEMPROF
>> +  const char *err_details = NULL;
>> +  err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> +  return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> +#endif /* !LJ_HASMEMPROF */
>>     int status = lj_memprof_stop(L);
>>     if (status != PROFILE_SUCCESS) {
>>       switch (status) {
>> 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")
> I suppose this should be declared under `#else` directive (i.e. if we
> have at least one profiler disabled).

err message is required by code for checking disabled profilers (see above)

>
>>   #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..a0669cc6
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>> @@ -0,0 +1,22 @@
>> +local tap = require("tap")
> Please use single quotes everywhere across this file for the
> consistency.

Fixed.


>> +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 memprof is disabled.
> Minor: s/start memprof when memprof/start memprof when it/
>
Fixed.
>> +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 memprof is disabled.
> Minor: s/stop memprof when memprof/stop memprof when it/
Fixed.
>
>> +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..79707434
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
>> @@ -0,0 +1,29 @@
>> +local tap = require("tap")
> Please use single quotes everywhere across this file for the
> consistency.
Fixed.
>> +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.34.1
>>

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

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!

On 19.02.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review!
> 
> On 19.02.2025 11:06, Sergey Kaplun via Tarantool-patches wrote:

<snipped>

> > It's more LuaJIT-way to use something like the following:
> > | if (!LJ_HASSYSPROF) {
> > |   /* ... */
> > | }
> >
> > This helps to avoid strange early return.
> Fixed.
> > Also, please be avaired to declare all variables (stataus, opt,
> > err_details) in the beginning of the block.
> 
>   It is a requirement for the code that strictly follows c89, when you 
> must declare all of your variables at the beginning of a scope block.
> 
> AFAIK, we have no such requirement, and also there is no option 
> "-std=c89" in CI and no any mentions in the contribution guide.

This is not mentioned in Tarantool's contribution guide, since it is
LuaJIT-specific. In the perfect world, it should be checked by the flags
mentioned in <src/Makefile.original> (see the second CWARN).
Unfortunately, there are some bugs that should be fixed first.

<snipped>

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi,

On 19.02.2025 18:41, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
>
> On 19.02.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for review!
>>
>> On 19.02.2025 11:06, Sergey Kaplun via Tarantool-patches wrote:
> <snipped>
>
>>> It's more LuaJIT-way to use something like the following:
>>> | if (!LJ_HASSYSPROF) {
>>> |   /* ... */
>>> | }
>>>
>>> This helps to avoid strange early return.
>> Fixed.
>>> Also, please be avaired to declare all variables (stataus, opt,
>>> err_details) in the beginning of the block.
>>    It is a requirement for the code that strictly follows c89, when you
>> must declare all of your variables at the beginning of a scope block.
>>
>> AFAIK, we have no such requirement, and also there is no option
>> "-std=c89" in CI and no any mentions in the contribution guide.
> This is not mentioned in Tarantool's contribution guide, since it is
> LuaJIT-specific. In the perfect world, it should be checked by the flags
> mentioned in <src/Makefile.original> (see the second CWARN).
> Unfortunately, there are some bugs that should be fixed first.
>
> <snipped>

Fixed:


diff --git a/src/lib_misc.c b/src/lib_misc.c
index aaf08718..7b934e52 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -312,15 +312,14 @@ static int sysprof_error(lua_State *L, int status, 
const char *err_details)
  LJLIB_CF(misc_sysprof_start)
  {
    const char *err_details = NULL;
+  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);
    }

-  int status = PROFILE_SUCCESS;
-
-  struct luam_Sysprof_Options opt = {};
-
    status = parse_sysprof_opts(L, &opt, &err_details);
    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
      return sysprof_error(L, status, err_details);
@@ -337,11 +336,11 @@ LJLIB_CF(misc_sysprof_start)
  /* local res, err, errno = profile.sysprof_stop() */
  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);
    }
-  int status = luaM_sysprof_stop(L);
    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
      return sysprof_error(L, status, NULL);

@@ -352,15 +351,15 @@ LJLIB_CF(misc_sysprof_stop)
  /* local counters, err, errno = sysprof.report() */
  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);
    }
-  struct luam_Sysprof_Counters counters = {};
-  GCtab *data_tab = NULL;
-  GCtab *count_tab = NULL;

-  int status = luaM_sysprof_report(&counters);
    if (status != PROFILE_SUCCESS)
      return sysprof_error(L, status, NULL);

@@ -395,14 +394,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);
-  }
    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.
@@ -453,11 +452,11 @@ LJLIB_CF(misc_memprof_start)
  /* local stopped, err, errno = misc.memprof.stop() */
  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);
    }
-  int status = lj_memprof_stop(L);
    if (status != PROFILE_SUCCESS) {
      switch (status) {
      case PROFILE_ERRUSE:

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

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

[Sergey Bronnikov via Tarantool-patches]

sysprof has an optional parameter `path`, that set a path to
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 memprof profiling output file - `memprof.bin`.
---
 src/lib_misc.c                                |  5 ++-
 ...misclib-memprof-lapi-default-file.test.lua | 41 +++++++++++++++++++
 2 files changed, 45 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 7666d85f..4f5d72fc 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -398,6 +398,8 @@ 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)
 {
@@ -407,7 +409,8 @@ LJLIB_CF(misc_memprof_start)
   return sysprof_error(L, PROFILE_ERRUSE, err_details);
 #endif /* !LJ_HASMEMPROF */
   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;
 
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..b6233d4a
--- /dev/null
+++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
@@ -0,0 +1,41 @@
+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 def_output_file = 'memprof.bin'
+  os.remove(def_output_file)
+
+  local res, err = misc.memprof.start()
+  -- Should start successfully.
+  assert(res, err)
+
+  res, err = misc.memprof.stop()
+  -- Should stop successfully.
+  assert(res, err)
+
+  -- Want to cleanup carefully if something went wrong.
+  if not res then
+    os.remove(def_output_file)
+    error(err)
+  end
+
+  local profile_buf = tools.read_file(def_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(def_output_file)
+end)
+
+test:done(true)
-- 
2.34.1

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, with a minor nits below.

On 13.02.25, Sergey Bronnikov wrote:
> sysprof has an optional parameter `path`, that set a path to

Typo: s/set/sets/

> profiling output file, by default the path is `sysprof.bin`.

Typo: s/profiling/the profiling/
Typo: s/file, by default/file. By 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 memprof profiling output file - `memprof.bin`.

Typo: s/memprof/the memprof/

> ---
>  src/lib_misc.c                                |  5 ++-
>  ...misclib-memprof-lapi-default-file.test.lua | 41 +++++++++++++++++++
>  2 files changed, 45 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 7666d85f..4f5d72fc 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..b6233d4a
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
> @@ -0,0 +1,41 @@
> +local tap = require("tap")

Minor: Let's use single quotes here and below.
Side note: This code style part wasn't fixed when the first tests for
memprof was written.

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

Minor: Please use brackets for function calls.

> +
> +test:test("default-output-file", function(subtest)
> +
> +  subtest:plan(1)
> +
> +  local def_output_file = 'memprof.bin'

Minor: s/def/default/

> +  os.remove(def_output_file)
> +
> +  local res, err = misc.memprof.start()
> +  -- Should start successfully.
> +  assert(res, err)

This assert should be removed since we want to remove the file in case
of an error (for example, we can't write the memprof prologue).

> +
> +  res, err = misc.memprof.stop()
> +  -- Should stop successfully.
> +  assert(res, err)

This assert should be removed since we want to remove the file in case
of an error.

> +
> +  -- Want to cleanup carefully if something went wrong.
> +  if not res then
> +    os.remove(def_output_file)
> +    error(err)
> +  end
> +
> +  local profile_buf = tools.read_file(def_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(def_output_file)
> +end)
> +
> +test:done(true)
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review!

On 18.02.2025 14:55, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, with a minor nits below.
>
> On 13.02.25, Sergey Bronnikov wrote:
>> sysprof has an optional parameter `path`, that set a path to
> Typo: s/set/sets/
Fixed.
>
>> profiling output file, by default the path is `sysprof.bin`.
> Typo: s/profiling/the profiling/
> Typo: s/file, by default/file. By 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 memprof profiling output file - `memprof.bin`.
> Typo: s/memprof/the memprof/
Fixed.
>
>> ---
>>   src/lib_misc.c                                |  5 ++-
>>   ...misclib-memprof-lapi-default-file.test.lua | 41 +++++++++++++++++++
>>   2 files changed, 45 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 7666d85f..4f5d72fc 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..b6233d4a
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
>> @@ -0,0 +1,41 @@
>> +local tap = require("tap")
> Minor: Let's use single quotes here and below.
> Side note: This code style part wasn't fixed when the first tests for
> memprof was written.
Fixed.
>> +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"
> Minor: Please use brackets for function calls.
Fixed.
>
>> +
>> +test:test("default-output-file", function(subtest)
>> +
>> +subtest:plan(1)
>> +
>> +  local def_output_file = 'memprof.bin'
> Minor: s/def/default/
Fixed.
>
>> +  os.remove(def_output_file)
>> +
>> +  local res, err = misc.memprof.start()
>> +  -- Should start successfully.
>> +  assert(res, err)
> This assert should be removed since we want to remove the file in case
> of an error (for example, we can't write the memprof prologue).
Fixed.
>
>> +
>> +  res, err = misc.memprof.stop()
>> +  -- Should stop successfully.
>> +  assert(res, err)
> This assert should be removed since we want to remove the file in case
> of an error.


Fixed.

>
>> +
>> +  -- Want to cleanup carefully if something went wrong.
>> +  if not res then
>> +    os.remove(def_output_file)
>> +    error(err)
>> +  end
>> +
>> +  local profile_buf = tools.read_file(def_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(def_output_file)
>> +end)
>> +
>> +test:done(true)
>> -- 
>> 2.34.1
>>

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