From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 77BB34765E0 for ; Wed, 30 Dec 2020 13:50:59 +0300 (MSK) From: Sergey Ostanevich Message-Id: <2B46045C-1624-431A-AC2A-0BA583A45990@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_9E5F5A1F-3A81-483E-846A-924878E60502" Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Date: Wed, 30 Dec 2020 13:50:57 +0300 In-Reply-To: <20201230095031.GR14702@root> References: <20201230093916.GX5396@tarantool.org> <20201230095031.GR14702@root> Subject: Re: [Tarantool-patches] [PATCH luajit 3/3] core: remove excess assertion inside memprof List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org --Apple-Mail=_9E5F5A1F-3A81-483E-846A-924878E60502 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Hi! Thanks for the patch! Just one nit in comments. LGTM. Sergos >=20 > Reworded. See the iterative patch below. Branch is force-pushed. >=20 > | core: remove excess assertion inside memprof > | > | There are the cases when the memory profiler attempts to attribute ^^^ remove, just 'there are cases' =09 > | allocations triggered by JIT engine recording phase with a Lua = function > | to be recorded. At this case lj_debug_frameline() may return = BC_NOPOS > | (i.e. a negative value) so the assertion in the Lua writer > | memprof_write_lfunc() is violated. > | > | This patch removes this assertion. For negative returned line value > | profiler is reported zero frameline. > | > | Follows up tarantool/tarantool#5442 >=20 >>=20 >>> ** -DLUAJIT_DISABLE_DEBUGINFO flag. >>> */ >>> - lua_assert(line >=3D 0); >>> - lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC); >>> - lj_wbuf_addu64(out, (uintptr_t)funcproto(fn)); >>> - lj_wbuf_addu64(out, (uint64_t)line); >>> + lj_wbuf_addu64(out, line >=3D 0 ? (uint64_t)line : 0); >>> } >>>=20 >>> static void memprof_write_cfunc(struct lj_wbuf *out, uint8_t aevent, >>> --=20 >>> 2.28.0 >>>=20 >>=20 >> --=20 >> Best regards, >> IM >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > diff --git a/src/lj_memprof.c b/src/lj_memprof.c > index 0568049..37ec4c9 100644 > --- a/src/lj_memprof.c > +++ b/src/lj_memprof.c > @@ -94,7 +94,10 @@ static void memprof_write_lfunc(struct lj_wbuf = *out, uint8_t aevent, > lj_wbuf_addu64(out, (uintptr_t)funcproto(fn)); > /* > ** Line is >=3D 0 if we are inside a Lua function. > - ** An exception may be when the Lua function is on top. > + ** There are the cases when the memory profiler attempts > + ** to attribute allocations triggered by JIT engine recording > + ** phase with a Lua function to be recorded. At this case > + ** lj_debug_frameline() may return BC_NOPOS (i.e. a negative = value). > ** Equals to zero when LuaJIT is built with the > ** -DLUAJIT_DISABLE_DEBUGINFO flag. > */ > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 > --=20 > Best regards, > Sergey Kaplun --Apple-Mail=_9E5F5A1F-3A81-483E-846A-924878E60502 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii Hi!

Thanks = for the patch!
Just one nit in comments.

LGTM.
Sergos


Reworded. See = the iterative patch below. Branch is force-pushed.

| core: = remove excess assertion inside memprof
|
| There are = the cases when the memory profiler attempts to attribute
      ^^^ remove, = just 'there are cases'

| allocations triggered by JIT engine recording phase with a = Lua function
| to be = recorded. At this case lj_debug_frameline() may return = BC_NOPOS
| (i.e. a = negative value) so the assertion in the Lua writer
| = memprof_write_lfunc() is violated.
|
| This patch = removes this assertion. For negative returned line value
| profiler is = reported zero frameline.
|
| Follows up = tarantool/tarantool#5442


  ** -DLUAJIT_DISABLE_DEBUGINFO = flag.
  */
-  lua_assert(line = >=3D 0);
-  lj_wbuf_addbyte(out, aevent | = ASOURCE_LFUNC);
-  lj_wbuf_addu64(out, = (uintptr_t)funcproto(fn));
-  lj_wbuf_addu64(out, = (uint64_t)line);
+  lj_wbuf_addu64(out, line >=3D = 0 ? (uint64_t)line : 0);
}

static void memprof_write_cfunc(struct lj_wbuf *out, uint8_t = aevent,
-- 
2.28.0


-- 
Best = regards,
IM

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
diff --git = a/src/lj_memprof.c b/src/lj_memprof.c
index 0568049..37ec4c9 100644
--- a/src/lj_memprof.c
+++ b/src/lj_memprof.c
@@ -94,7 +94,10 @@ static void memprof_write_lfunc(struct = lj_wbuf *out, uint8_t aevent,
  lj_wbuf_addu64(out, = (uintptr_t)funcproto(fn));
  /*
  ** Line is >=3D 0 if we are inside a Lua = function.
-  ** An = exception may be when the Lua function is on top.
+  ** = There are the cases when the memory profiler attempts
+  ** to = attribute allocations triggered by JIT engine recording
+  ** = phase with a Lua function to be recorded. At this case
+  ** = lj_debug_frameline() may return BC_NOPOS (i.e. a negative = value).
  ** = Equals to zero when LuaJIT is built with the
  ** = -DLUAJIT_DISABLE_DEBUGINFO flag.
  */
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

-- 
Best = regards,
Sergey = Kaplun

= --Apple-Mail=_9E5F5A1F-3A81-483E-846A-924878E60502--