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 177BD134873; Tue, 29 Nov 2022 18:34:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 177BD134873 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1669736092; bh=RiDDd8bz2TzWVchVg6xNO+QNun8uUlescu5pn49OIts=; 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=coli0GPcCBjiIZueYftb+VOhDPuwn0f3eEHtGUovxEBPI47Y675rmlSWr1MNspexS Ekr7GG/TGmsf9M2JrL4jkiPxaPAWJEsNaumat2tPIseLef9tw/9iDKPA7jtrYt3B1v Bv44xRxOHQ4vlU3rEm3D285xsQwiK2FedeyISiD0= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [94.100.179.3]) (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 5AD2842542 for ; Tue, 29 Nov 2022 18:34:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5AD2842542 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1p02dG-0001tP-HP; Tue, 29 Nov 2022 18:34:50 +0300 Date: Tue, 29 Nov 2022 18:20:55 +0300 To: Maxim Kokryashkin Message-ID: References: <20221005144331.39666-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20221005144331.39666-1-m.kokryashkin@tarantool.org> X-Clacks-Overhead: GNU Terry Pratchett X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9F70D9A593BA47E2A56F09602343F538C58B4BC6FE7C5B6B3182A05F5380850409C0C4556E59C3A90B32163FBBD15406F3AE5B95AC8E5CDD4B71A667D646C32D4 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B8498AC83B273C12EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006373C9FC9F3BACECB908638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8537DE18A75CB5940ACFAADF5B4992F46117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC3A703B70628EAD7BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520CCD848CCB6FE560C618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6F7FD1A3A8AE6177F089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34F6110710A6CC527FBD878CE802DEE5BC24B2DD9FD28956C4DF7D118842D6194EC7A360D4FF2B53D11D7E09C32AA3244C0BD79037E1DC1A33BADC449E742D37FB51E887DA02A9F7BF927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojLCIl0w9hXVwYb6HjUDo5ew== X-DA7885C5: 0AC235B90861AD61F8A0D4DFF1F7F72735C9FD2FAD89EEC295C97724FEA617A4262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73933AF1F914F131DBF54CB8553BB76E373ACE99AAF598A41E92A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E3365FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] luajit-gdb: support full-range 64-bit lightud 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Max, Thanks for the patch! LGTM in general, but consider my nits below. On 05.10.22, Maxim Kokryashkin via Tarantool-patches wrote: > Following up the introduction of full-range 64-bit lightuserdata > support in commit 2cacfa8 ("Add support for full-range 64 bit > lightuserdata."), this patch modifies the corresponding dumper > behavior for LJ_64 platforms in the luajit-gdb extension. > > Resolves tarantool/tarantool#6481 > --- > Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-6481-luajit-gdb-light-ud > Issue: https://github.com/tarantool/tarantool/issues/6481 > src/luajit-gdb.py | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py > index 6480d014..2993f2c1 100644 > --- a/src/luajit-gdb.py > +++ b/src/luajit-gdb.py > @@ -166,6 +166,9 @@ LJ_GCVMASK = ((1 << 47) - 1) > LJ_TISNUM = None > PADDING = None > > +LJ_LIGHTUD_BITS_SEG = 8 > +LJ_LIGHTUD_BITS_LO = 47 - LJ_LIGHTUD_BITS_SEG I have no strong opinion regarding this constant: on the one hand we're trying to be as much close to LuaJIT sources here as we can, but Sergos' comment looks valid either. I leave the final decision regarding this on your mighty shoulders, since I'm OK with both introducing the constant or leaving everything as is. > + > # }}} > > def itype(o): > @@ -315,6 +318,12 @@ def frames(L): > break > framelink = frame_prev(framelink) > > +def lightudseg(u): > + return (u >> LJ_LIGHTUD_BITS_LO) & ((1 << LJ_LIGHTUD_BITS_SEG) - 1) > + > +def lightudlo(u): > + return u & ((1 << LJ_LIGHTUD_BITS_LO) - 1) Regarding this function and your change in the final version, I have a strong opinion, that we have to respect LuaJIT sources as close as we can, so even if you decided to expand the macros (and hence do not use the corresponding function in the Python extension), please do similar work GCC does when expanding macros with -E parameter: add the comment with the macro name that is expanded. Consider the following: | def lightudV(tv): | if LJ_64: | u = int(tv['u64']) | # lightudseg macro expanded. | seg = (u >> LJ_LIGHTUD_BITS_LO) & LIGHTUD_SEG_MASK | segmap = mref('uint32_t *', G(L(None))['gc']['lightudseg']) | # lightudlo macro expanded. | return (int(segmap[seg]) << 32) | (u & LIGHTUD_LO_MASK) | else: | return gcval(tv['gcr']) Furthermore, why don't you consider hoisting the compile time condition and create two functions to be specialized on the startup as lightudV implementation? I doubt, this would change the performance (at least this is not the key achievement), but the readability may be increased. Anyway feel free to ignore the last comment. > + > # Dumpers {{{ > > def dump_lj_tnil(tv): > @@ -327,7 +336,14 @@ def dump_lj_ttrue(tv): > return 'true' > > def dump_lj_tlightud(tv): > - return 'light userdata @ {}'.format(strx64(gcval(tv['gcr']))) > + if LJ_64: > + u = int(tv['u64']) > + seg = lightudseg(u) > + segmap = mref('uint32_t *', G(L(None))['gc']['lightudseg']) > + addr = (int(segmap[seg]) << 32) | lightudlo(u) > + else: > + addr = gcval(tv['gcr']) > + return 'light userdata @ {}'.format(strx64(addr)) > > def dump_lj_tstr(tv): > return 'string {body} @ {address}'.format( > -- > 2.36.1 > -- Best regards, IM