Re: [Tarantool-patches] [PATCH luajit v2 2/3] memprof: extend symtab with information about .so libs

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

| memprof: extend symtab with information about .so libs

Width limit for commit title is 50 symbols including subsystem name [1].
Commit message should fit 72 symbols line width [1].

Also, you noticed [2], that just dump all symbols from so libraries shown
better performance than the current solution. So looks like we want to
reimplement this patch in this way, don't we?

So, for now I stop reviewing (this patch is the last one) until we don't
make the decision about implementation.

On 20.08.21, Maxim Kokryashkin wrote:
> This commit enriches memprof's symbol table with information about loaded
> ".so" files. That information will provide demangling capabilities to
> the parser.
> 
> The following data is stored for each ".so" library:
> | SYMTAB_SO | path to .so | it's address |  CRC32 Hash  |
>   1 byte                       8 bytes       8 bytes
>   magic
>   number
> 
> C symbol address from profile events with an address of the ".so" file will
> give us an offset, and it can be used to get the symbol in that file.
> 
> CRC32 hash will be used by the parser to determine whether the shared
> object that was used during the process profiling and during the process
> of parsing is the same.
> 
> Part of tarantool/tarantool#5813
> ---
>  src/lj_memprof.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/lj_memprof.h |  3 ++-
>  2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
> index 2c1ef3b8..c8367c95 100644
> --- a/src/lj_memprof.c
> +++ b/src/lj_memprof.c
> @@ -5,10 +5,15 @@
>  ** Copyright (C) 2015-2019 IPONWEB Ltd.
>  */
>  
> +#define _GNU_SOURCE

Should it be defind exactly behind <link.h>?
And even undefined after?

> +
>  #define lj_memprof_c
>  #define LUA_CORE
>  
>  #include <errno.h>
> +#include <stdio.h>
> +#include <stdlib.h>

Typo: this include looks redundant.

> +#include <link.h>
>  
>  #include "lj_arch.h"
>  #include "lj_memprof.h"
> @@ -18,12 +23,64 @@
>  #include "lj_obj.h"
>  #include "lj_frame.h"
>  #include "lj_debug.h"
> +#include "lj_utils.h"
>  
>  /* --------------------------------- Symtab --------------------------------- */
>  
>  static const unsigned char ljs_header[] = {'l', 'j', 's', LJS_CURRENT_VERSION,
>  					   0x0, 0x0, 0x0};
>  
> +int lj_file_crc32(const char* name, uint32_t* result) 
                                                        ^
Trailing whitespace here -------------------------------^

> +{
> +#define CRC_BUF_SIZE 1024 * 1024

Minor: It is better to use parentheses, when you define not one-token
marco.
Rationale: ~CRC_BUF_SIZE and ~(CRC_BUF_SIZE) return different values.
(Of course, this example is a little bit crazy, but it shows the point).

Also, why do we need 1 MB on the stack?
I suppose that 512 bytes will be enough.

> +
> +  char buf[CRC_BUF_SIZE] = "";

Don't get this line. Why do we need this initialization?

> +  size_t read_bytes = 0;

This too.

> +  FILE* so = NULL;

Typo: please use FILE *so instead FILE* so, considering our code
style.

> +
> +  if (result == NULL) {
> +    return 1;
> +  }
> +
> +  *result = 0xffffffff;

Please avoid the magic values in code.
Also, I don't see any check for this value latter, so it looks
excess.

> +
> +  so = fopen(name, "rb");
> +  if (!so)
> +    return 1;
> +
> +  while (CRC_BUF_SIZE == (read_bytes = fread(buf, sizeof(*buf), CRC_BUF_SIZE, so))) {

The brackets looks excess.

Side note: why do you use "reversed order" i.e. constant == some_call()?

> +    *result = lj_utils_crc32(buf, CRC_BUF_SIZE, *result); 
                                                            ^
Trailing whitespace here -----------------------------------^

> +  }
> +  
   ^^
Trailing whitespaces here.

> +  if(ferror(so) || !feof(so)) {
> +    fclose(so);
> +    return 1;
> +  }
> +  
   ^^
Trailing white spaces here.

Side note: Please adjust your favorite editor to highlight trailing
whytespaces.

> +  *result = lj_utils_crc32(buf, read_bytes, *result);
> +  fclose(so);
> +  return 0;
> +}
> +
> +int write_shared_obj(struct dl_phdr_info *info, size_t size, void *data)

I got the following warning during compilation:

| /home/burii/reviews/luajit/csymbols/src/lj_memprof.c: In function 'write_shared_obj':
| /home/burii/reviews/luajit/csymbols/src/lj_memprof.c:64:56: warning: unused parameter 'size' [-Wunused-parameter]
|    64 | int write_shared_obj(struct dl_phdr_info *info, size_t size, void *data)

Please, used the corresponding UNUSED() macro.

> +{
> +  uint32_t so_hash = 0;
> +  struct lj_wbuf *buf = data;
> +
> +  if (info->dlpi_name && strlen(info->dlpi_name)) {
> +    if (0 != lj_file_crc32(info->dlpi_name, &so_hash))
> +      // XXX: Maybe it is reasonable to print warning here

I don't think so. This warning should be printed by the symtab parser.
So we need to write this event, but with special hash value means "failed
to calculate hash". OTOH, I don't know what we can do with hash collision.

> +      return 0;
> +
> +    lj_wbuf_addbyte(buf, SYMTAB_SO);
> +    lj_wbuf_addstring(buf, info->dlpi_name);
> +    lj_wbuf_addu64(buf, info->dlpi_addr);
> +    lj_wbuf_addu64(buf, so_hash);
> +  }
> +
> +  return 0;
> +}
> +
>  static void dump_symtab(struct lj_wbuf *out, const struct global_State *g)
>  {
>    const GCRef *iter = &g->gc.root;
> @@ -49,6 +106,9 @@ static void dump_symtab(struct lj_wbuf *out, const struct global_State *g)
>      iter = &o->gch.nextgc;
>    }
>  
> +  /* Write shared libraries. */
> +  dl_iterate_phdr(write_shared_obj, out);
> +
>    lj_wbuf_addbyte(out, SYMTAB_FINAL);
>  }
>  
> diff --git a/src/lj_memprof.h b/src/lj_memprof.h
> index 3417475d..0cefc403 100644
> --- a/src/lj_memprof.h
> +++ b/src/lj_memprof.h
> @@ -16,7 +16,7 @@
>  #include "lj_def.h"
>  #include "lj_wbuf.h"
>  
> -#define LJS_CURRENT_VERSION 0x1
> +#define LJS_CURRENT_VERSION 0x2

It means that we need to upgrade parser with the same commit.
Rationale: without this test will fail -- it complicates debugging with
bisecting later (for now, tests fail on this commit).

>  
>  /*
>  ** symtab format:
> @@ -51,6 +51,7 @@
>  */
>  
>  #define SYMTAB_LFUNC ((uint8_t)0)
> +#define SYMTAB_SO    ((uint8_t)1)

Please, add entry for the corresponging events in symtab description.

>  #define SYMTAB_FINAL ((uint8_t)0x80)
>  
>  #define LJM_CURRENT_FORMAT_VERSION 0x01
> -- 
> 2.32.0
> 

Also, please add some tests to check the behaviour of your patch.

[1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025707.html

-- 
Best regards,
Sergey Kaplun