Re: [Tarantool-patches] [PATCH luajit v1 04/11] profile: introduce symtab write module

[Igor Munkin <imun@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 <ljp_symtab.h> 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 <lj_wbuf_symtab.*> considering the naming
in the third patch.

> @@ -469,7 +469,7 @@ DASM_FLAGS= $(DASM_XFLAGS) $(DASM_AFLAGS)

<snipped>

> 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 @@

<snipped>

> +#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 <write_buffer>
function in <ljp_write.c> public?

> +{

<snipped>

> +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) {

<snipped>

> +    case (~LJ_TTRACE): {
> +      /* TODO: Implement dumping a trace info */
> +      break;
> +    }

Minor: So this case be dropped for now?

> +    default: {

<snipped>

> 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 @@

<snipped>

> +struct global_State;
> +struct ljp_buffer;

Why do not simply include <lj_obj.h> and <profile/ljp_write.h>?
Otherwise, just move it close to the function signature and mention the
reason why you omit these headers inclusion.

> +

<snipped>

> +
> +/* 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. <lj_wbuf_addsymtab>.

> +void ljp_symtab_write(struct ljp_buffer *out, const struct global_State *g);
> +
> +#endif
> -- 
> 2.28.0
> 

-- 
Best regards,
IM