[Tarantool-patches] [PATCH] sysprof: change C configuration API

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

This patch replaces the `lj_sysprof_configure` with individual
configuration handles for each config entry. Also, it chnages the
internal sysprof checks to let users run sysprof in default mode without
configuratons.

Part of tarantool/tarantool#781
---
Branch: https://github.com/tarantool/luajit/tree/fckxorg/sysprof-rc-full-ci
PR with Tarantool integration:
https://github.com/tarantool/tarantool/pull/7123

 src/lib_misc.c                                | 20 +---
 src/lj_mapi.c                                 | 13 ++-
 src/lj_sysprof.c                              | 91 +++++++++++++------
 src/lj_sysprof.h                              |  6 +-
 src/lmisclib.h                                | 54 +++++------
 .../misclib-sysprof-capi/testsysprof.c        | 17 +---
 .../misclib-sysprof-lapi.test.lua             |  2 +-
 7 files changed, 116 insertions(+), 87 deletions(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index e07f1b32..370307a7 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -288,23 +288,10 @@ LJLIB_CF(misc_sysprof_start)
   int status = PROFILE_SUCCESS;
 
   struct luam_Sysprof_Options opt = {};
-  struct luam_Sysprof_Config conf = {};
-
 
   status = parse_options(L, &opt);
-  if (LJ_UNLIKELY(status != PROFILE_SUCCESS)) {
-    return sysprof_error(L, status);
-  }
-
-  conf.writer = buffer_writer_default;
-  conf.on_stop = on_stop_cb_default;
-  conf.backtracer = NULL;
-
-  status = luaM_sysprof_configure(&conf);
-  if (LJ_UNLIKELY(status != PROFILE_SUCCESS)) {
-    on_stop_cb_default(opt.ctx, opt.buf);
+  if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
     return sysprof_error(L, status);
-  }
 
   status = luaM_sysprof_start(L, &opt);
   if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
@@ -454,9 +441,12 @@ LJLIB_CF(misc_memprof_stop)
 
 LUALIB_API int luaopen_misc(struct lua_State *L)
 {
+  luaM_sysprof_set_writer(buffer_writer_default);
+  luaM_sysprof_set_on_stop(on_stop_cb_default);
+  luaM_sysprof_set_backtracer(NULL);
+
   LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc);
   LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof);
   LJ_LIB_REG(L, LUAM_MISCLIBNAME ".sysprof", misc_sysprof);
-
   return 1;
 }
diff --git a/src/lj_mapi.c b/src/lj_mapi.c
index 73b5dc74..b327d2ca 100644
--- a/src/lj_mapi.c
+++ b/src/lj_mapi.c
@@ -67,10 +67,19 @@ LUAMISC_API void luaM_metrics(lua_State *L, struct luam_Metrics *metrics)
 }
 
 /* --- Platform and Lua profiler ------------------------------------------ */
+LUAMISC_API int luaM_sysprof_set_writer(sp_writer writer)
+{
+  return lj_sysprof_set_writer(writer);
+}
+
+LUAMISC_API int luaM_sysprof_set_on_stop(sp_on_stop on_stop)
+{
+  return lj_sysprof_set_on_stop(on_stop);
+}
 
-LUAMISC_API int luaM_sysprof_configure(const struct luam_Sysprof_Config *config)
+LUAMISC_API int luaM_sysprof_set_backtracer(sp_backtracer backtracer)
 {
-  return lj_sysprof_configure(config);
+  return lj_sysprof_set_backtracer(backtracer);
 }
 
 LUAMISC_API int luaM_sysprof_start(lua_State *L,
diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index 23947315..3088036f 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -54,8 +54,6 @@
 #define vmstfit4(st) ((st & ~(uint32_t)((1 << 4) - 1)) == 0)
 
 enum sysprof_state {
-  /* Profiler needs to be configured. */
-  SPS_UNCONFIGURED,
   /* Profiler is not running. */
   SPS_IDLE,
   /* Profiler is running. */
@@ -74,7 +72,9 @@ struct sysprof {
   struct lj_wbuf out; /* Output accumulator. */
   struct luam_Sysprof_Counters counters; /* Profiling counters. */
   struct luam_Sysprof_Options opt; /* Profiling options. */
-  struct luam_Sysprof_Config config; /* Profiler configurations. */
+  sp_writer writer; /* Writer function for profile events. */
+  sp_on_stop on_stop; /* Callback on profiling stopping. */
+  sp_backtracer backtracer; /* Backtracing function for the host stack. */
   lj_profile_timer timer; /* Profiling timer. */
   int saved_errno; /* Saved errno when profiler failed. */
   uint32_t lib_adds; /* Number of libs loaded. Monotonic. */
@@ -95,6 +95,11 @@ static int stream_is_needed(struct sysprof *sp)
   return sp->opt.mode != LUAM_SYSPROF_DEFAULT;
 }
 
+static int is_unconfigured(struct sysprof *sp)
+{
+  return sp->backtracer == NULL || sp->on_stop == NULL || sp->writer == NULL;
+}
+
 static void stream_prologue(struct sysprof *sp)
 {
   lj_symtab_dump(&sp->out, sp->g, &sp->lib_adds);
@@ -206,8 +211,8 @@ static void default_backtrace_host(void *(writer)(int frame_no, void *addr))
 
 static void stream_backtrace_host(struct sysprof *sp)
 {
-  lua_assert(sp->config.backtracer != NULL);
-  sp->config.backtracer(stream_frame_host);
+  lua_assert(sp->backtracer != NULL);
+  sp->backtracer(stream_frame_host);
   lj_wbuf_addu64(&sp->out, (uintptr_t)LJP_FRAME_HOST_LAST);
 }
 
@@ -325,15 +330,11 @@ static int sysprof_validate(struct sysprof *sp,
                             const struct luam_Sysprof_Options *opt)
 {
   switch (sp->state) {
-    case SPS_UNCONFIGURED:
-      return PROFILE_ERRUSE;
-
     case SPS_IDLE:
       if (opt->mode > LUAM_SYSPROF_CALLGRAPH) {
         return PROFILE_ERRUSE;
       } else if (opt->mode != LUAM_SYSPROF_DEFAULT &&
-                 (opt->buf == NULL || opt->len == 0 ||
-                  sp->config.writer == NULL || sp->config.on_stop == NULL)) {
+                 (opt->buf == NULL || opt->len == 0 || is_unconfigured(sp))) {
         return PROFILE_ERRUSE;
       } else if (opt->interval == 0) {
         return PROFILE_ERRUSE;
@@ -373,24 +374,46 @@ static int sysprof_init(struct sysprof *sp, lua_State *L,
   sp->saved_errno = 0;
 
   if (stream_is_needed(sp))
-    lj_wbuf_init(&sp->out, sp->config.writer, opt->ctx, opt->buf, opt->len);
+    lj_wbuf_init(&sp->out, sp->writer, opt->ctx, opt->buf, opt->len);
 
   return PROFILE_SUCCESS;
 }
 
 /* -- Public profiling API ------------------------------------------------ */
 
-int lj_sysprof_configure(const struct luam_Sysprof_Config *config)
-{
+int lj_sysprof_set_writer(sp_writer writer) {
+  struct sysprof *sp = &sysprof;
+
+  if (sp->state != SPS_IDLE || writer == NULL)
+    return PROFILE_ERRUSE;
+
+  sp->writer = writer;
+  if (!is_unconfigured(sp)) {
+    sp->state = SPS_IDLE;
+  }
+  return PROFILE_SUCCESS;
+}
+
+int lj_sysprof_set_on_stop(sp_on_stop on_stop) {
   struct sysprof *sp = &sysprof;
-  lua_assert(config != NULL);
-  if (sp->state != SPS_UNCONFIGURED && sp->state != SPS_IDLE)
+
+  if (sp->state != SPS_IDLE || on_stop == NULL)
     return PROFILE_ERRUSE;
 
-  memcpy(&sp->config, config, sizeof(*config));
+  sp->on_stop = on_stop;
+  if (!is_unconfigured(sp)) {
+    sp->state = SPS_IDLE;
+  }
+  return PROFILE_SUCCESS;
+}
+
+int lj_sysprof_set_backtracer(sp_backtracer backtracer) {
+  struct sysprof *sp = &sysprof;
 
-  if (sp->config.backtracer == NULL) {
-    sp->config.backtracer = default_backtrace_host;
+  if (sp->state != SPS_IDLE)
+    return PROFILE_ERRUSE;
+  if (backtracer == NULL) {
+    sp->backtracer = default_backtrace_host;
     /*
     ** XXX: `backtrace` is not signal-safe, according to man,
     ** because it is lazy loaded on the first call, which triggers
@@ -400,9 +423,12 @@ int lj_sysprof_configure(const struct luam_Sysprof_Config *config)
     void *dummy = NULL;
     backtrace(&dummy, 1);
   }
-
-  sp->state = SPS_IDLE;
-
+  else {
+    sp->backtracer = backtracer;
+  }
+  if (!is_unconfigured(sp)) {
+    sp->state = SPS_IDLE;
+  }
   return PROFILE_SUCCESS;
 }
 
@@ -412,12 +438,12 @@ int lj_sysprof_start(lua_State *L, const struct luam_Sysprof_Options *opt)
 
   int status = sysprof_init(sp, L, opt);
   if (PROFILE_SUCCESS != status) {
-    if (NULL != sp->config.on_stop) {
+    if (NULL != sp->on_stop) {
       /*
       ** Initialization may fail in case of unconfigured sysprof,
       ** so we cannot guarantee cleaning up resources in this case.
       */
-      sp->config.on_stop(opt->ctx, opt->buf);
+      sp->on_stop(opt->ctx, opt->buf);
     }
     return status;
   }
@@ -430,7 +456,7 @@ int lj_sysprof_start(lua_State *L, const struct luam_Sysprof_Options *opt)
       /* on_stop call may change errno value. */
       const int saved_errno = lj_wbuf_errno(&sp->out);
       /* Ignore possible errors. mp->out.buf may be NULL here. */
-      sp->config.on_stop(opt->ctx, sp->out.buf);
+      sp->on_stop(opt->ctx, sp->out.buf);
       lj_wbuf_terminate(&sp->out);
       sp->state = SPS_IDLE;
       errno = saved_errno;
@@ -473,7 +499,7 @@ int lj_sysprof_stop(lua_State *L)
     stream_epilogue(sp);
     lj_wbuf_flush(out);
 
-    cb_status = sp->config.on_stop(sp->opt.ctx, out->buf);
+    cb_status = sp->on_stop(sp->opt.ctx, out->buf);
     if (LJ_UNLIKELY(lj_wbuf_test_flag(out, STREAM_ERRIO | STREAM_STOP)) ||
         cb_status != 0) {
       errno = lj_wbuf_errno(out);
@@ -530,9 +556,18 @@ void lj_sysprof_add_trace(const struct GCtrace *tr)
 
 #else /* LJ_HASSYSPROF */
 
-int lj_sysprof_configure(const struct luam_Sysprof_Config *config)
-{
-  UNUSED(config);
+int lj_sysprof_set_writer(sp_writer writer) {
+  UNUSED(writer);
+  return PROFILE_ERRUSE;
+}
+
+int lj_sysprof_set_on_stop(sp_on_stop on_stop) {
+  UNUSED(on_stop);
+  return PROFILE_ERRUSE;
+}
+
+int lj_sysprof_set_backtracer(sp_backtracer backtracer) {
+  UNUSED(backtracer);
   return PROFILE_ERRUSE;
 }
 
diff --git a/src/lj_sysprof.h b/src/lj_sysprof.h
index c31d61d8..d869013e 100644
--- a/src/lj_sysprof.h
+++ b/src/lj_sysprof.h
@@ -86,7 +86,11 @@
 #define LJP_SYMTAB_TRACE_EVENT ((uint8_t)12)
 #define LJP_EPILOGUE_BYTE 0x80
 
-int lj_sysprof_configure(const struct luam_Sysprof_Config *config);
+int lj_sysprof_set_writer(sp_writer writer);
+
+int lj_sysprof_set_on_stop(sp_on_stop on_stop);
+
+int lj_sysprof_set_backtracer(sp_backtracer backtracer);
 
 int lj_sysprof_start(lua_State *L, const struct luam_Sysprof_Options *opt);
 
diff --git a/src/lmisclib.h b/src/lmisclib.h
index b41f7f59..3ebf18ff 100644
--- a/src/lmisclib.h
+++ b/src/lmisclib.h
@@ -63,31 +63,29 @@ LUAMISC_API void luaM_metrics(lua_State *L, struct luam_Metrics *metrics);
 /* --- Sysprof - platform and lua profiler -------------------------------- */
 
 /* Profiler configurations. */
-struct luam_Sysprof_Config {
-  /*
-  ** Writer function for profile events. Must be async-safe, see also
-  ** `man 7 signal-safety`.
-  ** Should return amount of written bytes on success or zero in case of error.
-  ** Setting *data to NULL means end of profiling.
-  ** For details see <lj_wbuf.h>.
-  */
-  size_t (*writer)(const void **data, size_t len, void *ctx);
-  /*
-  ** Callback on profiler stopping. Required for correctly cleaning
-  ** at VM finalization when profiler is still running.
-  ** Returns zero on success.
-  */
-  int (*on_stop)(void *ctx, uint8_t *buf);
-  /*
-  ** Backtracing function for the host stack. Should call `frame_writer` on
-  ** each frame in the stack in the order from the stack top to the stack
-  ** bottom. The `frame_writer` function is implemented inside the sysprof
-  ** and will be passed to the `backtracer` function. If `frame_writer` returns
-  ** NULL, backtracing should be stopped. If `frame_writer` returns not NULL,
-  ** the backtracing should be continued if there are frames left.
-  */
-  void (*backtracer)(void *(*frame_writer)(int frame_no, void *addr));
-};
+/*
+** Writer function for profile events. Must be async-safe, see also
+** `man 7 signal-safety`.
+** Should return amount of written bytes on success or zero in case of error.
+** Setting *data to NULL means end of profiling.
+** For details see <lj_wbuf.h>.
+*/
+typedef size_t (*sp_writer)(const void **data, size_t len, void *ctx);
+/*
+** Callback on profiler stopping. Required for correctly cleaning
+** at VM finalization when profiler is still running.
+** Returns zero on success.
+*/
+typedef int (*sp_on_stop)(void *ctx, uint8_t *buf);
+/*
+** Backtracing function for the host stack. Should call `frame_writer` on
+** each frame in the stack in the order from the stack top to the stack
+** bottom. The `frame_writer` function is implemented inside the sysprof
+** and will be passed to the `backtracer` function. If `frame_writer` returns
+** NULL, backtracing should be stopped. If `frame_writer` returns not NULL,
+** the backtracing should be continued if there are frames left.
+*/
+typedef void (*sp_backtracer)(void *(*frame_writer)(int frame_no, void *addr));
 
 /*
 ** DEFAULT mode collects only data for luam_sysprof_counters, which is stored
@@ -144,7 +142,11 @@ struct luam_Sysprof_Options {
 #define PROFILE_ERRMEM  3
 #define PROFILE_ERRIO   4
 
-LUAMISC_API int luaM_sysprof_configure(const struct luam_Sysprof_Config *config);
+LUAMISC_API int luaM_sysprof_set_writer(sp_writer writer);
+
+LUAMISC_API int luaM_sysprof_set_on_stop(sp_on_stop on_stop);
+
+LUAMISC_API int luaM_sysprof_set_backtracer(sp_backtracer backtracer);
 
 LUAMISC_API int luaM_sysprof_start(lua_State *L,
                                    const struct luam_Sysprof_Options *opt);
diff --git a/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c b/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
index 4b258766..984fc121 100644
--- a/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
+++ b/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
@@ -122,14 +122,9 @@ static int c_payload(lua_State *L)
 
 static int base(lua_State *L)
 {
-	struct luam_Sysprof_Config config = {};
 	struct luam_Sysprof_Options opt = {};
 	struct luam_Sysprof_Counters cnt = {};
 
-	(void)config.writer;
-	(void)config.on_stop;
-	(void)config.backtracer;
-
 	(void)opt.interval;
 	(void)opt.mode;
 	(void)opt.ctx;
@@ -156,13 +151,9 @@ static int base(lua_State *L)
 
 static int validation(lua_State *L)
 {
-	struct luam_Sysprof_Config config = {};
 	struct luam_Sysprof_Options opt = {};
 	int status = PROFILE_SUCCESS;
 
-	status = luaM_sysprof_configure(&config);
-	assert(PROFILE_SUCCESS == status);
-
 	/* Unknown mode. */
 	opt.mode = 0x40;
 	status = luaM_sysprof_start(L, &opt);
@@ -204,7 +195,6 @@ static int validation(lua_State *L)
 
 static int profile_func(lua_State *L)
 {
-	struct luam_Sysprof_Config config = {};
 	struct luam_Sysprof_Options opt = {};
 	struct luam_Sysprof_Counters cnt = {};
 	int status = PROFILE_ERRUSE;
@@ -222,10 +212,9 @@ static int profile_func(lua_State *L)
 	opt.interval = SYSPROF_INTERVAL_DEFAULT;
 	stream_init(&opt);
 
-	config.on_stop = on_stop_cb_default;
-	config.writer = buffer_writer_default;
-	status = luaM_sysprof_configure(&config);
-	assert(PROFILE_SUCCESS == status);
+  assert(luaM_sysprof_set_writer(buffer_writer_default) == PROFILE_SUCCESS);
+  assert(luaM_sysprof_set_on_stop(on_stop_cb_default) == PROFILE_SUCCESS);
+  assert(luaM_sysprof_set_backtracer(NULL) == PROFILE_SUCCESS);
 
 	status = luaM_sysprof_start(L, &opt);
 	assert(PROFILE_SUCCESS == status);
diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
index bb0e065d..2f53d771 100644
--- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
@@ -70,7 +70,7 @@ res, err = misc.sysprof.start{ mode = "D" }
 assert(res, err)
 
 res, err, errno = misc.sysprof.start{ mode = "D" }
-test:ok(res == nil and err:match("profiler misuse"))
+test:ok(res == nil and err:match("profiler is running already"))
 test:ok(type(errno) == "number")
 
 res, err = misc.sysprof.stop()
-- 
2.35.1