Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] sysprof: move symtab update into profile hook
@ 2023-05-23 15:27 Maxim Kokryashkin via Tarantool-patches
  2023-05-24  9:46 ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 2+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-05-23 15:27 UTC (permalink / raw)
  To: tarantool-patches, skaplun, sergos

Before the patch, the symtab update was done in the signal
handler. That update requires memory allocation, which
can't be done safely in a signal handler. This patch reuses
LuaJIT's HOOK_PROFILE for symtab update routine execution, so
it is now possible to update it safely after the signal
handler exit.

Resolves tarantool/tarantool#8140
---
Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-8140-sysprof-allocator-crash
Issue: https://github.com/tarantool/tarantool/issues/8140
PR: https://github.com/tarantool/tarantool/pull/8691

 src/lj_dispatch.c                             | 10 ++++
 src/lj_sysprof.c                              | 34 +++++++++++++-
 src/lj_sysprof.h                              |  4 ++
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../gh-8140-sysprof-allocator-crash.test.lua  | 46 +++++++++++++++++++
 .../CMakeLists.txt                            |  9 ++++
 .../sysprofalloc.c                            | 17 +++++++
 7 files changed, 119 insertions(+), 2 deletions(-)
 create mode 100644 test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua
 create mode 100644 test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt
 create mode 100644 test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c

diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
index ee735450..65948a0c 100644
--- a/src/lj_dispatch.c
+++ b/src/lj_dispatch.c
@@ -30,6 +30,9 @@
 #if LJ_HASPROFILE
 #include "lj_profile.h"
 #endif
+#if LJ_HASSYSPROF
+#include "lj_sysprof.h"
+#endif
 #include "lj_vm.h"
 #include "luajit.h"
 
@@ -552,7 +555,14 @@ void LJ_FASTCALL lj_dispatch_profile(lua_State *L, const BCIns *pc)
   global_State *g;
   setcframe_pc(cf, pc);
   L->top = L->base + cur_topslot(pt, pc, cframe_multres_n(cf));
+#if LJ_HASSYSPROF
+  if (lj_symtab_update_requested())
+    lj_symtab_update_hook(L);
+  else
+    lj_profile_interpreter(L);
+#else
   lj_profile_interpreter(L);
+#endif
   setcframe_pc(cf, oldpc);
   g = G(L);
   setgcref(g->cur_L, obj2gco(L));
diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index 2e9ed9b3..5b5cbe2a 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -76,6 +76,7 @@ struct sysprof {
   lj_profile_timer timer; /* Profiling timer. */
   int saved_errno; /* Saved errno when profiler failed. */
   uint32_t lib_adds; /* Number of libs loaded. Monotonic. */
+  volatile sig_atomic_t symtab_update_needed; /* Symtab update request flag. */
 };
 /*
 ** XXX: Only one VM can be profiled at a time.
@@ -88,6 +89,36 @@ static struct sysprof sysprof = {0};
 static const uint8_t ljp_header[] = {'l', 'j', 'p', LJP_FORMAT_VERSION,
                                       0x0, 0x0, 0x0};
 
+void lj_symtab_update_hook(lua_State *L) {
+  struct sysprof *sp = &sysprof;
+  global_State *g = G(L);
+  uint8_t mask;
+  mask = (g->hookmask & ~HOOK_PROFILE);
+  sp->symtab_update_needed = 0;
+  if (!(mask & HOOK_VMEVENT)) {
+    g->hookmask = HOOK_VMEVENT;
+    lj_dispatch_update(g);
+    lj_symtab_dump_newc(&sp->lib_adds, &sp->out, LJP_SYMTAB_CFUNC_EVENT, L);
+  }
+  g->hookmask = mask;
+  lj_dispatch_update(g);
+}
+
+int lj_symtab_update_requested() {
+  struct sysprof *sp = &sysprof;
+  return sp->symtab_update_needed;
+}
+
+static void setup_symtab_update_hook(struct sysprof *sp) {
+  global_State *g = sp->g;
+  uint8_t mask = g->hookmask;
+  if (!(mask & (HOOK_PROFILE|HOOK_VMEVENT|HOOK_GC))) {
+    sp->symtab_update_needed = 1;
+    g->hookmask = (mask | HOOK_PROFILE);
+    lj_dispatch_update(g);
+  }
+}
+
 static int stream_is_needed(struct sysprof *sp)
 {
   return sp->opt.mode != LUAM_SYSPROF_DEFAULT;
@@ -240,8 +271,7 @@ static void stream_guest(struct sysprof *sp, uint32_t vmstate)
 
 static void stream_host(struct sysprof *sp, uint32_t vmstate)
 {
-  struct lua_State *L = gco2th(gcref(sp->g->cur_L));
-  lj_symtab_dump_newc(&sp->lib_adds, &sp->out, LJP_SYMTAB_CFUNC_EVENT, L);
+  setup_symtab_update_hook(sp);
   lj_wbuf_addbyte(&sp->out, (uint8_t)vmstate);
   stream_backtrace_host(sp);
 }
diff --git a/src/lj_sysprof.h b/src/lj_sysprof.h
index 7e8c2e6e..456ca76f 100644
--- a/src/lj_sysprof.h
+++ b/src/lj_sysprof.h
@@ -108,4 +108,8 @@ void lj_sysprof_add_proto(const struct GCproto *pt);
 void lj_sysprof_add_trace(const struct GCtrace *tr);
 #endif /* LJ_HASJIT */
 
+void lj_symtab_update_hook(lua_State *L);
+
+int lj_symtab_update_requested();
+
 #endif
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index a428d009..25244733 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -61,6 +61,7 @@ add_subdirectory(gh-5813-resolving-of-c-symbols/gnuhash)
 add_subdirectory(gh-5813-resolving-of-c-symbols/stripped)
 add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64)
 add_subdirectory(gh-6189-cur_L)
+add_subdirectory(gh-8140-sysprof-allocator-crash)
 add_subdirectory(lj-49-bad-lightuserdata)
 add_subdirectory(lj-416-xor-before-jcc)
 add_subdirectory(lj-601-fix-gc-finderrfunc)
diff --git a/test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua b/test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua
new file mode 100644
index 00000000..afce83e2
--- /dev/null
+++ b/test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua
@@ -0,0 +1,46 @@
+local tap = require('tap')
+local test = tap.test('gh-8140-sysprof-allocator-crash'):skipcond({
+  ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
+                                               jit.arch ~= "x64",
+  ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
+})
+test:plan(2)
+
+local profilename = require("utils").profilename
+local profile = require('jit.profile')
+
+local TMP_BINFILE = profilename("sysprofdata.tmp.bin")
+local callback_called = false
+
+local function payload()
+  local r = 0
+  for i = 1, 1e8 do
+    r = r + i
+  end
+  return r
+end
+
+local function callback(_, _, _)
+    callback_called = true
+end
+
+profile.start('f', callback)
+payload()
+profile.stop()
+
+test:ok(callback_called, 'LuaJIT profiler callback was not called.')
+
+jit.off()
+misc.sysprof.start{mode='C', interval=1, path=TMP_BINFILE}
+for _ = 1, 1e4 do
+    require('sysprofalloc').get_string()
+    -- Make sure that C library is collected, so it will be loaded
+    -- again on the next iteration.
+    collectgarbage()
+end
+misc.sysprof.stop()
+
+test:ok(true, 'Sysprof has crashed.')
+
+os.remove(TMP_BINFILE)
+os.exit(test:check() and 0 or 1)
diff --git a/test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt b/test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt
new file mode 100644
index 00000000..dbe60219
--- /dev/null
+++ b/test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt
@@ -0,0 +1,9 @@
+if (NOT(CMAKE_SYSTEM_NAME STREQUAL "Darwin"))
+  BuildTestCLib(sysprofalloc sysprofalloc.c)
+  # Unfortunately, <target_link_options> command is introduced
+  # since CMake 3.13, so we can't use it now considering ancient
+  # distros support. Just build linker flags by hands.
+  set(CMAKE_SHARED_LINKER_FLAGS
+    "${CMAKE_SHARED_LINKER_FLAGS} -Wl,--hash-style=both"
+  )
+endif()
diff --git a/test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c b/test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c
new file mode 100644
index 00000000..d3c41d42
--- /dev/null
+++ b/test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c
@@ -0,0 +1,17 @@
+#include <lua.h>
+#include <lauxlib.h>
+
+int get_string(lua_State *L) {
+    lua_pushstring(L, "test string");
+    return 1;
+}
+
+static const struct luaL_Reg sysprofalloc [] = {
+    {"get_string", get_string},
+    {NULL, NULL}
+};
+
+int luaopen_sysprofalloc(lua_State *L) {
+    luaL_register(L, "sysprofalloc", sysprofalloc);
+    return 1;
+}
-- 
2.40.1


^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-05-24  9:50 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-23 15:27 [Tarantool-patches] [PATCH luajit] sysprof: move symtab update into profile hook Maxim Kokryashkin via Tarantool-patches
2023-05-24  9:46 ` 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