Hi, Sergey,
thanks for the comments!
Hi, Sergey! Thanks for the patch! Please consider my comments below. On 04.02.25, Sergey Bronnikov wrote:sysprof has a number of options and with any incorrect option it returns `false` and error message "profiler misuse". This message does not descriptive for sysprof users and make using sysprof more complicated. The patch splits error `PROFILE_ERRUSE` to four specific errors: `PROFILE_ERRBADMODE`, `PROFILE_ERRBADINTERVAL`, `PROFILE_ERRBADPATH` and `PROFILE_ERRBADTABLE`, and use default profiling mode ("C", callgraph) if it was not passed.I don't like this approach: Now we have profiler-specific statuses (valid only for sysprof) in the public API. They have no meaning for memprof. Also, that makes the definition of errors more twisted.
Also, sysprof has options that have no meaning for memprof:
mode and interval, and sysprof accepts a table when memprof accepts
a string. It is not considered as a problem for you, but specific messages
is a problem. Seems there is an inconsistency.
Plus, the error message is a little bit incompatible with the previous version (you may notice this in test changes).
We discussed this verbally (you, Sergos and me) and decided that it is
not a problem.
I suggest the following approach: We append all additional details in the error message, so it looks like: | profiler misuse: sysprof mode must be 'D', 'L' or 'C' The implementation is also simple.
Honestly, it becomes more complicated in code, because error messages are not error messages,
but error prefixes, and now you should care about two things: error message and details.
Anyway, I agree that introducing error details is a compromise in a current situtation.We don't need any additional statuses for error messages, just to return in the `char **` pointer to string as additional details. So the usage is the following: `parse_sysprof_opts(L, &opts, &err_details);` `sysprof_error(L, &opts, err_details); /*details may be NULL here */` Pros: * No custom statuses for sysprof in the public API (or reserved for internal usage). * More backward compatible with previous errors -- error message still matches with 'profiler misuse'. * No additional custom error definitions. * More consistent behaviour between Lua and C API -- as we operate only `PROFILER_MISUSE` status everywhere. * It simplifies error handling in `sysprof_error()` -- we just need to check if the given argument is NULL.
Merged.Cons: * Additional argument to the internal functions `sysprof_error()`, `parse_sysprof_opts()`. Side note: I believe, since we have no valuable logic in the `parse_opts()`, its body can be merged in the `parse_sysprof_opts()`).
It is not reproduced with updated patchset.See my comment below. Friendly reminder: Also, I got the following error when building LuaJIT with sysprof disabled (we discussed it online): | src/luajit -e 'misc.sysprof.start()' | LuaJIT ASSERT /home/burii/reviews/luajit/sysprof-fixes/src/lj_state.c:214: close_state: memory leak of 8388624 bytes | Aborted (core dumped) It seems that this is an old problem unrelated to your patchset. This happens for a non-default mode if we use the build with disabled sysprof. Looks like we still allocated the write buffer for sysprof for some reason.
Fixed. (I remember you told me that would be good to set "C" as default mode)--- src/lib_misc.c | 47 ++++++++++++++----- src/lj_errmsg.h | 4 ++ src/lmisclib.h | 5 ++ .../profilers/misclib-sysprof-lapi.test.lua | 45 ++++++++++++++++-- 4 files changed, 85 insertions(+), 16 deletions(-) diff --git a/src/lib_misc.c b/src/lib_misc.c index 5b7a4b62..a28e4a3c 100644 --- a/src/lib_misc.c +++ b/src/lib_misc.c @@ -163,6 +163,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf) /* The default profiling interval equals to 10 ms. */ #define SYSPROF_DEFAULT_INTERVAL 10 +#define SYSPROF_DEFAULT_MODE "C"Why is the default mode C and not D?
#define SYSPROF_DEFAULT_OUTPUT "sysprof.bin" static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) { @@ -185,13 +186,16 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in const char *mode = NULL; cTValue *mode_opt = lj_tab_getstr(options, lj_str_newlit(L, "mode")); - if (!mode_opt || !tvisstr(mode_opt)) { - return PROFILE_ERRUSE; + if (mode_opt) { + if (!tvisstr(mode_opt)) + return PROFILE_ERRBADMODE; + mode = strVdata(mode_opt); + if (mode[1] != '\0')If we have `mode = ''`, this will read the byte from `g->hookmask`. The result is always `PROFILER_ERRBADMODE` (in the current terms), but it is still dirty reads.
Fixed by checking a string len:
}
mode = strVdata(mode_opt);
- if (mode[1] != '\0') {
+ if (strlen(mode) > 0 && mode[1] != '\0') {
*err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE);
return PROFILE_ERRUSE;
}
(1/1) Stage this h
Removed parse_options.+ return PROFILE_ERRBADMODE; } - mode = strVdata(mode_opt); - if (mode[1] != '\0') - return PROFILE_ERRUSE; + if (!mode) + mode = SYSPROF_DEFAULT_MODE; switch (*mode) { case 'D': @@ -204,7 +208,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in opt->mode = LUAM_SYSPROF_CALLGRAPH; break; default: - return PROFILE_ERRUSE; + return PROFILE_ERRBADMODE; } } @@ -215,7 +219,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in if (interval && tvisnumber(interval)) { int32_t signed_interval = numberVint(interval); if (signed_interval < 1) - return PROFILE_ERRUSE; + return PROFILE_ERRBADINTERVAL; opt->interval = signed_interval; } } @@ -231,7 +235,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in if (!pathtv) path = SYSPROF_DEFAULT_OUTPUT; else if (!tvisstr(pathtv)) - return PROFILE_ERRUSE; + return PROFILE_ERRBADPATH; else path = strVdata(pathtv); @@ -253,11 +257,12 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in static int parse_options(lua_State *L, struct luam_Sysprof_Options *opt)I suppose this helper function isn't needed. It is not related to memprof, so its name is misleading. Its body may be merged with `parse_sysprof_options()` to avoid these problems.
{ - if (lua_gettop(L) != 1) - return PROFILE_ERRUSE; + if (lua_gettop(L) != 1) {If there are 2+ arguments with different content, we still create a table. I suppose there is no need for table creation. We may just set the default options at once without additional processing and creation of the table.
Good advice! Thanks, fixed.
@@ -180,8 +180,11 @@ 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) {
- if (lua_gettop(L) != 1) {
- lua_createtable(L, 0, 0);
+ int n = (int)(L->top - L->base);
+ if (n != 1) {
+ opt->mode = LUAM_SYSPROF_DEFAULT;
+ opt->interval = SYSPROF_DEFAULT_INTERVAL;
+ goto set_path;
}
if (!lua_istable(L, 1)) {
@@ -241,6 +244,8 @@ static int parse_sysprof_opts(lua_State *L,
struct luam_Sysprof_Options *opt,
}
}
+set_path:
+
/* Get output path. */
if (opt->mode != LUAM_SYSPROF_DEFAULT)
{
Fixed, thanks!Minor: Also, maybe it is better to use `(int)(L->top - L->base)` instead of the call to the `lua_gettop()` for consistency with the internal LuaJIT code base. I know that this function does the same thing, but since it is the public API, this call isn't inlined.
+ lua_createtable(L, 0, 0); + } if (!lua_istable(L, 1)) - return PROFILE_ERRUSE; + return PROFILE_ERRBADTABLE; return parse_sysprof_opts(L, opt, 1); } @@ -270,6 +275,26 @@ static int sysprof_error(lua_State *L, int status) lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); lua_pushinteger(L, EINVAL); return 3; + case PROFILE_ERRBADMODE: + lua_pushnil(L); + lua_pushstring(L, err2msg(LJ_ERR_PROF_BADMODE)); + lua_pushinteger(L, EINVAL); + return 3; + case PROFILE_ERRBADINTERVAL: + lua_pushnil(L); + lua_pushstring(L, err2msg(LJ_ERR_PROF_BADINTERVAL)); + lua_pushinteger(L, EINVAL); + return 3; + case PROFILE_ERRBADPATH: + lua_pushnil(L); + lua_pushstring(L, err2msg(LJ_ERR_PROF_BADPATH)); + lua_pushinteger(L, EINVAL); + return 3; + case PROFILE_ERRBADTABLE: + lua_pushnil(L); + lua_pushstring(L, err2msg(LJ_ERR_PROF_BADTABLE)); + lua_pushinteger(L, EINVAL); + return 3; #if LJ_HASSYSPROF case PROFILE_ERRRUN: lua_pushnil(L); diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h index 19c41f0b..9713d6c7 100644 --- a/src/lj_errmsg.h +++ b/src/lj_errmsg.h @@ -183,6 +183,10 @@ ERRDEF(FFI_NYICALL, "NYI: cannot call this C function (yet)") /* Profiler errors. */ ERRDEF(PROF_MISUSE, "profiler misuse") +ERRDEF(PROF_BADMODE, "profiler mode must be 'D', 'L' or 'C'") +ERRDEF(PROF_BADINTERVAL, "profiler interval must be greater than 1") +ERRDEF(PROF_BADPATH, "profiler path does not exist") +ERRDEF(PROF_BADTABLE, "profiler expects a table with parameters")The errors are sysprof-specific, so they should be defined under the corresponding ifdef. The comment is irrelevant if you consider my suggestion above.
Fixed.
these additional err codes were removed#if LJ_HASMEMPROF || LJ_HASSYSPROF ERRDEF(PROF_ISRUNNING, "profiler is running already") ERRDEF(PROF_NOTRUNNING, "profiler is not running") diff --git a/src/lmisclib.h b/src/lmisclib.h index 9084319c..b4eb0b57 100644 --- a/src/lmisclib.h +++ b/src/lmisclib.h @@ -142,6 +142,11 @@ struct luam_Sysprof_Options { #define PROFILE_ERRMEM 3 #define PROFILE_ERRIO 4 +#define PROFILE_ERRBADMODE 5 +#define PROFILE_ERRBADINTERVAL 6 +#define PROFILE_ERRBADPATH 7 +#define PROFILE_ERRBADTABLE 8The errors are sysprof-specific, so they should be defined under the corresponding ifdef. Also, they are blocking the future introduction of profiler-general errors. The comment is irrelevant if you consider my suggestion above.
Fixed.+ LUAMISC_API int luaM_sysprof_set_writer(luam_Sysprof_writer writer); LUAMISC_API int luaM_sysprof_set_on_stop(luam_Sysprof_on_stop on_stop); diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua index 581fb7fd..68a4b72f 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("misc-sysprof-lapi"):skipcond({ ["Disabled due to #10803"] = os.getenv("LUAJIT_TEST_USE_VALGRIND"), }) -test:plan(19) +test:plan(33) jit.off() -- XXX: Run JIT tuning functions in a safe frame to avoid errors @@ -65,9 +65,25 @@ end -- Wrong profiling mode. local res, err, errno = misc.sysprof.start{ mode = "A" } -test:ok(res == nil and err:match("profiler misuse"), "res with no parameters") +test:ok(res == nil, "res with no parameters") +test:ok(err:match("profiler mode must be 'D', 'L' or 'C'"), + "error with no parameters") test:ok(type(errno) == "number", "errno with no parameters") +-- Missed profiling mode. +res, err, errno = misc.sysprof.start{} +test:is(res, true, "res with missed profiling mode") +test:is(err, nil, "error with missed profiling mode")Typo: s/error/no error/
Fixed.+test:is(errno, nil, "errno with missed profiling mode")Typo: s/errno/no errno/
Removed.+assert(misc.sysprof.stop(), "sysprof is not stopped")I would rather drop the "sysprof is not stopped" -- in that case we will see the reason _why_ it is not stopped successfully instead.
Fixed.+ +-- Not a table. +res, err, errno = misc.sysprof.start("NOT A TABLE") +test:ok(res == nil, "res with not a table") +test:ok(err:match("profiler expects a table with parameters"), + "error with not a table") +test:ok(type(errno) == "number", "errno with not a table") + -- Already running. res, err = misc.sysprof.start{ mode = "D" } assert(res, err) @@ -90,10 +106,29 @@ res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH }) test:ok(res == nil and err:match("No such file or directory"), "res and error with bad path") test:ok(type(errno) == "number", "errno with bad path") --- Bad interval. +-- Bad interval (-1). res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 } -test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval") -test:ok(type(errno) == "number", "errno with bad interval") +test:ok(res == nil, "res with bad interval -1") +test:ok(err:match("profiler interval must be greater than 1"), + "error with bad interval -1") +test:ok(type(errno) == "number", "errno with bad interval -1") + +-- Bad interval (0). +res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 } +test:ok(res == nil, "res with bad interval 0") +test:ok(err:match("profiler interval must be greater than 1"), + "error with bad interval 0") +test:ok(type(errno) == "number", "errno with bad interval 0") + +-- Good interval (1). +res, err, errno = misc.sysprof.start{Here we created a file and don't delete it after the test ends. Maybe it is better to use '/dev/null', since we don't care about the content of the file.
+ mode = "C", + interval = 1, +} +test:is(res, true, "res with good interval 1") +test:is(err, nil, "error with good interval 1") +test:is(errno, nil, "errno with good interval 1") +misc.sysprof.stop() -- DEFAULT MODE -- 2.34.1