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 A87766ECC0; Thu, 7 Apr 2022 12:49:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A87766ECC0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1649324964; bh=PEGFxpnnpV3yHsm/M5b9Wwp4P0g5I1D0315NZMtRKyo=; 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=ouOa1PvCQzFcAXAjiT6L3r8i0SIbHGtqfOI7IX6utXLZgprdvsFEksdlbF+Gd29os J0PhFHq9lL+nHKkDfbhB4ly8/MKV0nPF1eRWryrfP3L3w02mt+i5IZDKYnrdDlRtkL YgU3E1th7rdnVVmt8Cwp06aHKIr31E4wVC28clcg= 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 D0CCA6ECC0 for ; Thu, 7 Apr 2022 12:49:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D0CCA6ECC0 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1ncOlV-0004ID-Ko; Thu, 07 Apr 2022 12:49:22 +0300 Date: Thu, 7 Apr 2022 12:47:13 +0300 To: Maxim Kokryashkin Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD90FA8EB11E8F64C5623511C2B057BE7EEE268055ACFC0C190182A05F53808504016F53E1DD3C86ED7310547E91FB704119CE7AD2FF6C75E53106BBECA644088AE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE788A2BECDB72E1542EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637531CC3E3F637A59A8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8EA87CCDA9CD8D01D2247E8E0B1A6E6DF117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B2EE5AD8F952D28FBA471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C565C1E6824D8037B43847C11F186F3C59DAA53EE0834AAEE X-8FC586DF: 6EFBBC1D9D64D975 X-B7AD71C0: 4965CFDFE0519134C1FE400A9E48C5401DD40DE57556AFB266D16FC5F53507A1816E0A2A8F779BBED8D40077074E805C66D16FC5F53507A117535B0CF9F6D0C3EE9D5CB6078CC77C1CBCF3B334B3B244EE319BF62A11F40A X-C1DE0DAB: 0D63561A33F958A56284099B15057FAB9B6DC67F6177970F1B2E65555489F511D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7506FE1F977233B9BB410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C2E47CA9A5606090F588364CFBA3A455D3B5070FBB92228615E0025A8DBD15902E529A736A2780E11D7E09C32AA3244C62FF73EF6A6DA16988CAAA3C394EBE61C3B3ADDA61883BB5927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojzDs61/8Pi1EOP7wrhdT6DA== X-Mailru-Sender: 689FA8AB762F739339CABD9B3CA9A7D60994A778B2CAE1C0660B4AD68E1604610FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v3 1/7] vm: save topframe info into global_State 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 review comments below. However, they are almost the same as for the previous (v1) version, since there is no feedback nor related changes. On 06.04.22, Maxim Kokryashkin wrote: > From: Mikhail Shishatskiy > > Since commit 111d377d524e54e02187148a1832683291d620b2 > ('vm: introduce VM states for Lua and fast functions') > the VM has LFUNC and FFUNC states. The upcoming sampling > profiler uses these vmstates to determine if the guest > stack is valid or not. So, we need to provide a There is an inconsistent behavior > of the VM when the Lua stack is not valid, but the state > is set to LFUNC. This patch is just a gross hack with which > the profiler works fine. About what hack are you talking about? > The problem is to be investigated > more deeply :( Typo: s/ :(/./. Minor: I suggest to drop this line. Minor: lines of commit message look un-filled well (except the one line that filled to the brim :). > --- > src/lj_obj.h | 12 ++++++++++++ > src/vm_x64.dasc | 52 +++++++++++++++++++++++++++++++++++++++---------- > src/vm_x86.dasc | 52 +++++++++++++++++++++++++++++++++++++++---------- > 3 files changed, 96 insertions(+), 20 deletions(-) > > diff --git a/src/lj_obj.h b/src/lj_obj.h > index d26e60be..b76c3155 100644 > --- a/src/lj_obj.h > +++ b/src/lj_obj.h > @@ -514,6 +514,17 @@ typedef struct GCtab { > #define setfreetop(t, n, v) (setmref((n)->freetop, (v))) > #endif > > +/* -- Misc objects -------------------------------------------------------- */ Minor: I suggest not Misc, but Profiler. > + > +struct lj_sysprof_topframe { > + uint8_t ffid; /* FFUNC: fast function id. */ Why this field can't be a part of union? Due to set vmstate we always know the necessary union's "subtype". > + union { > + uint64_t raw; /* Raw value for context save/restore. */ > + TValue *interp_base; /* LFUNC: Base of the executed coroutine. */ > + lua_CFunction cf; /* CFUNC: Address of the C function. */ Nit: please use tabs instead spaces for comments alignment, like it is done for other structures in this header. > + } guesttop; > +}; > + > /* -- State objects ------------------------------------------------------- */ > > /* VM states. */ > @@ -674,6 +685,7 @@ typedef struct global_State { > MRef jit_base; /* Current JIT code L->base or NULL. */ > MRef ctype_state; /* Pointer to C type state. */ > GCRef gcroot[GCROOT_MAX]; /* GC roots. */ > + struct lj_sysprof_topframe top_frame; /* Top frame for sysprof */ Nit: I suppose that this structure should be introduced only if sysprof is enabled. OTOH, I see no reason to hide this structure, so I suggest to drop it as is for now. Side note: Also, I'm not sure that this field has addressable offset for ARM architecture (for DynASM). Typo: s/for sysprof/for sysprof./ > } global_State; > > #define mainthread(g) (&gcref(g->mainthref)->th) > diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc > index 974047d3..c4beb5e7 100644 > --- a/src/vm_x64.dasc > +++ b/src/vm_x64.dasc > @@ -345,6 +345,35 @@ > | mov dword [DISPATCH+DISPATCH_GL(vmstate)], ~LJ_VMST_..st > |.endmacro > | > +|// Stash interpreter's internal base and enter LFUNC VM state. > +|// PROFILER: Each time profiler sees LFUNC state, it will inspect [BASE-1] I suppose, that this is not valid for GC64. LuaJIT uses 2-slot frame info here. The func value is in BASE - 2 slot (see for details). So this part should be adjusted. | (gdb) f 0 | #0 lj_cf_print (L=0x7ffff7c83378) at src/lib_base.c:486 | 486 ptrdiff_t i, nargs = L->top - L->base; | | (gdb) lj-arch | LJ_64: True, LJ_GC64: True, LJ_DUALNUM: False | (gdb) p gcval(L->base - 2)->fn.l.ffid | $11 = 29 '\035' # print | (gdb) p gcval(L->base - 1)->fn.l.ffid | $12 = 200 '\310' Minor: please use well-known special comments instead PROFILER. XXX [1] looks good here, IMO. Here and below. | Use XXX in a comment to flag something that is bogus but works. > +|// expecting to see a valid framelink there. So enter this state only when > +|// BASE is stable and slots are not moved on the stack. > +|.macro set_vmstate_lfunc > +| set_vmstate INTERP // Guard for non-atomic VM context restoration Nit: missed dot at he end of the sentence. Here and below. > +| mov dword [DISPATCH+DISPATCH_GL(top_frame.guesttop)], BASE > +| set_vmstate LFUNC > +|.endmacro > +| > +|// Stash ID of the fast function about to be executed and enter FFUNC VM state. > +|// PROFILER: Each time profiler sees FFUNC state, it will write ffid > +|// to the profile stream. > +|.macro set_vmstate_ffunc > +| set_vmstate INTERP // Guard for non-atomic VM context restoration > +| mov XCHGd, dword [BASE-8] XCHGd register is not defined in . | /home/burii/reviews/luajit/sysprof/src/vm_x64.dasc:501: error: bad operand mode in `mov i?,i?': | | mov XCHGd, L:RBa->base | ... Also, BASE stands for 64-bit register (*). | ... | /home/burii/reviews/luajit/sysprof/src/vm_x64.dasc:467: error: mixed operand size in `mov xd,rq': | | set_vmstate_cfunc | | mov dword [DISPATCH+DISPATCH_GL(top_frame.guesttop)], BASE [MACRO set_vmstate_cfunc (0)] > +| mov dword [DISPATCH+DISPATCH_GL(top_frame.ffid)], XCHGd Don't get it. Why is it `dword` instead of `byte`? > +| set_vmstate FFUNC > +|.endmacro > +| > +|// Stash address of the C function about to be executed and enter CFUNC VM state. Nit: Line width is more than 80 symbols. > +|// PROFILER: Each time profiler sees CFUNC state, it will write this address > +|// to the profile stream. > +|.macro set_vmstate_cfunc > +| set_vmstate INTERP // Guard for non-atomic VM context restoration > +| mov dword [DISPATCH+DISPATCH_GL(top_frame.guesttop)], BASE Ditto (*). > +| set_vmstate CFUNC > +|.endmacro > +| > |// Uses TMPRd (r10d). > |.macro save_vmstate > |.if not WIN > @@ -435,7 +464,7 @@ static void build_subroutines(BuildCtx *ctx) > | jnz ->vm_returnp > | > | // Return to C. > - | set_vmstate CFUNC > + | set_vmstate_cfunc > | and PC, -8 > | sub PC, BASE > | neg PC // Previous base = BASE - delta. > @@ -467,6 +496,9 @@ static void build_subroutines(BuildCtx *ctx) > | xor eax, eax // Ok return status for vm_pcall. > | > |->vm_leave_unw: > + | set_vmstate INTERP // Guard for non-atomic VM context restoration > + | mov XCHGd, L:RBa->base RBa register is not defined in . > + | mov dword [DISPATCH+DISPATCH_GL(top_frame.guesttop)], XCHGd > | // DISPATCH required to set properly. > | restore_vmstate // Caveat: uses TMPRd (r10d). > | restoreregs > @@ -725,7 +757,7 @@ static void build_subroutines(BuildCtx *ctx) > diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc > index ab8e6f27..222754fe 100644 > --- a/src/vm_x86.dasc > +++ b/src/vm_x86.dasc > @@ -443,6 +443,35 @@ > | mov dword [DISPATCH+DISPATCH_GL(vmstate)], ~LJ_VMST_..st > |.endmacro > | > +|// Stash interpreter's internal base and enter LFUNC VM state. > +|// PROFILER: Each time profiler sees LFUNC state, it will inspect [BASE-1] > +|// expecting to see a valid framelink there. So enter this state only when > +|// BASE is stable and slots are not moved on the stack. > +|.macro set_vmstate_lfunc > +| set_vmstate INTERP // Guard for non-atomic VM context restoration > +| mov dword [DISPATCH+DISPATCH_GL(top_frame.guesttop)], BASE > +| set_vmstate LFUNC > +|.endmacro > +| > +|// Stash ID of the fast function about to be executed and enter FFUNC VM state. > +|// PROFILER: Each time profiler sees FFUNC state, it will write ffid > +|// to the profile stream. > +|.macro set_vmstate_ffunc > +| set_vmstate INTERP // Guard for non-atomic VM context restoration > +| mov XCHGd, dword [BASE-8] This register is defined only for x64 architecture. The error is occured, when build with the following command: | $ make CC="gcc -m32" -f Makefile.original -j | ... | DYNASM host/buildvm_arch.h | vm_x86.dasc:632: error: bad operand mode in `mov i?,x?': | | mov XCHGd, L:RBa->base | vm_x86.dasc:1544: error: bad operand mode in `mov i?,xd': | |.ffunc_1 assert | | mov XCHGd, dword [BASE-8] [MACRO set_vmstate_ffunc (0)] | vm_x86.dasc:1572: error: bad operand mode in `mov i?,xd': | |.ffunc_1 type | | mov XCHGd, dword [BASE-8] [MACRO set_vmstate_ffunc (0)] | vm_x86.dasc:1599: error: bad operand mode in `mov i?,xd': | |.ffunc_1 getmetatable | | mov XCHGd, dword [BASE-8] [MACRO set_vmstate_ffunc (0)] Side note: Unfortunately it's impossible for now forcify x32 build via cmake. It requires to proxy CMAKE_C_FLAGS to macro in LuaJITUtils, IINM. > +| mov dword [DISPATCH+DISPATCH_GL(top_frame.ffid)], XCHGd Please clarify the following things: 1) IINM, we save not ffid but the GCref for this function (since we not load ffid field from function). 2) Why do we store 64 bytes instead 8? Yes, the struct is not packed and there is a hole in it, so it works correct. But it is a little bit confusing. Also, why can't we write BASE here too and inspect ffid from this base later? > +| set_vmstate FFUNC > +|.endmacro > +| > +|// Stash address of the C function about to be executed and enter CFUNC VM state. > +|// PROFILER: Each time profiler sees CFUNC state, it will write this address > +|// to the profile stream. > +|.macro set_vmstate_cfunc > +| set_vmstate INTERP // Guard for non-atomic VM context restoration > +| mov dword [DISPATCH+DISPATCH_GL(top_frame.guesttop)], BASE > +| set_vmstate CFUNC > +|.endmacro > +| > |// Uses spilled ecx on x86 or XCHGd (r11d) on x64. > |.macro save_vmstate > |.if not WIN > @@ -560,7 +589,7 @@ static void build_subroutines(BuildCtx *ctx) > @@ -599,6 +628,9 @@ static void build_subroutines(BuildCtx *ctx) > | xor eax, eax // Ok return status for vm_pcall. > | > |->vm_leave_unw: > + | set_vmstate INTERP // Guard for non-atomic VM context restoration > + | mov XCHGd, L:RBa->base AFAICS, there is no garantee, that L:RBa is set up to `lua_State *`. For example, during `vm_unwind_c_eh` RB register is set to `global_State *`. |->vm_unwind_c_eh: // Landing pad for external unwinder. | mov L:DISPATCH, SAVE_L | mov GL:RB, L:DISPATCH->glref | mov dword GL:RB->cur_L, L:DISPATCH | mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC | mov DISPATCH, L:DISPATCH->glref // Setup pointer to dispatch table. | add DISPATCH, GG_G2DISP | jmp ->vm_leave_unw > + | mov dword [DISPATCH+DISPATCH_GL(top_frame.guesttop)], XCHGd > | // DISPATCH required to set properly. > | restore_vmstate // Caveat: on x64 uses XCHGd (r11d). > | restoreregs > @@ -934,7 +966,7 @@ static void build_subroutines(BuildCtx *ctx) > -- > 2.35.1 > [1]: https://www.oracle.com/java/technologies/javase/codeconventions-programmingpractices.html -- Best regards, Sergey Kaplun