Re: [Tarantool-patches] [PATCH luajit] codehealth: fix sign extension in sysprof counters

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

Hi, Maxim!
Thanks for the patch!
Please, consider my comments below.

On 14.03.23, Maksim Kokryashkin wrote:
> The `setnumfield` function takes `int64_t` as an argument.

I see, that this function is also used for the `getmetrics` API to fill
the similar structure. It uses size_t type, instead of uint64_t. Should
we obscure the similar issue for this type?

If no, maybe it's better to use size_t here instead to emphasize that
these values are unsigned?

If yes, we should fix both places.

> Profile counters used to have the `uint64_t` type before
> this patch, which could lead to issues with sign extension.

Can you provide some examples of such issues?
If it is about type overflow, that is kinda highly unlikely (same about
getmetrics fields even for strhash_{hit,miss} and gc_{allocated,freed}):

| perl -E 'say 0x7fffffffffffffff / 1000 / 60 / 60 /24 / 366'
| 291672107.014483

And the value still "can overflow" but just become negative instead.

> Now their type is `int64_t`, so they correspond to the
> `setnumfield` argument type.
> ---
> PR: https://github.com/tarantool/tarantool/pull/8440
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/svace-sysprof-report
>  src/lj_sysprof.c |  2 +-
>  src/lmisclib.h   | 22 +++++++++++-----------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
> index 2e9ed9b3..4723e6f9 100644
> --- a/src/lj_sysprof.c
> +++ b/src/lj_sysprof.c
> @@ -285,7 +285,7 @@ static void sysprof_record_sample(struct sysprof *sp, siginfo_t *info)
>    lua_assert(pthread_self() == sp->thread);
>  
>    /* Caveat: order of counters must match vmstate order in <lj_obj.h>. */
> -  ((uint64_t *)&sp->counters)[vmstate]++;
> +  ((int64_t *)&sp->counters)[vmstate]++;
>  
>    sp->counters.samples++;
>  
> diff --git a/src/lmisclib.h b/src/lmisclib.h
> index 9084319c..599a6ea9 100644
> --- a/src/lmisclib.h
> +++ b/src/lmisclib.h
> @@ -105,21 +105,21 @@ typedef void (*luam_Sysprof_backtracer)(void *(*frame_writer)(int frame_no, void
>  #define LUAM_SYSPROF_CALLGRAPH 2
>  
>  struct luam_Sysprof_Counters {
> -  uint64_t vmst_interp;
> -  uint64_t vmst_lfunc;
> -  uint64_t vmst_ffunc;
> -  uint64_t vmst_cfunc;
> -  uint64_t vmst_gc;
> -  uint64_t vmst_exit;
> -  uint64_t vmst_record;
> -  uint64_t vmst_opt;
> -  uint64_t vmst_asm;
> -  uint64_t vmst_trace;
> +  int64_t vmst_interp;
> +  int64_t vmst_lfunc;
> +  int64_t vmst_ffunc;
> +  int64_t vmst_cfunc;
> +  int64_t vmst_gc;
> +  int64_t vmst_exit;
> +  int64_t vmst_record;
> +  int64_t vmst_opt;
> +  int64_t vmst_asm;
> +  int64_t vmst_trace;
>    /*
>    ** XXX: Order of vmst counters is important: it should be the same as the
>    ** order of the vmstates.
>    */
> -  uint64_t samples;
> +  int64_t samples;
>  };
>  
>  /* Profiler options. */
> -- 
> 2.37.1 (Apple Git-137.1)
> 

-- 
Best regards,
Sergey Kaplun