[Tarantool-patches] [PATCH luajit 4/8][v3] sysprof: introduce specific errors and default mode
Sergey Bronnikov
sergeyb at tarantool.org
Wed Mar 5 13:48:09 MSK 2025
Hi, Sergey!
Changes applied and force-pushed.
Sergey
On 05.03.2025 10:55, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, after fixing 3 minor nits below.
>
> On 24.02.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> changes applied and force-pushed.
>>
>> Sergey
>>
>> On 24.02.2025 15:46, Sergey Kaplun via Tarantool-patches wrote:
>>> Hi, Sergey!
>>> Thanks for the fixes!
>>> LGTM, after the final polishing, please consider my comments below.
>>>
>>> On 20.02.25, Sergey Bronnikov wrote:
> <snipped>
>
>>>> ---
>>>> src/lib_misc.c | 77 ++++++++++++-------
>>>> src/lj_errmsg.h | 5 ++
>>>> .../profilers/misclib-sysprof-lapi.test.lua | 68 ++++++++++++++--
>>>> 3 files changed, 114 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>>>> index 5b7a4b62..1fd06dd1 100644
>>>> --- a/src/lib_misc.c
>>>> +++ b/src/lib_misc.c
>>>> @@ -14,6 +14,7 @@
>>>>
>>>> #include "lj_obj.h"
>>>> #include "lj_str.h"
>>>> +#include "lj_strfmt.h"
>>> Please add the corresponding include (after lj_str.h) in <src/Makefile.dep.original> to
>>> avoid conflicts when we use this file (for crossbuilds or whatever).
> There is no corresponding update on the branch.
Sorry, missed this comment.
Fixed:
--- a/src/Makefile.dep.original
+++ b/src/Makefile.dep.original
@@ -30,7 +30,7 @@ lib_jit.o: lib_jit.c lua.h luaconf.h lauxlib.h
lualib.h lj_obj.h lj_def.h \
lib_math.o: lib_math.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \
lj_def.h lj_arch.h lj_lib.h lj_vm.h lj_libdef.h
lib_misc.o: lib_misc.c lua.h luaconf.h lmisclib.h lauxlib.h lj_obj.h \
- lj_def.h lj_arch.h lj_str.h lj_tab.h lj_lib.h lj_gc.h lj_err.h \
+ lj_def.h lj_arch.h lj_str.h lj_strfmt.h lj_tab.h lj_lib.h lj_gc.h
lj_err.h \
lj_errmsg.h lj_memprof.h lj_wbuf.h lj_libdef.h
lib_os.o: lib_os.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h lj_def.h \
lj_arch.h lj_gc.h lj_err.h lj_errmsg.h lj_buf.h lj_str.h lj_lib.h \
>
> <snipped>
>
>>>> -static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, int idx) {
>>>> - GCtab *options = lj_lib_checktab(L, idx);
>>>> +static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,
>>>> + const char **err_details) {
>>> Nit: Please, replace every 8 spaces with one tab.
>> Fixed:
>>
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -180,7 +180,7 @@ static int set_output_path(const char *path, struct
>> luam_Sysprof_Options *opt) {
>> }
>>
>> static int parse_sysprof_opts(lua_State *L, struct
>> luam_Sysprof_Options *opt,
>> - const char **err_details) {
>> + const char **err_details) {
> Thanks!
> Please apply the following patch. It fixes all patchset-related places.
Applied.
>
> ===================================================================
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index 94ec6564..79cfaf7b 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -199,13 +199,13 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,
> cTValue *mode_opt = lj_tab_getstr(options, lj_str_newlit(L, "mode"));
> if (mode_opt) {
> if (!tvisstr(mode_opt)) {
> - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
> - return PROFILE_ERRUSE;
> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
> + return PROFILE_ERRUSE;
> }
> mode = strVdata(mode_opt);
> if (strlen(mode) == 0 || mode[1] != '\0') {
> - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
> - return PROFILE_ERRUSE;
> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
> + return PROFILE_ERRUSE;
> }
> }
>
> @@ -223,8 +223,8 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,
> opt->mode = LUAM_SYSPROF_CALLGRAPH;
> break;
> default:
> - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
> - return PROFILE_ERRUSE;
> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
> + return PROFILE_ERRUSE;
> }
> }
>
> @@ -235,8 +235,8 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,
> if (interval && tvisnumber(interval)) {
> int32_t signed_interval = numberVint(interval);
> if (signed_interval < 1) {
> - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL);
> - return PROFILE_ERRUSE;
> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL);
> + return PROFILE_ERRUSE;
> }
> opt->interval = signed_interval;
> }
> @@ -281,9 +281,9 @@ static int sysprof_error(lua_State *L, int status, const char *err_details)
> case PROFILE_ERRUSE:
> lua_pushnil(L);
> if (err_details)
> - lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
> + lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
> else
> - lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
> + lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
> lua_pushinteger(L, EINVAL);
> return 3;
> #if LJ_HASSYSPROF
> ===================================================================
>
>> int n = (int)(L->top - L->base);
> <snipped>
>
>>>> +test:is(res, false, "res with not a table")
>>>> +test:ok(err:match("table expected, got string"),
>>>> + "error with not a table")
>>>> +test:is(errno, nil, "errno with not a table")
>>> There is no need to check or declare the `errno` variable. It's always
>>> `nil` for the case when the error is raised.
>> The check is cheap, why not?
> This is kind of misleading, plus this is a dead check. It is __always__
> `nil`, or we will get an error in the first test and the second test.
> Since we are not testing `pcall()` implementation here, let's just drop
> it (it is even cheaper :)).
okay, removed:
@@ -85,11 +85,10 @@ test:ok(ok == false and err_msg:match("cannot open a
file"),
assert(misc.sysprof.stop())
-- Not a table.
-res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")
+res, err = pcall(misc.sysprof.start, "NOT A TABLE")
test:is(res, false, "res with not a table")
test:ok(err:match("table expected, got string"),
"error with not a table")
-test:is(errno, nil, "errno with not a table")
-- All parameters are invalid. Check parameter validation order
-- (not strict documented behaviour).
>
> <snipped>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20250305/a0c8f84c/attachment.htm>
More information about the Tarantool-patches
mailing list