[Tarantool-patches] [PATCH luajit] sysprof: move symtab update into profile hook

[Tarantool-patches] [PATCH luajit] sysprof: move symtab update into profile hook

[Maxim Kokryashkin via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH luajit] sysprof: move symtab update into profile hook

[Sergey Kaplun via Tarantool-patches]

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

On 23.05.23, Maxim Kokryashkin wrote:
> 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

<snipped>

> 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);

Nit: I suppose, that two lines above may be joined.

> +  sp->symtab_update_needed = 0;
> +  if (!(mask & HOOK_VMEVENT)) {

Does it mean, that if we have signal inside VMEVENT we don't update our
symtab table (since flag is reset) (*)? Should we move the line
| sp->symtab_update_needed = 0;
inside if condition?


> +    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))) {

Why GC hook is mentioned here?

> +    sp->symtab_update_needed = 1;
> +    g->hookmask = (mask | HOOK_PROFILE);
> +    lj_dispatch_update(g);
> +  }

The similar to (*) question here: If we have a signal inside VMEVENT, does
this mean that there is no symbols to load for sure?
If so, coment is highly desired.

> +}
> +
>  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)

<snipped>

> 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);

<snipped>

> 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

<snipped>

> 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",
> +})

I've reverted your changes and test still passes before the patch:

| /home/burii/reviews/luajit/gh-8140-sysprof-in-alloc/test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua ............. ok   
| All tests successful.

I've restored the sysprof.[ch] and dispatch changes, and leave tests as
is.

| gst
| On branch fckxorg/gh-8140-sysprof-allocator-crash
| Your branch is behind 'origin/fckxorg/gh-8140-sysprof-allocator-crash' by 1 commit, and can be fast-forwarded.
|   (use "git pull" to update your local branch)
|
| Changes to be committed:
|   (use "git restore --staged <file>..." to unstage)
|         modified:   test/tarantool-tests/CMakeLists.txt
|         new file:   test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua
|         new file:   test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt
|         new file:   test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c


I've tried several times and test still passes.

> +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

May be we should use more GC-specific payload?

> +
> +local function callback(_, _, _)
> +    callback_called = true
> +end
> +
> +profile.start('f', callback)
> +payload()
> +profile.stop()
> +
> +test:ok(callback_called, 'LuaJIT profiler callback was not called.')

Looks like it should be "was called"?

> +
> +jit.off()
> +misc.sysprof.start{mode='C', interval=1, path=TMP_BINFILE}
> +for _ = 1, 1e4 do
> +    require('sysprofalloc').get_string()

Why do you try to require the module several times?
Is it saved in `package.loaded` anyway?

> +    -- 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.')

Looks like it should be "has not 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"

Do we need both hashes here?

> +  )
> +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;

Something strange with indentation: please, use tabs for __our__ C
code in tests.

> +}
> +
> +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
> 

-- 
Best regards,
Sergey Kaplun