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

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

[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

[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 | 49 +++++++++----------------------------------------
 1 file changed, 9 insertions(+), 40 deletions(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index b4b58509..c4b40996 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -286,14 +286,14 @@ 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));
       lua_pushinteger(L, EINVAL);
       return 3;
     case PROFILE_ERRIO:
-      return luaL_fileresult(L, 0, NULL);
+      return luaL_fileresult(L, 0, err_details);
 #endif
     default:
       lj_assertL(0, "bad sysprof error %d", status);
@@ -417,32 +417,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 */
@@ -456,27 +437,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:
+  if (LJ_UNLIKELY(status == 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_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]

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

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

both in `sysprof_error()` and fixes aforementioned problem.

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 c4b40996..74888d20 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -335,6 +335,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 ebd80cf6..770b5736 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
@@ -124,7 +124,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 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

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except 1 minor comment and 1 reminder below.

On 25.02.25, Sergey Bronnikov wrote:
> 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 | 49 +++++++++----------------------------------------
>  1 file changed, 9 insertions(+), 40 deletions(-)
> 
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index b4b58509..c4b40996 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -286,14 +286,14 @@ 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));
>        lua_pushinteger(L, EINVAL);
>        return 3;
>      case PROFILE_ERRIO:
> -      return luaL_fileresult(L, 0, NULL);
> +      return luaL_fileresult(L, 0, err_details);

Discussed offline that this should be done in the previous patch-set.

>  #endif

<snipped>

> +  if (LJ_UNLIKELY(status == 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_SUCCESS))

Minor: Looks like more natural to use `else if` here?
Feel free to ignore.

> +    return prof_error(L, status, NULL);
> +
>    lua_pushboolean(L, 1);
>    return 1;
>  #endif /* !LJ_HASMEMPROF */
> -- 
> 2.43.0
> 

-- 
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 patch!
LGTM, with a few nits regarding the commit message and 1 ignorable
comment to the code style.

On 25.02.25, Sergey Bronnikov wrote:
> The function `misc.sysprof.stop()` reports that profiler is

Typo: s/profiler/the profiler/

> already running:
> 
> | $ ./src/luajit -e 'print(misc.sysprof.stop())'
> | nil     profiler is running already     22
> 
> both in `sysprof_error()` and fixes aforementioned problem.

Don't get this sentence.

Typo: s/aforementioned/the aforementioned/

> 
> 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 c4b40996..74888d20 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -335,6 +335,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))

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 ebd80cf6..770b5736 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: 2637 bytes --]

Hi, Sergey,

the patch series was rebased to the master branch (tarantool/master)

without conflicts.

Sergey

On 05.03.2025 17:52, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, with a few nits regarding the commit message and 1 ignorable
> comment to the code style.
>
> On 25.02.25, Sergey Bronnikov wrote:
>> The function `misc.sysprof.stop()` reports that profiler is
> Typo: s/profiler/the profiler/
>
>> already running:
>>
>> | $ ./src/luajit -e 'print(misc.sysprof.stop())'
>> | nil     profiler is running already     22
>>
>> both in `sysprof_error()` and fixes aforementioned problem.
> Don't get this sentence.

Missed this comment.

Now commit message is updated, new version is below:

Author: Sergey Bronnikov <sergeyb@tarantool.org>
Date:   Tue Feb 4 11:48:05 2025 +0300

     sysprof: fix a message with stop without run

     When sysprof is not started the function `misc.sysprof.stop()`
     reports that 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

>
> Typo: s/aforementioned/the aforementioned/
>
>> 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 c4b40996..74888d20 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -335,6 +335,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))
> 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 ebd80cf6..770b5736 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: 4148 bytes --]

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

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

see my comments

On 05.03.2025 17:49, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except 1 minor comment and 1 reminder below.
>
> On 25.02.25, Sergey Bronnikov wrote:
>> 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 | 49 +++++++++----------------------------------------
>>   1 file changed, 9 insertions(+), 40 deletions(-)
>>
>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>> index b4b58509..c4b40996 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -286,14 +286,14 @@ 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));
>>         lua_pushinteger(L, EINVAL);
>>         return 3;
>>       case PROFILE_ERRIO:
>> -      return luaL_fileresult(L, 0, NULL);
>> +      return luaL_fileresult(L, 0, err_details);
> Discussed offline that this should be done in the previous patch-set.
The hunk is gone after rebase.
>
>>   #endif
> <snipped>
>
>> +  if (LJ_UNLIKELY(status == 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_SUCCESS))
> Minor: Looks like more natural to use `else if` here?
> Feel free to ignore.

Fixed:

--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -443,9 +443,9 @@ LJLIB_CF(misc_memprof_stop)
        lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING));
        lua_pushinteger(L, EINVAL);
        return 3;
-  }
-  if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
+  } 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);
>> +
>>     lua_pushboolean(L, 1);
>>     return 1;
>>   #endif /* !LJ_HASMEMPROF */
>> -- 
>> 2.43.0
>>

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

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

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

[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