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 3F66AA716E9; Thu, 18 Apr 2024 01:42:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3F66AA716E9 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1713393778; bh=68XNPAGLuay5HySYZS+FKSA+6wY1KPuW7yBaMzHq4Gs=; 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=fumXG2fdU6dA4R2tO/s8xIMNvRv0tXlxBlB8SXfZKctBkOv4VW43qsPNv93owwhpw Ck+IvEuuRGHlts+sTniYjLeeywyoexX/67yswp4r2i6ZNYi6f6QWOwtuw7l/YL2nyY HX91gkJHBWLiqhjA04nl1IxlNDn0exBUmSx6JRCk= Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [95.163.41.96]) (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 37632A716EB for ; Thu, 18 Apr 2024 01:42:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 37632A716EB Received: by smtp58.i.mail.ru with esmtpa (envelope-from ) id 1rxDzT-0000000B82X-47BZ; Thu, 18 Apr 2024 01:42:56 +0300 Date: Thu, 18 Apr 2024 01:42:54 +0300 To: Sergey Kaplun Message-ID: References: <13501abba00ac3e072284d36a531c721e279722f.1712182830.git.m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9F97E3C14763C38E29909D6ED2E0D50F8D9C3AEED85640F0C182A05F53808504057EDB9A82AD4852A33594132A326AF8BA0D2BCC056B6EA49813D46A38C6CBFB44AADF9F6CE649D98 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75DF2B1F23425CAE5EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379703D7D5B138802D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8EA701AA4203372A6CF5C3E321C73CDD4DBDB26FE1ED39FFACC7F00164DA146DAFE8445B8C89999728AA50765F790063741F7343E26298569389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC80839144E5BB460BAF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C4CB6874B0BCFF0B8C0837EA9F3D197644AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3048DACD9C49003B6BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFED8438A78DFE0A9E1DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C341E1C46B3B04646035872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A55776B339BF17EFD95002B1117B3ED6961DA4B989419C2DEE5D145BB8EF0DE66B823CB91A9FED034534781492E4B8EEADEEA082C9A12FE455BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFA650554EF8C1E95B63AED70B726A045FE0714AF8CC224A62B4DBC0BCF4040B9646C06E59B4E814054AA6A4DA8F6324161D5A5F251E15592404E84578F371E6865B7DFAB018B1257FC226CC413062362A913E6812662D5F2A54F6898A6FDCBDC72A617DFBE5FEC2C6383653B6C8D9AE0FD16FCAA6493B703A X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojKLjB1Eikxbox870fBLg05Q== X-Mailru-Sender: 7940E2A4EB16C9972AA1F0BE8738C4AEBBCCA045EF0590167F8A00840ADCF450FC7F7197EB40C79FB6F7D78CE1F58EAD61AD1BC23DFB21333DDE9B364B0DF289BB83A8C3DAEBA78A61AAEF30F77CACB9EAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v6 1/2] debug: generalized extension 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: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: Maxim Kokryashkin , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the review! See my answers below. On Wed, Apr 17, 2024 at 07:00:00PM +0300, Sergey Kaplun wrote: > Hi, Maxim! > Thanks for the patch! > Please consider my comments below. > > On 04.04.24, Maxim Kokryashkin wrote: > > > > > --- > > src/luajit-gdb.py | 885 -------------------------- > > src/{luajit_lldb.py => luajit_dbg.py} | 616 ++++++++++++------ > > Since luajit_lldb.py is gone, please change the comment in <.flake8rc>. Fixed. > > > 2 files changed, 416 insertions(+), 1085 deletions(-) > > delete mode 100644 src/luajit-gdb.py > > rename src/{luajit_lldb.py => luajit_dbg.py} (63%) > > > > diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py > > deleted file mode 100644 > > > > > diff --git a/src/luajit_lldb.py b/src/luajit_dbg.py > > similarity index 63% > > rename from src/luajit_lldb.py > > rename to src/luajit_dbg.py > > index 5ac11b65..a42d8f25 100644 > > --- a/src/luajit_lldb.py > > +++ b/src/luajit_dbg.py > > @@ -1,10 +1,230 @@ > > -# LLDB extension for LuaJIT post-mortem analysis. > > -# To use, just put 'command script import /src/luajit_lldb.py' > > -# in lldb. > > +# Debug extension for LuaJIT post-mortem analysis. > > +# To use in LLDB: 'command script import /src/luajit_dbg.py' > > +# To use in GDB: 'source /src/luajit_dbg.py' > > > > import abc > > import re > > -import lldb > > +import sys > > +import types > > + > > +from importlib import import_module > > + > > +# make script compatible with the ancient Python {{{ > > Typo: s/make script/Make the script/ > > Fixed. > > > +class Debugger(object): > > + def __init__(self): > > + self.GDB = False > > + self.LLDB = False > > + > > + debuggers = { > > + 'gdb': lambda lib: True, > > + 'lldb': lambda lib: lib.debugger is not None, > > + } > > + for name, healthcheck in debuggers.items(): > > + lib = None > > + try: > > + lib = import_module(name) > > + if healthcheck(lib): > > Why do we need this healthcheck? Why just import of the module isn't > enough? > Please add a comment near `debuggers` definition. Added. > > > + setattr(self, name.upper(), True) > > + globals()[name] = lib > > + self.name = name > > + except Exception: > > + continue > > + > > + assert self.LLDB != self.GDB > > > > > + > > + def find_type(self, typename): > > + if self.LLDB: > > + return target.FindFirstType(typename) > > + else: > > + return gdb.lookup_type(typename) > > Why do you drop the cache for types here? It may be critical when > running scripts for the search of objects on big coredumps or the > attached process. Well, I believe I just forgot about that. Added. Side note: if only we could do the python3-only version of that, the fix would just be a single decorator in front of the function definition... > > + > > + def type_to_pointer_type(self, tp): > > + if self.LLDB: > > + return tp.GetPointerType() > > + else: > > + return tp.pointer() > > > > > + > > + def type_member_offset(self, member): > > + if self.LLDB: > > + return member.GetOffsetInBytes() > > + else: > > + return member.bitpos / 8 > > Should it be `//`? Doesn't really matter, but ok. Fixed. > > > > > +class Struct(object): > > Should we do this part for GDB too? I thought that this class generation > may be skipped for GDB. No, the whole idea is to encapsulate the debugger-specific things into the debugger object and these classes. So we cannot skip them, they have crucial role in this adapter. > > > def __init__(self, value): > > self.value = value > > > > def __getitem__(self, name): > > > > > +def make_property_from_metadata(field, tp): > > + builtin = { > > + 'uint': dbg.to_unsigned, > > + 'int': dbg.to_signed, > > + 'string': dbg.to_str, > > + } > > + if tp in builtin.keys(): > > + return lambda self: builtin[tp](self[field]) > > + else: > > + return lambda self: globals()[tp](self[field]) > > + > > + > > +for cls, metainfo in c_structs.items(): > > + cls_dict = {} > > + for field in metainfo: > > May you please name field[0], field[1] as local variables for better > readability? Fixed. > > > + if not isinstance(field[0], str): > > + cls_dict[field[1]] = field[0] > > + else: > > + cls_dict[field[1]] = property( > > + make_property_from_metadata(field[1], field[0]) > > + ) > > + globals()[cls] = type(cls, (Struct, ), cls_dict) > > > > > > for cls in Struct.__subclasses__(): > > ptr_name = cls.__name__ + 'Ptr' > > > > > > > > - > > - ret = frame.EvaluateExpression(command) > > - return ret > > + return dbg.to_unsigned(dbg.eval(command)) > > Why do we need return unsigned here? Because all of our commands accept either pointers, or numbers as argumnents and lldb's eval may return a string instead. > > > > > @abc.abstractproperty > > def command(self): > > @@ -270,7 +491,7 @@ class Command(object): > > > > > @@ -278,6 +499,11 @@ class Command(object): > > properly routed to LLDB frontend. Any unhandled exception will be > > automatically transformed into proper errors. > > """ > > + def invoke(self, arg, from_tty): > > + try: > > + self.execute(arg) > > + except Exception as e: > > + dbg.write(e) > > Why do we need this change? > > The error message for such situation is changed and non informative: > > | Breakpoint 1, lj_cf_dofile (L=0x2) at /home/burii/reviews/luajit/lj-dbg/src/lib_base.c:429 > | 429 { > | (gdb) lj-stack L > | Python Exception : unsupported operand type(s) for +: 'MemoryError' and 'str' > | Error occurred in Python: unsupported operand type(s) for +: 'MemoryError' and 'str' > > Within the following implementation all works as expected. > | def invoke(self, arg, from_tty): > | self.execute(arg) Fixed. > > This produces more understandable reason of an error: > | (gdb) lj-stack L > | Python Exception : Cannot access memory at address 0x26 > | Error occurred in Python: Cannot access memory at address 0x26 > Without the explicit exception output LLDB command just fails silently. Choosing from a less readable error and not understanding what happened (and whether it even happened at all) I would personally prefer the first option. > Also, maybe it is good to add a test for this error. Kind of a strange idea to test whether the extension throws an unhandled exception. It's like checking if Tarantool gives a segfault with a certain backtrace after a NULL dereference. > > > -- > Best regards, > Sergey Kaplun