[Tarantool-patches] [PATCH luajit 7/8][v3] misc: specific message for disabled profilers
Sergey Bronnikov
sergeyb at tarantool.org
Mon Feb 24 21:37:20 MSK 2025
Hi, Sergey!
thanks for review!
Updated and force-pushed.
Sergey
On 24.02.2025 14:28, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, after fixing my comments below.
>
> On 20.02.25, Sergey Bronnikov wrote:
>> sysprof and memprof Lua API functions return an error message
>> "profiler misuse", when the corresponding profiler is disabled in
>> the build. It is not possible to easily distinguish whether it is
>> really misuse or if the profiler was not enabled in the build. The
>> patch changes error messages, so when profiler is not enabled in
>> the build, the message is the following:"profiler misuse: profiler is disabled".
>> ---
>> src/lib_misc.c | 25 ++++++++++++++--
>> src/lj_errmsg.h | 1 +
>> .../misclib-memprof-lapi-disabled.test.lua | 22 ++++++++++++++
>> .../misclib-sysprof-lapi-disabled.test.lua | 29 +++++++++++++++++++
>> 4 files changed, 75 insertions(+), 2 deletions(-)
>> create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>> create mode 100644 test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
>>
>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>> index 1fd06dd1..d98cf3f0 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -306,10 +306,15 @@ static int sysprof_error(lua_State *L, int status, const char *err_details)
>> /* local res, err, errno = sysprof.start(options) */
>> LJLIB_CF(misc_sysprof_start)
>> {
> As we discussed ofline we may use 2 approaches here:
>
> 1) Use branching:
> a) Main branch first.
> | {
> | if (LJ_HASSYSPROF) {
> | /* ... */
> | } else {
> | const char *err_details = //
> | /* ... */
> | }
> | }
>
> b) Early return first
> | {
> | if (!LJ_HASSYSPROF) {
> | const char *err_details = //
> | /* ... */
> | } else {
> | /* ... */
> | }
> | }
>
> 2) Use macros directives instead:
> a) Main branch first.
> | {
> | #if LJ_HASSYSPROF
> | /* ... */
> | #else
> | /* ... */
> | }
> | #endif
>
> b) Early return first
> | {
> | #if !LJ_HASSYSPROF
> | /* ... */
> | #else
> | /* ... */
> | }
> | #endif
>
> We decided to use the second approach to avoid huge diff changes and
> make code more readable.
> It's up to you to use an approach a) or b).
> This helps us to avoid warnings with `-Wdeclaration-after-statement`
> enabled.
Updated (approach b)):
diff --git a/src/lib_misc.c b/src/lib_misc.c
index 62d0597e..d5122665 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -304,15 +304,14 @@ static int sysprof_error(lua_State *L, int status,
const char *err_details)
/* local res, err, errno = sysprof.start(options) */
LJLIB_CF(misc_sysprof_start)
{
- const char *err_details = NULL;
+#if !LJ_HASSYSPROF
+ const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
int status = PROFILE_SUCCESS;
struct luam_Sysprof_Options opt = {};
-
- if (!LJ_HASSYSPROF) {
- err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
- return sysprof_error(L, PROFILE_ERRUSE, err_details);
- }
+ const char *err_details = NULL;
status = parse_sysprof_opts(L, &opt, &err_details);
if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
@@ -325,35 +324,36 @@ LJLIB_CF(misc_sysprof_start)
lua_pushboolean(L, 1);
return 1;
+#endif /* !LJ_HASSYSPROF */
}
/* local res, err, errno = profile.sysprof_stop() */
LJLIB_CF(misc_sysprof_stop)
{
+#if !LJ_HASSYSPROF
+ const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
int status = luaM_sysprof_stop(L);
- if (!LJ_HASSYSPROF) {
- const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
- return sysprof_error(L, PROFILE_ERRUSE, err_details);
- }
if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
return sysprof_error(L, status, NULL);
lua_pushboolean(L, 1);
return 1;
+#endif /* !LJ_HASSYSPROF */
}
/* local counters, err, errno = sysprof.report() */
LJLIB_CF(misc_sysprof_report)
{
+#if !LJ_HASSYSPROF
+ const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
struct luam_Sysprof_Counters counters = {};
GCtab *data_tab = NULL;
GCtab *count_tab = NULL;
int status = luaM_sysprof_report(&counters);
- if (!LJ_HASSYSPROF) {
- const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
- return sysprof_error(L, PROFILE_ERRUSE, err_details);
- }
-
if (status != PROFILE_SUCCESS)
return sysprof_error(L, status, NULL);
@@ -379,6 +379,7 @@ LJLIB_CF(misc_sysprof_report)
lua_setfield(L, -2, "vmstate");
return 1;
+#endif /* !LJ_HASSYSPROF */
}
/* ----- misc.memprof module
---------------------------------------------- */
@@ -388,14 +389,14 @@ LJLIB_CF(misc_sysprof_report)
/* local started, err, errno = misc.memprof.start(fname) */
LJLIB_CF(misc_memprof_start)
{
+#if !LJ_HASMEMPROF
+ const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
struct lj_memprof_options opt = {0};
const char *fname = strdata(lj_lib_checkstr(L, 1));
struct profile_ctx *ctx;
int memprof_status;
- if (!LJ_HASMEMPROF) {
- const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
- return sysprof_error(L, PROFILE_ERRUSE, err_details);
- }
/*
** FIXME: more elegant solution with ctx.
@@ -441,16 +442,17 @@ LJLIB_CF(misc_memprof_start)
}
lua_pushboolean(L, 1);
return 1;
+#endif /* !LJ_HASMEMPROF */
}
/* local stopped, err, errno = misc.memprof.stop() */
LJLIB_CF(misc_memprof_stop)
{
+#if !LJ_HASMEMPROF
+ const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);
+#else
int status = lj_memprof_stop(L);
- if (!LJ_HASMEMPROF) {
- const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
- return sysprof_error(L, PROFILE_ERRUSE, err_details);
- }
if (status != PROFILE_SUCCESS) {
switch (status) {
case PROFILE_ERRUSE:
@@ -474,6 +476,7 @@ LJLIB_CF(misc_memprof_stop)
}
lua_pushboolean(L, 1);
return 1;
+#endif /* !LJ_HASMEMPROF */
}
#endif /* !LJ_TARGET_WINDOWS */
> Here and below.
>
> Also, it helps to avoid an excess call to `lj_{sysprof,memprof}_stop()`.
>
>> + const char *err_details = NULL;
>> int status = PROFILE_SUCCESS;
>>
>> struct luam_Sysprof_Options opt = {};
>> - const char *err_details = NULL;
>> +
>> + if (!LJ_HASSYSPROF) {
>> + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> + }
>>
>> status = parse_sysprof_opts(L, &opt, &err_details);
>> if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>> @@ -328,6 +333,10 @@ LJLIB_CF(misc_sysprof_start)
>> LJLIB_CF(misc_sysprof_stop)
>> {
>> int status = luaM_sysprof_stop(L);
>> + if (!LJ_HASSYSPROF) {
>> + const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> + }
>> if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
>> return sysprof_error(L, status, NULL);
>>
>> @@ -341,8 +350,12 @@ LJLIB_CF(misc_sysprof_report)
>> struct luam_Sysprof_Counters counters = {};
>> GCtab *data_tab = NULL;
>> GCtab *count_tab = NULL;
>> -
>> int status = luaM_sysprof_report(&counters);
>> + if (!LJ_HASSYSPROF) {
>> + const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> + }
>> +
>> if (status != PROFILE_SUCCESS)
>> return sysprof_error(L, status, NULL);
>>
>> @@ -381,6 +394,10 @@ LJLIB_CF(misc_memprof_start)
>> const char *fname = strdata(lj_lib_checkstr(L, 1));
>> struct profile_ctx *ctx;
>> int memprof_status;
>> + if (!LJ_HASMEMPROF) {
>> + const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> + }
>>
>> /*
>> ** FIXME: more elegant solution with ctx.
>> @@ -432,6 +449,10 @@ LJLIB_CF(misc_memprof_start)
>> LJLIB_CF(misc_memprof_stop)
>> {
>> int status = lj_memprof_stop(L);
>> + if (!LJ_HASMEMPROF) {
>> + const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
>> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
>> + }
>> if (status != PROFILE_SUCCESS) {
>> switch (status) {
>> case PROFILE_ERRUSE:
>> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h
>> index b5c3a275..e26f5e38 100644
>> --- a/src/lj_errmsg.h
>> +++ b/src/lj_errmsg.h
>> @@ -193,6 +193,7 @@ ERRDEF(PROF_DETAILS_BADINTERVAL, "profiler interval must be greater than 1")
>> ERRDEF(PROF_DETAILS_BADPATH, "profiler path does not exist")
>> ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
>>
>> +ERRDEF(PROF_DETAILS_DISABLED, "profiler is disabled")
>> #undef ERRDEF
>>
>> /* Detecting unused error messages:
>> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>> new file mode 100644
>> index 00000000..de0aa136
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
>> @@ -0,0 +1,22 @@
>> +local tap = require('tap')
>> +local test = tap.test('misclib-memprof-lapi-disabled'):skipcond({
>> + ['Memprof is enabled'] = not os.getenv('LUAJIT_DISABLE_MEMPROF'),
>> +})
>> +
>> +test:plan(6)
>> +
>> +-- Attempt to start memprof when it is disabled.
>> +local res, err, errno = misc.memprof.start()
>> +test:is(res, nil, 'result status on start when memprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> + 'error on start when memprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
>> +
>> +-- Attempt to stop memprof when it is disabled.
>> +res, err, errno = misc.memprof.stop()
>> +test:is(res, nil, 'result status on stop when memprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> + 'error on stop when memprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when memprof is disabled')
>> +
>> +test:done(true)
>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
>> new file mode 100644
>> index 00000000..2a9ce796
>> --- /dev/null
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
>> @@ -0,0 +1,29 @@
>> +local tap = require('tap')
>> +local test = tap.test('misclib-sysprof-lapi-disabled'):skipcond({
>> + ['Sysprof is enabled'] = not os.getenv('LUAJIT_DISABLE_SYSPROF'),
>> +})
>> +
>> +test:plan(9)
>> +
>> +-- Attempt to start sysprof when sysprof is disabled.
>> +local res, err, errno = misc.sysprof.start()
>> +test:is(res, nil, 'result status on start when sysprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> + 'error on start when sysprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
>> +
>> +-- Attempt to stop sysprof when sysprof is disabled.
>> +res, err, errno = misc.sysprof.stop()
>> +test:is(res, nil, 'result status on stop when sysprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> + 'error on stop when sysprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
>> +
>> +-- Attempt to report when sysprof is disabled.
>> +res, err, errno = misc.sysprof.report()
>> +test:is(res, nil, 'result status on report when sysprof is disabled')
>> +test:ok(err:match('profiler is disabled'),
>> + 'error on stop when sysprof is disabled')
>> +test:ok(type(errno) == 'number', 'errno on start when sysprof is disabled')
>> +
>> +test:done(true)
>> --
>> 2.43.0
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20250224/d0112090/attachment.htm>
More information about the Tarantool-patches
mailing list