Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] codehealth: fix sign extension in sysprof counters
@ 2023-03-14 15:25 Maksim Kokryashkin via Tarantool-patches
  2023-03-15  6:48 ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 2+ messages in thread
From: Maksim Kokryashkin via Tarantool-patches @ 2023-03-14 15:25 UTC (permalink / raw)
  To: tarantool-patches, sergos, skaplun, m.kokryashkin; +Cc: Maksim Kokryashkin

The `setnumfield` function takes `int64_t` as an argument.
Profile counters used to have the `uint64_t` type before
this patch, which could lead to issues with sign extension.
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)


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] codehealth: fix sign extension in sysprof counters
  2023-03-14 15:25 [Tarantool-patches] [PATCH luajit] codehealth: fix sign extension in sysprof counters Maksim Kokryashkin via Tarantool-patches
@ 2023-03-15  6:48 ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 2+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-03-15  6:48 UTC (permalink / raw)
  To: Maksim Kokryashkin; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2023-03-15  6:52 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-14 15:25 [Tarantool-patches] [PATCH luajit] codehealth: fix sign extension in sysprof counters Maksim Kokryashkin via Tarantool-patches
2023-03-15  6:48 ` Sergey Kaplun via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox