From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 21DFB12A46C1; Wed, 19 Feb 2025 11:07:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 21DFB12A46C1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1739952446; bh=pAqxpdS76G71X8q8o65S67qRimekFSpV7mbEGsvmXMU=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=cmd5Wj9zGfr5JVXMdWT+MQZbznQ5ekN5SIHAV1SDBsB2Xjb4r8+R3RcSebk3+9W8B PQA0h2jylCKqoxM+tbIAPPbrC1mLGP1Ah1SbraorxFH82X909XMlJDJyJ220QB8t3a fTvZXfZew5ahuemJYbRdG/Wm8kXydMd4zmCVTLoU= Received: from send196.i.mail.ru (send196.i.mail.ru [95.163.59.35]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7AF9D12A46C0 for ; Wed, 19 Feb 2025 11:07:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7AF9D12A46C0 Received: by exim-smtp-844687bc8-frwp8 with esmtpa (envelope-from ) id 1tkf75-00000000XMn-1QfD; Wed, 19 Feb 2025 11:07:23 +0300 Date: Wed, 19 Feb 2025 11:06:42 +0300 To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Message-ID: References: <7f83ca101b76bba7b5789a6c5a7e9acb8044fed1.1739444510.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7f83ca101b76bba7b5789a6c5a7e9acb8044fed1.1739444510.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD916C41472748AFA04C2817BA953C7B1C761AF83692520876400894C459B0CD1B9D01E946C26226DD17A388D641E84891FAA9D95C439463992EF6669252E7417080DE140DF427568CF X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7C6068CE86C2B75F5EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637DD7A7F9003AF293F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8C5F12B9A94535F515EB2293029893E77F79D48AC9DEF84AACC7F00164DA146DAFE8445B8C89999728AA50765F7900637F924B32C592EA89F389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8989FD0BDF65E50FBF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C2A336C65186350912D242C3BD2E3F4C64AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C39A1C9D3BA3303E89BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE74ABCC139FF3F849B731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A58ADDB4E92C2772D95002B1117B3ED696787A0057B1790B3C3E67C18142C611B7823CB91A9FED034534781492E4B8EEADB1D70E2111C441FFBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFF7C259A82B4D3344701FCD90346DE73A8475D2031DB00330DF2457B8ECC5F32FA4BD9291AB2A84560526A8B1FD8BF66B6E048E73BAEFC4D30BA4A11F8CD5EAB0783E99685156E9BF111DC66A97D0BFE2913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVWiyXSWEEqdrDcOhoGG3a+Q= X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B59F320081CDD5F19D73DE06ABAFEAF67055CF76F6B3B6017B9B7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 6/7] misc: specific message for disabled profilers X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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