<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey!</p>
<p>thanks for review!</p>
<p>Updated and force-pushed.</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 24.02.2025 14:28, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z7xX-W8u39kfNI2G@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing my comments below.
On 20.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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: <a class="moz-txt-link-rfc2396E" href="profilermisuse:profilerisdisabled">"profiler misuse:
profiler is disabled"</a>.
---
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)
{
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.
</pre>
</blockquote>
<p>Updated (approach b)):</p>
<p>diff --git a/src/lib_misc.c b/src/lib_misc.c<br>
index 62d0597e..d5122665 100644<br>
--- a/src/lib_misc.c<br>
+++ b/src/lib_misc.c<br>
@@ -304,15 +304,14 @@ static int sysprof_error(lua_State *L, int
status, const char *err_details)<br>
/* local res, err, errno = sysprof.start(options) */<br>
LJLIB_CF(misc_sysprof_start)<br>
{<br>
- const char *err_details = NULL;<br>
+#if !LJ_HASSYSPROF<br>
+ const char *err_details =
err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
+#else<br>
int status = PROFILE_SUCCESS;<br>
<br>
struct luam_Sysprof_Options opt = {};<br>
-<br>
- if (!LJ_HASSYSPROF) {<br>
- err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
- return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
- }<br>
+ const char *err_details = NULL;<br>
<br>
status = parse_sysprof_opts(L, &opt, &err_details);<br>
if (LJ_UNLIKELY(status != PROFILE_SUCCESS))<br>
@@ -325,35 +324,36 @@ LJLIB_CF(misc_sysprof_start)<br>
<br>
lua_pushboolean(L, 1);<br>
return 1;<br>
+#endif /* !LJ_HASSYSPROF */<br>
}<br>
<br>
/* local res, err, errno = profile.sysprof_stop() */<br>
LJLIB_CF(misc_sysprof_stop)<br>
{<br>
+#if !LJ_HASSYSPROF<br>
+ const char *err_details =
err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
+#else<br>
int status = luaM_sysprof_stop(L);<br>
- if (!LJ_HASSYSPROF) {<br>
- const char *err_details =
err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
- return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
- }<br>
if (LJ_UNLIKELY(status != PROFILE_SUCCESS))<br>
return sysprof_error(L, status, NULL);<br>
<br>
lua_pushboolean(L, 1);<br>
return 1;<br>
+#endif /* !LJ_HASSYSPROF */<br>
}<br>
<br>
/* local counters, err, errno = sysprof.report() */<br>
LJLIB_CF(misc_sysprof_report)<br>
{<br>
+#if !LJ_HASSYSPROF<br>
+ const char *err_details =
err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
+#else<br>
struct luam_Sysprof_Counters counters = {};<br>
GCtab *data_tab = NULL;<br>
GCtab *count_tab = NULL;<br>
int status = luaM_sysprof_report(&counters);<br>
- if (!LJ_HASSYSPROF) {<br>
- const char *err_details =
err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
- return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
- }<br>
-<br>
if (status != PROFILE_SUCCESS)<br>
return sysprof_error(L, status, NULL);<br>
<br>
@@ -379,6 +379,7 @@ LJLIB_CF(misc_sysprof_report)<br>
lua_setfield(L, -2, "vmstate");<br>
<br>
return 1;<br>
+#endif /* !LJ_HASSYSPROF */<br>
}<br>
<br>
/* ----- misc.memprof module
---------------------------------------------- */<br>
@@ -388,14 +389,14 @@ LJLIB_CF(misc_sysprof_report)<br>
/* local started, err, errno = misc.memprof.start(fname) */<br>
LJLIB_CF(misc_memprof_start)<br>
{<br>
+#if !LJ_HASMEMPROF<br>
+ const char *err_details =
err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
+#else<br>
struct lj_memprof_options opt = {0};<br>
const char *fname = strdata(lj_lib_checkstr(L, 1));<br>
struct profile_ctx *ctx;<br>
int memprof_status;<br>
- if (!LJ_HASMEMPROF) {<br>
- const char *err_details =
err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
- return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
- }<br>
<br>
/*<br>
** FIXME: more elegant solution with ctx.<br>
@@ -441,16 +442,17 @@ LJLIB_CF(misc_memprof_start)<br>
}<br>
lua_pushboolean(L, 1);<br>
return 1;<br>
+#endif /* !LJ_HASMEMPROF */<br>
}<br>
<br>
/* local stopped, err, errno = misc.memprof.stop() */<br>
LJLIB_CF(misc_memprof_stop)<br>
{<br>
+#if !LJ_HASMEMPROF<br>
+ const char *err_details =
err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
+ return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
+#else<br>
int status = lj_memprof_stop(L);<br>
- if (!LJ_HASMEMPROF) {<br>
- const char *err_details =
err2msg(LJ_ERR_PROF_DETAILS_DISABLED);<br>
- return sysprof_error(L, PROFILE_ERRUSE, err_details);<br>
- }<br>
if (status != PROFILE_SUCCESS) {<br>
switch (status) {<br>
case PROFILE_ERRUSE:<br>
@@ -474,6 +476,7 @@ LJLIB_CF(misc_memprof_stop)<br>
}<br>
lua_pushboolean(L, 1);<br>
return 1;<br>
+#endif /* !LJ_HASMEMPROF */<br>
}<br>
#endif /* !LJ_TARGET_WINDOWS */<br>
<br>
</p>
<blockquote type="cite" cite="mid:Z7xX-W8u39kfNI2G@root">
<pre wrap="" class="moz-quote-pre">
Here and below.
Also, it helps to avoid an excess call to `lj_{sysprof,memprof}_stop()`.
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ 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(<a class="moz-txt-link-freetext" href="err:match('profiler">err:match('profiler</a> 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(<a class="moz-txt-link-freetext" href="err:match('profiler">err:match('profiler</a> 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(<a class="moz-txt-link-freetext" href="err:match('profiler">err:match('profiler</a> 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(<a class="moz-txt-link-freetext" href="err:match('profiler">err:match('profiler</a> 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(<a class="moz-txt-link-freetext" href="err:match('profiler">err:match('profiler</a> 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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>