[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

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

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

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM.

-- 
Best regards,
Sergey Kaplun

[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 94ec6564..b4b58509 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -275,7 +275,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:
@@ -306,7 +306,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;
 
@@ -315,12 +315,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;
@@ -332,11 +332,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;
@@ -348,14 +348,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);
@@ -393,7 +393,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);
@@ -453,7 +453,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