[Tarantool-patches] [PATCH luajit 0/3] Fix sysprof error on stop not started sysprof

[Tarantool-patches] [PATCH luajit 0/3] Fix sysprof error on stop not started sysprof

[Sergey Bronnikov via Tarantool-patches]

The patch series fix an incorrect error on stop sysprof when
it is not started.

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

Changes v2:
- Fixes according to comments by Sergey Kaplun.
- Rebased to tarantool/master after merging patch series with sysprof fixes.

Sergey Bronnikov (3):
  sysprof: rename sysprof_error to prof_error
  misc: use prof_error for handling errors
  sysprof: fix a message with stop without run

 src/lib_misc.c                                | 81 +++++++------------
 .../profilers/misclib-sysprof-lapi.test.lua   |  5 +-
 2 files changed, 31 insertions(+), 55 deletions(-)

-- 
2.43.0

[Tarantool-patches] [PATCH luajit 1/3] sysprof: rename sysprof_error to prof_error

[Sergey Bronnikov via Tarantool-patches]

Rename a function `sysprof_error()` to `prof_error()` to make
it profiler-independent.

Needed for the following commit.
---
 src/lib_misc.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 83669268..9c827157 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -276,7 +276,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,
   return PROFILE_SUCCESS;
 }
 
-static int sysprof_error(lua_State *L, int status, const char *err_details)
+static int prof_error(lua_State *L, int status, const char *err_details)
 {
   switch (status) {
     case PROFILE_ERRUSE:
@@ -307,7 +307,7 @@ LJLIB_CF(misc_sysprof_start)
 {
 #if !LJ_HASSYSPROF
   const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+  return prof_error(L, PROFILE_ERRUSE, err_details);
 #else
   int status = PROFILE_SUCCESS;
 
@@ -316,12 +316,12 @@ LJLIB_CF(misc_sysprof_start)
 
   status = parse_sysprof_opts(L, &opt, &err_details);
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
-    return sysprof_error(L, status, err_details);
+    return prof_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, err_details);
+    return prof_error(L, status, err_details);
 
   lua_pushboolean(L, 1);
   return 1;
@@ -333,11 +333,11 @@ LJLIB_CF(misc_sysprof_stop)
 {
 #if !LJ_HASSYSPROF
   const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+  return prof_error(L, PROFILE_ERRUSE, err_details);
 #else
   int status = luaM_sysprof_stop(L);
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
-    return sysprof_error(L, status, NULL);
+    return prof_error(L, status, NULL);
 
   lua_pushboolean(L, 1);
   return 1;
@@ -349,14 +349,14 @@ LJLIB_CF(misc_sysprof_report)
 {
 #if !LJ_HASSYSPROF
   const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+  return prof_error(L, PROFILE_ERRUSE, err_details);
 #else
   struct luam_Sysprof_Counters counters = {};
   GCtab *data_tab = NULL;
   GCtab *count_tab = NULL;
   int status = luaM_sysprof_report(&counters);
   if (status != PROFILE_SUCCESS)
-    return sysprof_error(L, status, NULL);
+    return prof_error(L, status, NULL);
 
   lua_createtable(L, 0, 3);
   data_tab = tabV(L->top - 1);
@@ -394,7 +394,7 @@ 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);
+  return prof_error(L, PROFILE_ERRUSE, err_details);
 #else
   struct lj_memprof_options opt = {0};
   GCstr *s = lj_lib_optstr(L, 1);
@@ -454,7 +454,7 @@ LJLIB_CF(misc_memprof_stop)
 {
 #if !LJ_HASMEMPROF
   const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
-  return sysprof_error(L, PROFILE_ERRUSE, err_details);
+  return prof_error(L, PROFILE_ERRUSE, err_details);
 #else
   int status = lj_memprof_stop(L);
   if (status != PROFILE_SUCCESS) {
-- 
2.43.0

[Tarantool-patches] [PATCH luajit 2/3] misc: use prof_error for handling errors

[Sergey Bronnikov via Tarantool-patches]

The patch consolidates handling profilers errors into a single
place - in a function `prof_error()` and handles PROFILE_ERRIO,
generated in a function `misc_memprof_start`, in a `prof_error()`.
---
 src/lib_misc.c | 55 +++++++++++---------------------------------------
 1 file changed, 12 insertions(+), 43 deletions(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 9c827157..1c81fd80 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -287,7 +287,7 @@ static int prof_error(lua_State *L, int status, const char *err_details)
 	lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
       lua_pushinteger(L, EINVAL);
       return 3;
-#if LJ_HASSYSPROF
+#if LJ_HASSYSPROF || LJ_HASMEMPROF
     case PROFILE_ERRRUN:
       lua_pushnil(L);
       lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
@@ -418,32 +418,13 @@ LJLIB_CF(misc_memprof_start)
 
   if (ctx->fd == -1) {
     lj_mem_free(ctx->g, ctx, sizeof(*ctx));
-    return luaL_fileresult(L, 0, fname);
+    return prof_error(L, PROFILE_ERRIO, fname);
   }
 
   memprof_status = lj_memprof_start(L, &opt);
+  if (LJ_UNLIKELY(memprof_status != PROFILE_SUCCESS))
+    return prof_error(L, memprof_status, NULL);
 
-  if (LJ_UNLIKELY(memprof_status != PROFILE_SUCCESS)) {
-    switch (memprof_status) {
-    case PROFILE_ERRUSE:
-      lua_pushnil(L);
-      lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
-      lua_pushinteger(L, EINVAL);
-      return 3;
-#if LJ_HASMEMPROF
-    case PROFILE_ERRRUN:
-      lua_pushnil(L);
-      lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
-      lua_pushinteger(L, EINVAL);
-      return 3;
-    case PROFILE_ERRIO:
-      return luaL_fileresult(L, 0, fname);
-#endif
-    default:
-      lj_assertL(0, "bad memprof error %d", memprof_status);
-      return 0;
-    }
-  }
   lua_pushboolean(L, 1);
   return 1;
 #endif /* !LJ_HASMEMPROF */
@@ -457,27 +438,15 @@ LJLIB_CF(misc_memprof_stop)
   return prof_error(L, PROFILE_ERRUSE, err_details);
 #else
   int status = lj_memprof_stop(L);
-  if (status != PROFILE_SUCCESS) {
-    switch (status) {
-    case PROFILE_ERRUSE:
-      lua_pushnil(L);
-      lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
-      lua_pushinteger(L, EINVAL);
-      return 3;
-#if LJ_HASMEMPROF
-    case PROFILE_ERRRUN:
-      lua_pushnil(L);
-      lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING));
-      lua_pushinteger(L, EINVAL);
-      return 3;
-    case PROFILE_ERRIO:
-      return luaL_fileresult(L, 0, NULL);
-#endif
-    default:
-      lj_assertL(0, "bad memprof error %d", status);
-      return 0;
-    }
+  if (LJ_UNLIKELY(status == PROFILE_ERRRUN)) {
+    lua_pushnil(L);
+    lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING));
+    lua_pushinteger(L, EINVAL);
+    return 3;
+  } else if (LJ_UNLIKELY(status != PROFILE_SUCCESS)) {
+    return prof_error(L, status, NULL);
   }
+
   lua_pushboolean(L, 1);
   return 1;
 #endif /* !LJ_HASMEMPROF */
-- 
2.43.0

Re: [Tarantool-patches] [PATCH luajit 2/3] misc: use prof_error for handling errors

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

When sysprof is not started the function `misc.sysprof.stop()`
reports that the profiler is already running:

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

The patch fixes that:

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

Follows up tarantool/tarantool#781
---
 src/lib_misc.c                                              | 6 ++++++
 .../tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 +++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 1c81fd80..88ae9e10 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -336,6 +336,12 @@ LJLIB_CF(misc_sysprof_stop)
   return prof_error(L, PROFILE_ERRUSE, err_details);
 #else
   int status = luaM_sysprof_stop(L);
+  if (LJ_UNLIKELY(status == PROFILE_ERRRUN)) {
+      lua_pushnil(L);
+      lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING));
+      lua_pushinteger(L, EINVAL);
+      return 3;
+  }
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
     return prof_error(L, status, NULL);
 
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index 91ea461b..f316c390 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(43)
+test:plan(44)
 
 jit.off()
 -- XXX: Run JIT tuning functions in a safe frame to avoid errors
@@ -123,7 +123,8 @@ assert(res, err)
 
 -- Not running.
 res, err, errno = misc.sysprof.stop()
-test:ok(res == nil and err, "result status and error with not running")
+test:is(res, nil, "result status with not running")
+test:ok(err:match("profiler is not running"), "error with not running")
 test:ok(type(errno) == "number", "errno with not running")
 
 -- Bad path.
-- 
2.43.0

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM, with 2 minor comments below.

On 06.03.25, Sergey Bronnikov wrote:
> When sysprof is not started the function `misc.sysprof.stop()`

Typo: s/started/started,/

> reports that the profiler is already running:
> 
> | $ ./src/luajit -e 'print(misc.sysprof.stop())'
> | nil     profiler is running already     22
> 
> The patch fixes that:
> 
> | $ ./src/luajit -e 'print(misc.sysprof.stop())'
> | nil     profiler is not running         22
> 
> Follows up tarantool/tarantool#781
> ---

<snipped>

> +  if (LJ_UNLIKELY(status == PROFILE_ERRRUN)) {
> +      lua_pushnil(L);
> +      lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING));
> +      lua_pushinteger(L, EINVAL);
> +      return 3;
> +  }
>    if (LJ_UNLIKELY(status != PROFILE_SUCCESS))

It looks like more natural now to use `else if` here now.

>      return prof_error(L, status, NULL);
>  
> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> index 91ea461b..f316c390 100644
> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey!

both issues were fixed and force-pushed to the branch.

Sergey

On 07.03.2025 10:21, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, with 2 minor comments below.
>
> On 06.03.25, Sergey Bronnikov wrote:
>> When sysprof is not started the function `misc.sysprof.stop()`
> Typo: s/started/started,/
Fixed.
>
>> reports that the profiler is already running:
>>
>> | $ ./src/luajit -e 'print(misc.sysprof.stop())'
>> | nil     profiler is running already     22
>>
>> The patch fixes that:
>>
>> | $ ./src/luajit -e 'print(misc.sysprof.stop())'
>> | nil     profiler is not running         22
>>
>> Follows up tarantool/tarantool#781
>> ---
> <snipped>
>
>> +  if (LJ_UNLIKELY(status == PROFILE_ERRRUN)) {
>> +      lua_pushnil(L);
>> +      lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING));
>> +      lua_pushinteger(L, EINVAL);
>> +      return 3;
>> +  }
>>     if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
> It looks like more natural now to use `else if` here now.

Fixed, updated version:

--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -336,8 +336,14 @@ LJLIB_CF(misc_sysprof_stop)
    return prof_error(L, PROFILE_ERRUSE, err_details);
  #else
    int status = luaM_sysprof_stop(L);
-  if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
+  if (LJ_UNLIKELY(status == PROFILE_ERRRUN)) {
+    lua_pushnil(L);
+    lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING));
+    lua_pushinteger(L, EINVAL);
+    return 3;
+  } else if (LJ_UNLIKELY(status != PROFILE_SUCCESS)) {
      return prof_error(L, status, NULL);
+  }

    lua_pushboolean(L, 1);
    return 1;

>
>>       return prof_error(L, status, NULL);
>>   
>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> index 91ea461b..f316c390 100644
>> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
> <snipped>
>
>> -- 
>> 2.43.0
>>

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

[Tarantool-patches] [PATCH luajit 0/3] Fix sysprof error on stop not started sysprof

[Sergey Bronnikov via Tarantool-patches]

The patch series fix an incorrect error on stop sysprof when
it is not started.

Note, patch series depends on patch series with fixes error messages
and default values in profilers, see branch [1].

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

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

Sergey Bronnikov (3):
  sysprof: rename sysprof_error to prof_error
  misc: use prof_error for handling errors
  sysprof: fix a message with stop without run

 src/lib_misc.c                                | 75 +++++++------------
 .../profilers/misclib-sysprof-lapi.test.lua   |  5 +-
 2 files changed, 28 insertions(+), 52 deletions(-)

--
2.43.0

Re: [Tarantool-patches] [PATCH luajit 0/3] Fix sysprof error on stop not started sysprof

[Sergey Kaplun via Tarantool-patches]

Sergey,

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

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

-- 
Best regards,
Sergey Kaplun