[Tarantool-patches] [PATCH luajit v1 04/11] profile: introduce symtab write module
Igor Munkin
imun at tarantool.org
Mon Dec 21 13:30:52 MSK 2020
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
More information about the Tarantool-patches
mailing list