

<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM, except for two questions below.</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16844429450089173137_BODY">This patch rewrites the aforementioned test with the usage libtest<br>recently introduced. The approach is similar to the previous patch.<br>`c_payload()` function to profile is now set up globally on script<br>loading. The payload for enabled and disabled JIT is the same. As far as<br>for sysprof testing is necessary to replace backtrace with libunwind<br>first, the payload tests are marked with `todo()`.<br><br>Nevertheless, glibc `assert()` still necessary to check the correctness<br>of the profile writer, so it is still used there.<br><br>Relates to tarantool/tarantool#781<br>Part of tarantool/tarantool#7900<br>---<br> .../misclib-sysprof-capi-script.lua | 35 ++<br> .../misclib-sysprof-capi.test.c | 325 ++++++++++++++++++<br> test/tarantool-tests/CMakeLists.txt | 1 -<br> .../misclib-sysprof-capi.test.lua | 54 ---<br> .../misclib-sysprof-capi/CMakeLists.txt | 1 -<br> .../misclib-sysprof-capi/testsysprof.c | 260 --------------<br> 6 files changed, 360 insertions(+), 316 deletions(-)<br> create mode 100644 test/tarantool-c-tests/misclib-sysprof-capi-script.lua<br> create mode 100644 test/tarantool-c-tests/misclib-sysprof-capi.test.c<br> delete mode 100644 test/tarantool-tests/misclib-sysprof-capi.test.lua<br> delete mode 100644 test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt<br> delete mode 100644 test/tarantool-tests/misclib-sysprof-capi/testsysprof.c<br><br>diff --git a/test/tarantool-c-tests/misclib-sysprof-capi-script.lua b/test/tarantool-c-tests/misclib-sysprof-capi-script.lua<br>new file mode 100644<br>index 00000000..dd8387db<br>--- /dev/null<br>+++ b/test/tarantool-c-tests/misclib-sysprof-capi-script.lua<br><snipped></div></div></div></div></blockquote><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div> </div></div></div></div></blockquote><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>diff --git a/test/tarantool-c-tests/misclib-sysprof-capi.test.c b/test/tarantool-c-tests/misclib-sysprof-capi.test.cnew file mode 100644<br>index 00000000..f75f82cd<br>--- /dev/null<br>+++ b/test/tarantool-c-tests/misclib-sysprof-capi.test.c<br>@@ -0,0 +1,325 @@<br>+#include "lauxlib.h"<br>+#include "lmisclib.h"<br>+#include "lua.h"<br>+#include "luajit.h"<br>+<br>+#include "test.h"<br>+#include "utils.h"<br>+<br>+#include <errno.h><br>+#include <fcntl.h><br>+#include <stdlib.h><br>+#include <unistd.h><br>+<br>+/* XXX: Still need normal assert inside writer functions. */<br>+#undef NDEBUG<br>+#include <assert.h><br>+<br>+/* Need for skipcond for OS and ARCH. */<br>+#include "lj_arch.h"<br>+<br>+#define UNUSED(x) ((void)(x))<br>+<br>+/* --- utils -------------------------------------------------- */<br>+<br>+#define SYSPROF_INTERVAL_DEFAULT 100<br>+<br>+/*<br>+ * Yep, 8Mb. Tuned in order not to bother the platform with too<br>+ * often flushes.<br>+ */<br>+#define STREAM_BUFFER_SIZE (8 * 1024 * 1024)<br>+<br>+<br>+/* --- C Payload ---------------------------------------------- */<br>+<br>+static double fib(double n)<br>+{<br>+ if (n <= 1)<br>+ return n;<br>+ return fib(n - 1) + fib(n - 2);<br>+}<br>+<br>+static int c_payload(lua_State *L)<br>+{<br>+ fib(luaL_checknumber(L, 1));<br>+ lua_pushboolean(L, 1);<br>+ return 1;<br>+}<br>+/* --- sysprof C API tests ------------------------------------ */<br>+<br>+static int base(void *test_state)<br>+{<br>+ UNUSED(test_state);<br>+ struct luam_Sysprof_Options opt = {};<br>+ struct luam_Sysprof_Counters cnt = {};<br>+<br>+ (void)opt.interval;<br>+ (void)opt.mode;<br>+ (void)opt.ctx;<br>+ (void)opt.buf;<br>+ (void)opt.len;<br>+<br>+ luaM_sysprof_report(&cnt);<br>+<br>+ (void)cnt.samples;<br>+ (void)cnt.vmst_interp;<br>+ (void)cnt.vmst_lfunc;<br>+ (void)cnt.vmst_ffunc;<br>+ (void)cnt.vmst_cfunc;<br>+ (void)cnt.vmst_gc;<br>+ (void)cnt.vmst_exit;<br>+ (void)cnt.vmst_record;<br>+ (void)cnt.vmst_opt;<br>+ (void)cnt.vmst_asm;<br>+ (void)cnt.vmst_trace;<br>+<br>+ return TEST_EXIT_SUCCESS;<br>+}<br>+<br>+static int validation(void *test_state)<br>+{<br>+ lua_State *L = test_state;<br>+ struct luam_Sysprof_Options opt = {};<br>+ int status = PROFILE_SUCCESS;<br>+<br>+ /* Unknown mode. */<br>+ opt.mode = 0x40;<br>+ status = luaM_sysprof_start(L, &opt);<br>+ assert_true(status == PROFILE_ERRUSE);<br>+<br>+ /* Buffer not configured. */<br>+ opt.mode = LUAM_SYSPROF_CALLGRAPH;<br>+ opt.buf = NULL;<br>+ status = luaM_sysprof_start(L, &opt);<br>+ assert_true(status == PROFILE_ERRUSE);<br>+<br>+ /* Bad interval. */<br>+ opt.mode = LUAM_SYSPROF_DEFAULT;<br>+ opt.interval = 0;<br>+ status = luaM_sysprof_start(L, &opt);<br>+ assert_true(status == PROFILE_ERRUSE);<br>+<br>+ /* Check if profiling started. */<br>+ opt.mode = LUAM_SYSPROF_DEFAULT;<br>+ opt.interval = SYSPROF_INTERVAL_DEFAULT;<br>+ status = luaM_sysprof_start(L, &opt);<br>+ assert_true(status == PROFILE_SUCCESS);<br>+<br>+ /* Already running. */<br>+ status = luaM_sysprof_start(L, &opt);<br>+ assert_true(status == PROFILE_ERRRUN);<br>+<br>+ /* Profiler stopping. */<br>+ status = luaM_sysprof_stop(L);<br>+ assert_true(status == PROFILE_SUCCESS);<br>+<br>+ /* Stopping profiler which is not running. */<br>+ status = luaM_sysprof_stop(L);<br>+ assert_true(status == PROFILE_ERRRUN);<br>+<br>+ return TEST_EXIT_SUCCESS;<br>+}<br>+<br>+/*<br>+ * FIXME: The following two tests are disabled because sometimes<br>+ * `backtrace` dynamically loads a platform-specific unwinder,<br>+ * which is not signal-safe.<br>+ */<br>+<br>+#if 0<br>+/*<br>+ * Structure given as ctx to sysprof writer and on_stop callback.<br>+ */<br>+struct sysprof_ctx {<br>+ /* Output file descriptor for data. */<br>+ int fd;<br>+ /* Buffer for data. */<br>+ uint8_t buf[STREAM_BUFFER_SIZE];<br>+};<br>+<br>+/*<br>+ * Default buffer writer function.<br>+ * Just call fwrite to the corresponding FILE.<br>+ */<br>+static size_t buffer_writer_default(const void **buf_addr, size_t len,<br>+ void *opt)<br>+{<br>+ struct sysprof_ctx *ctx = opt;<br>+ int fd = ctx->fd;<br>+ const void * const buf_start = *buf_addr;<br>+ const void *data = *buf_addr;<br>+ size_t write_total = 0;<br>+<br>+ assert(len <= STREAM_BUFFER_SIZE);<br>+<br>+ for (;;) {<br>+ const size_t written = write(fd, data, len - write_total);<br>+<br>+ if (written == 0) {<br>+ /* Re-tries write in case of EINTR. */<br>+ if (errno != EINTR) {<br>+ /*<br>+ * Will be freed as whole chunk<br>+ * later.<br>+ */<br>+ *buf_addr = NULL;<br>+ return write_total;<br>+ }<br>+ errno = 0;<br>+ continue;<br>+ }<br>+<br>+ write_total += written;<br>+ assert(write_total <= len);<br>+<br>+ if (write_total == len)<br>+ break;<br>+<br>+ data = (uint8_t *)data + (ptrdiff_t)written;<br>+ }<br>+<br>+ *buf_addr = buf_start;<br>+ return write_total;<br>+}<br>+<br>+/*<br>+ * Default on stop callback. Just close the corresponding stream.<br>+ */<br>+static int on_stop_cb_default(void *opt, uint8_t *buf)<br>+{<br>+ UNUSED(buf);<br>+ struct sysprof_ctx *ctx = opt;<br>+ int fd = ctx->fd;<br>+ free(ctx);<br>+ return close(fd);<br>+}<br>+<br>+static int stream_init(struct luam_Sysprof_Options *opt)<br>+{<br>+ struct sysprof_ctx *ctx = calloc(1, sizeof(struct sysprof_ctx));<br>+ if (NULL == ctx)<br>+ return PROFILE_ERRIO;<br>+<br>+ ctx->fd = open("/dev/null", O_WRONLY | O_CREAT, 0644);<br>+ if (-1 == ctx->fd) {<br>+ free(ctx);<br>+ return PROFILE_ERRIO;<br>+ }<br>+<br>+ opt->ctx = ctx;<br>+ opt->buf = ctx->buf;<br>+ opt->len = STREAM_BUFFER_SIZE;<br>+<br>+ return PROFILE_SUCCESS;<br>+}<br>+<br>+/* Get function to profile on top, call it. */<br>+static int check_profile_func(lua_State *L)<br>+{<br>+ struct luam_Sysprof_Options opt = {};<br>+ struct luam_Sysprof_Counters cnt = {};<br>+ int status = PROFILE_ERRUSE;<br>+ /*<br>+ * Since all the other modes functionality is the<br>+ * subset of CALLGRAPH mode, run this mode to test<br>+ * the profiler's behavior.<br>+ */<br>+ opt.mode = LUAM_SYSPROF_CALLGRAPH;<br>+ opt.interval = SYSPROF_INTERVAL_DEFAULT;<br>+ stream_init(&opt);<br>+<br>+ /*<br>+ * XXX: Payload function on top will not be removed if any<br>+ * of those assertions fail. So, the next call to the<br>+ * `utils_get_aux_lfunc()` will fail. It's OK, since<br>+ * we are already in trouble, just keep it in mind.<br>+ */<br>+ assert_true(luaM_sysprof_set_writer(buffer_writer_default)<br>+ == PROFILE_SUCCESS);<br>+ assert_true(luaM_sysprof_set_on_stop(on_stop_cb_default)<br>+ == PROFILE_SUCCESS);<br>+ assert_true(luaM_sysprof_set_backtracer(NULL) == PROFILE_SUCCESS);<br>+<br>+ status = luaM_sysprof_start(L, &opt);<br>+ assert_true(status == PROFILE_SUCCESS);<br>+<br>+ /* Run payload. */<br>+ if (lua_pcall(L, 0, 0, 0) != LUA_OK) {<br>+ test_comment("error running payload: %s", lua_tostring(L, -1));<br>+ bail_out("error running sysprof test payload");<br>+ }<br>+<br>+ status = luaM_sysprof_stop(L);<br>+ assert_true(status == PROFILE_SUCCESS);<br>+<br>+ status = luaM_sysprof_report(&cnt);<br>+ assert_true(status == PROFILE_SUCCESS);<br>+<br>+ assert_true(cnt.samples > 1);<br>+ assert_true(cnt.samples == cnt.vmst_asm +<br>+ cnt.vmst_cfunc +<br>+ cnt.vmst_exit +<br>+ cnt.vmst_ffunc +<br>+ cnt.vmst_gc +<br>+ cnt.vmst_interp +<br>+ cnt.vmst_lfunc +<br>+ cnt.vmst_opt +<br>+ cnt.vmst_record +<br>+ cnt.vmst_trace);<br>+<br>+ return TEST_EXIT_SUCCESS;<br>+}<br>+#endif<br>+<br>+static int profile_func_jitoff(void *test_state)<br>+{<br>+ UNUSED(test_state);<br>+ return todo("Need to replace backtrace with libunwind first");<br>+#if 0<br>+ lua_State *L = test_state;<br>+ utils_get_aux_lfunc(L);<br>+ (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_OFF);<br>+ (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_FLUSH);<br>+ check_profile_func(L);<br>+ (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_ON);<br>+ return TEST_EXIT_SUCCESS;<br>+#endif<br>+}<br>+<br>+static int profile_func_jiton(void *test_state)<br>+{<br>+ UNUSED(test_state);<br>+ return todo("Need to replace backtrace with libunwind first");<br>+#if 0<br>+ lua_State *L = test_state;<br>+ utils_get_aux_lfunc(L);<br>+ check_profile_func(L);<br>+ return TEST_EXIT_SUCCESS;<br>+#endif<br>+}<br>+<br>+int main(void)<br>+{<br>+ if (LUAJIT_OS != LUAJIT_OS_LINUX)<br>+ return skip_all("Sysprof is implemented for Linux only");<br>+ if (LUAJIT_TARGET != LUAJIT_ARCH_X86<br>+ && LUAJIT_TARGET != LUAJIT_ARCH_X64)<br>+ return skip_all("Sysprof is implemented for x86_64 only");<br>+<br>+ lua_State *L = utils_lua_init();<br>+<br>+ lua_pushcfunction(L, c_payload);<br>+ lua_setfield(L, LUA_GLOBALSINDEX, "c_payload");</div></div></div></div></blockquote><div>Is there a cleaner approach?</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div class="js-helper js-readmsg-msg">+ utils_load_aux_script(L);<br>+<br>+ const struct test_unit tgroup[] = {<br>+ test_unit_new(base),<br>+ test_unit_new(validation),<br>+ test_unit_new(profile_func_jitoff),<br>+ test_unit_new(profile_func_jiton)</div></blockquote><div>Maybe we shouldn’t put these tests into group as long as they are disabled?</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div class="js-helper js-readmsg-msg">+ };<br>+ const int test_result = test_run_group(tgroup, L);<br>+ utils_lua_close(L);<br>+ return test_result;<br>+}<br><snipped></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div><div> </div></div></blockquote></BODY></HTML>