[Tarantool-patches] [PATCH luajit 6/7] misc: specific message for disabled profilers
Sergey Kaplun
skaplun at tarantool.org
Wed Feb 19 11:06:42 MSK 2025
Hi, Sergey!
Thanks for the patch!
LGTM, after fixing minor nits below.
On 13.02.25, Sergey Bronnikov wrote:
> sysprof and memprof Lua API functions returns an error message
Typo: s/returns/return/
> "profiler misuse", when appropriate profiler is disabled in build.
Typo: s/appropriate/the corresponding/
Typo: s/build/the build/
> It is not possible to easily distinquish whether it is really
Typo: s/distinquish/distinguish/
> misuse or profiler was not enabled in build. The patch changes
Typo: s/profiler/or if the profiler/
Typo: s/build/the build/
> errors messages, so when profiler was not enabled in build message
Typo: s/errors/error/
Typo: s/profiler/a profiler/
Typo: s/was/is/
Typo: s/in build message/in the build, the message/
> is the following: "profiler misuse: profiler is disabled".
> ---
> src/lib_misc.c | 27 ++++++++++++++++-
> src/lj_errmsg.h | 1 +
> .../misclib-memprof-lapi-disabled.test.lua | 22 ++++++++++++++
> .../misclib-sysprof-lapi-disabled.test.lua | 29 +++++++++++++++++++
> 4 files changed, 78 insertions(+), 1 deletion(-)
> 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 d71904e4..7666d85f 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -315,10 +315,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)
> {
> + const char *err_details = NULL;
> +#if !LJ_HASSYSPROF
It's more LuaJIT-way to use something like the following:
| if (!LJ_HASSYSPROF) {
| /* ... */
| }
This helps to avoid strange early return.
Also, please be avaired to declare all variables (stataus, opt,
err_details) in the beginning of the block.
Here and below.
> + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASSYSPROF */
> +
> int status = PROFILE_SUCCESS;
>
> struct luam_Sysprof_Options opt = {};
> - const char *err_details = NULL;
>
> status = parse_sysprof_opts(L, &opt, &err_details);
> if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
> @@ -336,6 +341,11 @@ LJLIB_CF(misc_sysprof_start)
> /* local res, err, errno = profile.sysprof_stop() */
> LJLIB_CF(misc_sysprof_stop)
> {
> +#if !LJ_HASSYSPROF
> + const char *err_details = NULL;
> + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
Looks like we just can do the following for this block:
| const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
Here and below.
> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASSYSPROF */
> int status = luaM_sysprof_stop(L);
> if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
> return sysprof_error(L, status, NULL);
> @@ -347,6 +357,11 @@ LJLIB_CF(misc_sysprof_stop)
> /* local counters, err, errno = sysprof.report() */
> LJLIB_CF(misc_sysprof_report)
> {
> +#if !LJ_HASSYSPROF
> + const char *err_details = NULL;
> + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASSYSPROF */
> struct luam_Sysprof_Counters counters = {};
> GCtab *data_tab = NULL;
> GCtab *count_tab = NULL;
> @@ -386,6 +401,11 @@ 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 = NULL;
> + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASMEMPROF */
> struct lj_memprof_options opt = {0};
> const char *fname = strdata(lj_lib_checkstr(L, 1));
> struct profile_ctx *ctx;
> @@ -440,6 +460,11 @@ LJLIB_CF(misc_memprof_start)
> /* local stopped, err, errno = misc.memprof.stop() */
> LJLIB_CF(misc_memprof_stop)
> {
> +#if !LJ_HASMEMPROF
> + const char *err_details = NULL;
> + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);
> + return sysprof_error(L, PROFILE_ERRUSE, err_details);
> +#endif /* !LJ_HASMEMPROF */
> int status = lj_memprof_stop(L);
> if (status != PROFILE_SUCCESS) {
> switch (status) {
> 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")
I suppose this should be declared under `#else` directive (i.e. if we
have at least one profiler 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..a0669cc6
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua
> @@ -0,0 +1,22 @@
> +local tap = require("tap")
Please use single quotes everywhere across this file for the
consistency.
> +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 memprof is disabled.
Minor: s/start memprof when memprof/start memprof when it/
> +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 memprof is disabled.
Minor: s/stop memprof when memprof/stop memprof when it/
> +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..79707434
> --- /dev/null
> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua
> @@ -0,0 +1,29 @@
> +local tap = require("tap")
Please use single quotes everywhere across this file for the
consistency.
> +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.34.1
>
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list