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 7F386F36F91; Mon, 24 Feb 2025 14:29:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7F386F36F91 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1740396580; bh=8VWz/4IVK1ijcol0exN1cEjatx6A4xAgTQOB/euHCrE=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=py9enOHg/VN4C3EHahyy3drJ4PoQ3w2L6hNtlIPa45TwgAcD2wDjOoUGv/ubGH2Ni RmQncIEaaYGUyR9WykV3tCoNmf90DfylMa6gczPjvZy86gd1lILyWJ7e+PnslQzNwc trO+j4FTz1Iy+HQnX3aC3DhrTR0ZExAHwf+YJUNo= Received: from send81.i.mail.ru (send81.i.mail.ru [89.221.237.176]) (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 DD039F36F91 for ; Mon, 24 Feb 2025 14:29:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DD039F36F91 Received: by exim-smtp-855d9bd6f7-n8dk2 with esmtpa (envelope-from ) id 1tmWeY-000000006n2-1Cgh; Mon, 24 Feb 2025 14:29:38 +0300 Date: Mon, 24 Feb 2025 14:28:57 +0300 To: Sergey Bronnikov Message-ID: References: <4080c2556ca20ee272c4283ff75361c43e39d674.1740050074.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4080c2556ca20ee272c4283ff75361c43e39d674.1740050074.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD916C41472748AFA04848245097E48316D09B3304720E2CFD400894C459B0CD1B9FA1320B01C7474B22EB5D77EF37489D1D39002D1BE7B5D7EF3B29FE988C368064B72E82432766FBE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A3DED2DACB82E709C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE77F3E0A4828C8B68EEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BC08E230531AC9C9073E23ADD98C2D8F22A9E7E99E84EE9100B3167FC299A1C88A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3CF36E64A7E3F8E58117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CFD22EEC73CD09A32ABA3038C0950A5D36C8A9BA7A39EFB766D91E3A1F190DE8FDBA3038C0950A5D36D5E8D9A59859A8B63271950E746CB25C76E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C565C1E6824D8037B43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A58C992C4621599EFC5002B1117B3ED6960136855D7E601A7E8B25839F35DFE037823CB91A9FED034534781492E4B8EEAD37F46C620FF2CAEEBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF30E7F3983E479FE7302B616FB231436885AB350E46FF7743D521912CE68C36BEBDB65CFD5BA197A7DD1EB5C6CC1B9E94E3D590A13341F0685EDEF0ADCF38AFA6A32148D49ED305A05F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVQQG/FugD0/Cz89qzU/t510= X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B592B4D8456E54AE1103DE06ABAFEAF670501B6839E41D6B8F1B7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 7/8][v3] 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 Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the fixes! LGTM, after fixing my comments below. On 20.02.25, Sergey Bronnikov wrote: > 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: "profiler misuse: > profiler is disabled". > --- > 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) > { 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. Here and below. Also, it helps to avoid an excess call to `lj_{sysprof,memprof}_stop()`. > + 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(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 it is disabled. > +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..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(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.43.0 > -- Best regards, Sergey Kaplun