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 ED52437202D; Wed, 15 Mar 2023 09:52:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org ED52437202D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1678863125; bh=uY4IXRDAwPg2AtVGDS/cg8l9u6lo83G+2jkby0Tj9sg=; 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=U+/aRz977lVG0AQ0UNRJ+YForuJmjskWgGdxFs3+Oqc+c2GSVAhqMQq1OxoWx/Nil 3HX/W9At7Wo/c/+6iWe1Zf33emHjNz5qOy0w5hP61to1qlp5Bq4IYTQ9+pOTdbcMN9 enfMxlFPeRsFKFn5Zv0bHpRrovn4d1Y08Zhxjxio= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id AC06463EB1 for ; Wed, 15 Mar 2023 09:52:03 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AC06463EB1 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1pcKzS-0004Sh-Jg; Wed, 15 Mar 2023 09:52:03 +0300 Date: Wed, 15 Mar 2023 09:48:20 +0300 To: Maksim Kokryashkin Message-ID: References: <20230314152507.37168-1-max.kokryashkin@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230314152507.37168-1-max.kokryashkin@gmail.com> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9BCEC41593EBD8357ED47A4A6F8E39D1627AD6C6CCC9DF7C0182A05F5380850403E4318A6EAB44F9A47469BDD13DC9B68CC42D6D4C004CC323168CBC5B19B565B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7589DF9800DB1E191EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637DD75BBC768D349DD8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88D44E464EF693667D2806922DE2CE313117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC0E007FA4003F52FEA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735200AC5B80A05675ACDBDFBBEFFF4125B51D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6300D3B61E77C8D3B089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D348CDF4129B2EA8560B23E936F6D254019C6C6C2417745B6AC768D7AA54842013DAC3F76F3677FEB4C1D7E09C32AA3244CA44EB9C3E9D45837F4E1E61F20FA51B07C0C08F7987826B9927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojOXwBDQf4j7N/c4964kjWNw== X-DA7885C5: E96A601E0ABCC6277102EF82B14359E48C403C2CA1B6908DD9CFE12B904D5210262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73933AF1F914F131DBF54DBA8459E6B651BC30FF40149F6793750FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] codehealth: fix sign extension in sysprof counters 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, Maxim! Thanks for the patch! Please, consider my comments below. On 14.03.23, Maksim Kokryashkin wrote: > The `setnumfield` function takes `int64_t` as an argument. I see, that this function is also used for the `getmetrics` API to fill the similar structure. It uses size_t type, instead of uint64_t. Should we obscure the similar issue for this type? If no, maybe it's better to use size_t here instead to emphasize that these values are unsigned? If yes, we should fix both places. > Profile counters used to have the `uint64_t` type before > this patch, which could lead to issues with sign extension. Can you provide some examples of such issues? If it is about type overflow, that is kinda highly unlikely (same about getmetrics fields even for strhash_{hit,miss} and gc_{allocated,freed}): | perl -E 'say 0x7fffffffffffffff / 1000 / 60 / 60 /24 / 366' | 291672107.014483 And the value still "can overflow" but just become negative instead. > Now their type is `int64_t`, so they correspond to the > `setnumfield` argument type. > --- > PR: https://github.com/tarantool/tarantool/pull/8440 > Branch: https://github.com/tarantool/luajit/tree/fckxorg/svace-sysprof-report > src/lj_sysprof.c | 2 +- > src/lmisclib.h | 22 +++++++++++----------- > 2 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c > index 2e9ed9b3..4723e6f9 100644 > --- a/src/lj_sysprof.c > +++ b/src/lj_sysprof.c > @@ -285,7 +285,7 @@ static void sysprof_record_sample(struct sysprof *sp, siginfo_t *info) > lua_assert(pthread_self() == sp->thread); > > /* Caveat: order of counters must match vmstate order in . */ > - ((uint64_t *)&sp->counters)[vmstate]++; > + ((int64_t *)&sp->counters)[vmstate]++; > > sp->counters.samples++; > > diff --git a/src/lmisclib.h b/src/lmisclib.h > index 9084319c..599a6ea9 100644 > --- a/src/lmisclib.h > +++ b/src/lmisclib.h > @@ -105,21 +105,21 @@ typedef void (*luam_Sysprof_backtracer)(void *(*frame_writer)(int frame_no, void > #define LUAM_SYSPROF_CALLGRAPH 2 > > struct luam_Sysprof_Counters { > - uint64_t vmst_interp; > - uint64_t vmst_lfunc; > - uint64_t vmst_ffunc; > - uint64_t vmst_cfunc; > - uint64_t vmst_gc; > - uint64_t vmst_exit; > - uint64_t vmst_record; > - uint64_t vmst_opt; > - uint64_t vmst_asm; > - uint64_t vmst_trace; > + int64_t vmst_interp; > + int64_t vmst_lfunc; > + int64_t vmst_ffunc; > + int64_t vmst_cfunc; > + int64_t vmst_gc; > + int64_t vmst_exit; > + int64_t vmst_record; > + int64_t vmst_opt; > + int64_t vmst_asm; > + int64_t vmst_trace; > /* > ** XXX: Order of vmst counters is important: it should be the same as the > ** order of the vmstates. > */ > - uint64_t samples; > + int64_t samples; > }; > > /* Profiler options. */ > -- > 2.37.1 (Apple Git-137.1) > -- Best regards, Sergey Kaplun