From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 1F8A04765E0 for ; Mon, 21 Dec 2020 13:30:58 +0300 (MSK) Date: Mon, 21 Dec 2020 13:30:52 +0300 From: Igor Munkin Message-ID: <20201221103052.GU5396@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH luajit v1 04/11] profile: introduce symtab write module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Sergey, Thanks for the patch! Please consider minor comments below. On 16.12.20, Sergey Kaplun wrote: > This patch adds profile writer that writes all necessary Lua > functions prototypes info like GCproto address, name of the chunk this > function was defined in and number of the first line of it. > See for details. > > Usage of this module will be added at the next patches. I propose the following wording to make it a bit clearer: | core: introduce symtab dumping module | | This patch introduces the routine dumping the definitions of all | loaded Lua functions via the write buffer introduced in the previous | patch. The following information is recorded for each function: | * GCproto address | * The name of the Lua chunk where this function is defined | * The line number where this function is defined (i.e. the line where | its signature locates) > > Part of tarantool/tarantool#5442 > --- > src/Makefile | 2 +- > src/Makefile.dep | 2 ++ > src/profile/ljp_symtab.c | 55 ++++++++++++++++++++++++++++++++++++++ > src/profile/ljp_symtab.h | 57 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 115 insertions(+), 1 deletion(-) > create mode 100644 src/profile/ljp_symtab.c > create mode 100644 src/profile/ljp_symtab.h > > diff --git a/src/Makefile b/src/Makefile > index 4b1d937..e00265c 100644 > --- a/src/Makefile > +++ b/src/Makefile Please, adjust these changes considering the comments to the first patch. I propose to name this considering the naming in the third patch. > @@ -469,7 +469,7 @@ DASM_FLAGS= $(DASM_XFLAGS) $(DASM_AFLAGS) > diff --git a/src/profile/ljp_symtab.c b/src/profile/ljp_symtab.c > new file mode 100644 > index 0000000..5a17c97 > --- /dev/null > +++ b/src/profile/ljp_symtab.c > @@ -0,0 +1,55 @@ > +#define LJS_CURRENT_VERSION 2 Why is it already the second version? > + > +static const unsigned char ljs_header[] = {'l', 'j', 's', LJS_CURRENT_VERSION, > + 0x0, 0x0, 0x0}; > + > +static void symtab_write_prologue(struct ljp_buffer *out) Why do you need a separate routine instead of making function in public? > +{ > +void ljp_symtab_write(struct ljp_buffer *out, const struct global_State *g) > +{ > + const GCobj *o; > + const GCRef *iter = &g->gc.root; > + > + symtab_write_prologue(out); > + > + while (NULL != (o = gcref(*iter))) { > + switch (o->gch.gct) { > + case (~LJ_TTRACE): { > + /* TODO: Implement dumping a trace info */ > + break; > + } Minor: So this case be dropped for now? > + default: { > diff --git a/src/profile/ljp_symtab.h b/src/profile/ljp_symtab.h > new file mode 100644 > index 0000000..3a40d98 > --- /dev/null > +++ b/src/profile/ljp_symtab.h > @@ -0,0 +1,57 @@ > +struct global_State; > +struct ljp_buffer; Why do not simply include and ? Otherwise, just move it close to the function signature and mention the reason why you omit these headers inclusion. > + > + > +/* Writes the symbol table of the VM g to out. */ Strictly saying this routine dumps the symbol table to *ljp_buffer* and I guess it should be mentioned in its name e.g. . > +void ljp_symtab_write(struct ljp_buffer *out, const struct global_State *g); > + > +#endif > -- > 2.28.0 > -- Best regards, IM