Re: Can't interrupt tight loops in Lua 5.4 anymore?

["Joseph C. Sible" <josephcsible@...>]

On Tue, May 19, 2020 at 8:21 AM Roberto Ierusalimschy
<roberto@inf.puc-rio.br> wrote:
>
> > Under the assumption that function pointers are atomic, perhaps
> > there's a way to fix the CallInfo race, the Undefined Behavior with C
> > threads, and being unable to stop a coroutine, all at once: add the
> > concept of a "global hook". Here's how I think it could work:
> >
> > * Create a new function called "lua_noophook" that just returns
> > without doing anything.
> > * Add a single "volatile" function pointer to global_State called
> > "globalhook", that points to lua_noophook by default.
> > * Everywhere that we check to run hooks now, also unconditionally call
> > the "G(L)->globalhook" function pointer, as if it were a LUA_MASKCOUNT
> > hook with a count of 1.
> >
> > I think that would have the desired semantics in almost every case
> > today that currently sets a hook asynchronously. (And in the uncommon
> > case of wanting to do something else, you could do it from the global
> > hook, and then reset it back to lua_noophook.)
> >
> > Does this seem like a good solution?
>
> Any estimates of the runtime cost of this implementation?
>
> -- Roberto

I implemented my idea, attached as a patch and also viewable here:
https://github.com/josephcsible/lua/commit/0d83524122ba09467fcade6f66fbfdef67fe5d39

I also switched the standalone lua binary's SIGINT hook to use the new
global hook, so as a quick test, you can run this in it:

    coroutine.wrap(function() while true do end end)()

And you'll now be able to interrupt that with a single Ctrl+C.

As a very crude benchmark, I ran ./all 20 times without my patch and
20 times with it, and compared the time.txt outputs. Bizarrely, the
average runtime was slightly lower with my patch. I don't think my
patch actually made it faster, but rather just that the minuscule
extra slowness got completely buried by noise.

If someone has a better Lua benchmark, please try it and share the results.

Also, let me know if there's any improvements I can make to this patch
or anything I can do to increase this feature's chances of being
incorporated in the official Lua release.

Joseph C. Sible

From 0d83524122ba09467fcade6f66fbfdef67fe5d39 Mon Sep 17 00:00:00 2001
From: "Joseph C. Sible" <josephcsible@gmail.com>
Date: Tue, 19 May 2020 21:08:47 -0400
Subject: [PATCH] Add the concept of a "global hook"

This is meant to address the following shortcomings in the current debug hooks,
related to asynchronously setting them:

* Coroutines not being interrupted by hooks that are unaware of them
* Race conditions causing some tight loops to fail to be interrupted
* Memory ordering problems when setting a hook from a different C thread
---
 ldblib.c         | 72 +++++++++++++++++++++++++++++++++++++++++-------
 ldebug.c         | 27 ++++++++++++++----
 ldo.c            | 24 ++++++++++------
 ldo.h            |  2 +-
 lstate.c         |  1 +
 lstate.h         |  1 +
 lua.c            |  4 +--
 lua.h            |  2 ++
 lvm.c            |  4 +--
 manual/manual.of | 61 ++++++++++++++++++++++++++++++++++++++++
 10 files changed, 170 insertions(+), 28 deletions(-)

diff --git a/ldblib.c b/ldblib.c
index 745cfd27..24c22e57 100644
--- a/ldblib.c
+++ b/ldblib.c
@@ -27,6 +27,13 @@
 static const char *const HOOKKEY = "_HOOKKEY";
 
 
+/*
+** The hook function at registry[GLOBALHOOKKEY] contains the current
+** global hook function.
+*/
+static const char *const GLOBALHOOKKEY = "_GLOBALHOOKKEY";
+
+
 /*
 ** If L1 != L, L1 can be in any state, and therefore there are no
 ** guarantees about its stack space; any push in L1 must be
@@ -307,23 +314,37 @@ static int db_upvaluejoin (lua_State *L) {
 }
 
 
+static void commonhookf (lua_State *L, lua_Debug *ar) {
+  static const char *const hooknames[] =
+    {"call", "return", "line", "count", "tail call"};
+  lua_pushstring(L, hooknames[(int)ar->event]);  /* push event name */
+  if (ar->currentline >= 0)
+    lua_pushinteger(L, ar->currentline);  /* push current line */
+  else lua_pushnil(L);
+  lua_assert(lua_getinfo(L, "lS", ar));
+  lua_call(L, 2, 0);  /* call hook function */
+}
+
+
 /*
 ** Call hook function registered at hook table for the current
 ** thread (if there is one)
 */
 static void hookf (lua_State *L, lua_Debug *ar) {
-  static const char *const hooknames[] =
-    {"call", "return", "line", "count", "tail call"};
   lua_getfield(L, LUA_REGISTRYINDEX, HOOKKEY);
   lua_pushthread(L);
-  if (lua_rawget(L, -2) == LUA_TFUNCTION) {  /* is there a hook function? */
-    lua_pushstring(L, hooknames[(int)ar->event]);  /* push event name */
-    if (ar->currentline >= 0)
-      lua_pushinteger(L, ar->currentline);  /* push current line */
-    else lua_pushnil(L);
-    lua_assert(lua_getinfo(L, "lS", ar));
-    lua_call(L, 2, 0);  /* call hook function */
-  }
+  if (lua_rawget(L, -2) == LUA_TFUNCTION)  /* is there a hook function? */
+    commonhookf(L, ar);
+}
+
+
+/*
+** Call registered global hook function (if there is one)
+*/
+static void globalhookf (lua_State *L, lua_Debug *ar) {
+  /* is there a hook function? */
+  if (lua_getfield(L, LUA_REGISTRYINDEX, GLOBALHOOKKEY) == LUA_TFUNCTION)
+    commonhookf(L, ar);
 }
 
 
@@ -408,6 +429,35 @@ static int db_gethook (lua_State *L) {
 }
 
 
+static int db_setglobalhook (lua_State *L) {
+  lua_Hook func;
+  if (lua_isnoneornil(L, 1)) {  /* no hook? */
+    lua_settop(L, 1);
+    func = NULL;  /* turn off hooks */
+  }
+  else {
+    luaL_checktype(L, 1, LUA_TFUNCTION);
+    func = globalhookf;
+  }
+  lua_pushvalue(L, 1);  /* value (hook function) */
+  lua_setfield(L, LUA_REGISTRYINDEX, GLOBALHOOKKEY);
+  lua_setglobalhook(L, func);
+  return 0;
+}
+
+
+static int db_getglobalhook (lua_State *L) {
+  lua_Hook globalhook = lua_getglobalhook(L);
+  if (globalhook == NULL)  /* no hook? */
+    luaL_pushfail(L);
+  else if (globalhook != globalhookf)  /* external hook? */
+    lua_pushliteral(L, "external hook");
+  else  /* hook function must exist */
+    lua_getfield(L, LUA_REGISTRYINDEX, GLOBALHOOKKEY);
+  return 1;
+}
+
+
 static int db_debug (lua_State *L) {
   for (;;) {
     char buffer[250];
@@ -451,6 +501,7 @@ static int db_setcstacklimit (lua_State *L) {
 static const luaL_Reg dblib[] = {
   {"debug", db_debug},
   {"getuservalue", db_getuservalue},
+  {"getglobalhook", db_getglobalhook},
   {"gethook", db_gethook},
   {"getinfo", db_getinfo},
   {"getlocal", db_getlocal},
@@ -460,6 +511,7 @@ static const luaL_Reg dblib[] = {
   {"upvaluejoin", db_upvaluejoin},
   {"upvalueid", db_upvalueid},
   {"setuservalue", db_setuservalue},
+  {"setglobalhook", db_setglobalhook},
   {"sethook", db_sethook},
   {"setlocal", db_setlocal},
   {"setmetatable", db_setmetatable},
diff --git a/ldebug.c b/ldebug.c
index eaac16f7..b8a0ecbd 100644
--- a/ldebug.c
+++ b/ldebug.c
@@ -163,6 +163,21 @@ LUA_API int lua_gethookcount (lua_State *L) {
 }
 
 
+/*
+** This function can be called asynchronously (e.g. during a signal),
+** under "reasonable" assumptions. We assume that pointers are atomic (e.g.,
+** gcc ensures that for all platforms where it runs).
+*/
+LUA_API void lua_setglobalhook (lua_State *L, lua_Hook func) {
+  G(L)->globalhook = func;
+}
+
+
+LUA_API lua_Hook lua_getglobalhook (lua_State *L) {
+  return G(L)->globalhook;
+}
+
+
 LUA_API int lua_getstack (lua_State *L, int level, lua_Debug *ar) {
   int status;
   CallInfo *ci;
@@ -795,9 +810,10 @@ static int changedline (const Proto *p, int oldpc, int newpc) {
 
 int luaG_traceexec (lua_State *L, const Instruction *pc) {
   CallInfo *ci = L->ci;
+  lua_Hook globalhook = G(L)->globalhook;
   lu_byte mask = L->hookmask;
   int counthook;
-  if (!(mask & (LUA_MASKLINE | LUA_MASKCOUNT))) {  /* no hooks? */
+  if (!globalhook && !(mask & (LUA_MASKLINE | LUA_MASKCOUNT))) {  /* no hooks? */
     ci->u.l.trap = 0;  /* don't need to stop again */
     return 0;  /* turn off 'trap' */
   }
@@ -806,16 +822,17 @@ int luaG_traceexec (lua_State *L, const Instruction *pc) {
   counthook = (--L->hookcount == 0 && (mask & LUA_MASKCOUNT));
   if (counthook)
     resethookcount(L);  /* reset count */
-  else if (!(mask & LUA_MASKLINE))
-    return 1;  /* no line hook and count != 0; nothing to be done now */
+  else if (!globalhook && !(mask & LUA_MASKLINE))
+    return 1;  /* no global or line hook and count != 0; nothing to be done now */
   if (ci->callstatus & CIST_HOOKYIELD) {  /* called hook last time? */
     ci->callstatus &= ~CIST_HOOKYIELD;  /* erase mark */
     return 1;  /* do not call hook again (VM yielded, so it did not move) */
   }
   if (!isIT(*(ci->u.l.savedpc - 1)))
     L->top = ci->top;  /* prepare top */
+  luaD_hook(L, globalhook, LUA_HOOKCOUNT, -1, 0, 0);  /* call global hook */
   if (counthook)
-    luaD_hook(L, LUA_HOOKCOUNT, -1, 0, 0);  /* call count hook */
+    luaD_hook(L, L->hook, LUA_HOOKCOUNT, -1, 0, 0);  /* call count hook */
   if (mask & LUA_MASKLINE) {
     const Proto *p = ci_func(ci)->p;
     int npci = pcRel(pc, p);
@@ -823,7 +840,7 @@ int luaG_traceexec (lua_State *L, const Instruction *pc) {
         pc <= L->oldpc ||  /* when jump back (loop), or when */
         changedline(p, pcRel(L->oldpc, p), npci)) {  /* enter new line */
       int newline = luaG_getfuncline(p, npci);
-      luaD_hook(L, LUA_HOOKLINE, newline, 0, 0);  /* call line hook */
+      luaD_hook(L, L->hook, LUA_HOOKLINE, newline, 0, 0);  /* call line hook */
     }
     L->oldpc = pc;  /* 'pc' of last call to line hook */
   }
diff --git a/ldo.c b/ldo.c
index 64fe2915..c4e406cf 100644
--- a/ldo.c
+++ b/ldo.c
@@ -272,9 +272,8 @@ void luaD_inctop (lua_State *L) {
 ** called. (Both 'L->hook' and 'L->hookmask', which trigger this
 ** function, can be changed asynchronously by signals.)
 */
-void luaD_hook (lua_State *L, int event, int line,
+void luaD_hook (lua_State *L, lua_Hook hook, int event, int line,
                               int ftransfer, int ntransfer) {
-  lua_Hook hook = L->hook;
   if (hook && L->allowhook) {  /* make sure there is a hook */
     int mask = CIST_HOOKED;
     CallInfo *ci = L->ci;
@@ -312,14 +311,17 @@ void luaD_hook (lua_State *L, int event, int line,
 ** active.
 */
 void luaD_hookcall (lua_State *L, CallInfo *ci) {
+  lua_Hook globalhook = G(L)->globalhook;
   int hook = (ci->callstatus & CIST_TAIL) ? LUA_HOOKTAILCALL : LUA_HOOKCALL;
   Proto *p;
-  if (!(L->hookmask & LUA_MASKCALL))  /* some other hook? */
+  if (!globalhook && !(L->hookmask & LUA_MASKCALL))  /* some other hook? */
     return;  /* don't call hook */
   p = clLvalue(s2v(ci->func))->p;
   L->top = ci->top;  /* prepare top */
   ci->u.l.savedpc++;  /* hooks assume 'pc' is already incremented */
-  luaD_hook(L, hook, -1, 1, p->numparams);
+  luaD_hook(L, globalhook, hook, -1, 1, p->numparams);
+  if (L->hookmask & LUA_MASKCALL)
+    luaD_hook(L, L->hook, hook, -1, 1, p->numparams);
   ci->u.l.savedpc--;  /* correct 'pc' */
 }
 
@@ -327,6 +329,7 @@ void luaD_hookcall (lua_State *L, CallInfo *ci) {
 static StkId rethook (lua_State *L, CallInfo *ci, StkId firstres, int nres) {
   ptrdiff_t oldtop = savestack(L, L->top);  /* hook may change top */
   int delta = 0;
+  lua_Hook globalhook = G(L)->globalhook;
   if (isLuacode(ci)) {
     Proto *p = clLvalue(s2v(ci->func))->p;
     if (p->is_vararg)
@@ -334,11 +337,13 @@ static StkId rethook (lua_State *L, CallInfo *ci, StkId firstres, int nres) {
     if (L->top < ci->top)
       L->top = ci->top;  /* correct top to run hook */
   }
-  if (L->hookmask & LUA_MASKRET) {  /* is return hook on? */
+  if (globalhook || (L->hookmask & LUA_MASKRET)) {  /* is return hook on? */
     int ftransfer;
     ci->func += delta;  /* if vararg, back to virtual 'func' */
     ftransfer = cast(unsigned short, firstres - ci->func);
-    luaD_hook(L, LUA_HOOKRET, -1, ftransfer, nres);  /* call it */
+    luaD_hook(L, globalhook, LUA_HOOKRET, -1, ftransfer, nres);  /* call it */
+    if (L->hookmask & LUA_MASKRET)
+      luaD_hook(L, L->hook, LUA_HOOKRET, -1, ftransfer, nres);  /* call it */
     ci->func -= delta;
   }
   if (isLua(ci->previous))
@@ -467,6 +472,7 @@ void luaD_call (lua_State *L, StkId func, int nresults) {
      Cfunc: {
       int n;  /* number of returns */
       CallInfo *ci;
+      lua_Hook globalhook = G(L)->globalhook;
       checkstackp(L, LUA_MINSTACK, func);  /* ensure minimum stack size */
       ci = next_ci(L);
       ci->nresults = nresults;
@@ -474,9 +480,11 @@ void luaD_call (lua_State *L, StkId func, int nresults) {
       ci->top = L->top + LUA_MINSTACK;
       ci->func = func;
       lua_assert(ci->top <= L->stack_last);
-      if (L->hookmask & LUA_MASKCALL) {
+      if (globalhook || (L->hookmask & LUA_MASKCALL)) {
         int narg = cast_int(L->top - func) - 1;
-        luaD_hook(L, LUA_HOOKCALL, -1, 1, narg);
+        luaD_hook(L, globalhook, LUA_HOOKCALL, -1, 1, narg);
+        if (L->hookmask & LUA_MASKCALL)
+          luaD_hook(L, L->hook, LUA_HOOKCALL, -1, 1, narg);
       }
       lua_unlock(L);
       n = (*f)(L);  /* do the actual call */
diff --git a/ldo.h b/ldo.h
index 7760f853..727bb9ad 100644
--- a/ldo.h
+++ b/ldo.h
@@ -53,7 +53,7 @@ typedef void (*Pfunc) (lua_State *L, void *ud);
 LUAI_FUNC void luaD_seterrorobj (lua_State *L, int errcode, StkId oldtop);
 LUAI_FUNC int luaD_protectedparser (lua_State *L, ZIO *z, const char *name,
                                                   const char *mode);
-LUAI_FUNC void luaD_hook (lua_State *L, int event, int line,
+LUAI_FUNC void luaD_hook (lua_State *L, lua_Hook hook, int event, int line,
                                         int fTransfer, int nTransfer);
 LUAI_FUNC void luaD_hookcall (lua_State *L, CallInfo *ci);
 LUAI_FUNC void luaD_pretailcall (lua_State *L, CallInfo *ci, StkId func, int n);
diff --git a/lstate.c b/lstate.c
index 8fba70d7..09bd6c6b 100644
--- a/lstate.c
+++ b/lstate.c
@@ -419,6 +419,7 @@ LUA_API lua_State *lua_newstate (lua_Alloc f, void *ud) {
   g->gcstepsize = LUAI_GCSTEPSIZE;
   setgcparam(g->genmajormul, LUAI_GENMAJORMUL);
   g->genminormul = LUAI_GENMINORMUL;
+  g->globalhook = NULL;
   for (i=0; i < LUA_NUMTAGS; i++) g->mt[i] = NULL;
   if (luaD_rawrunprotected(L, f_luaopen, NULL) != LUA_OK) {
     /* memory allocation error: free partial state */
diff --git a/lstate.h b/lstate.h
index df9148eb..f49d4dc8 100644
--- a/lstate.h
+++ b/lstate.h
@@ -272,6 +272,7 @@ typedef struct global_State {
   lua_WarnFunction warnf;  /* warning function */
   void *ud_warn;         /* auxiliary data to 'warnf' */
   unsigned int Cstacklimit;  /* current limit for the C stack */
+  volatile lua_Hook globalhook;
 } global_State;
 
 
diff --git a/lua.c b/lua.c
index 18f53c30..2269a830 100644
--- a/lua.c
+++ b/lua.c
@@ -42,7 +42,7 @@ static const char *progname = LUA_PROGNAME;
 */
 static void lstop (lua_State *L, lua_Debug *ar) {
   (void)ar;  /* unused arg. */
-  lua_sethook(L, NULL, 0, 0);  /* reset hook */
+  lua_setglobalhook(L, NULL);  /* reset hook */
   luaL_error(L, "interrupted!");
 }
 
@@ -55,7 +55,7 @@ static void lstop (lua_State *L, lua_Debug *ar) {
 */
 static void laction (int i) {
   signal(i, SIG_DFL); /* if another SIGINT happens, terminate process */
-  lua_sethook(globalL, lstop, LUA_MASKCALL | LUA_MASKRET | LUA_MASKCOUNT, 1);
+  lua_setglobalhook(globalL, lstop);
 }
 
 
diff --git a/lua.h b/lua.h
index b348c147..8c0c4ea8 100644
--- a/lua.h
+++ b/lua.h
@@ -463,6 +463,8 @@ LUA_API void (lua_sethook) (lua_State *L, lua_Hook func, int mask, int count);
 LUA_API lua_Hook (lua_gethook) (lua_State *L);
 LUA_API int (lua_gethookmask) (lua_State *L);
 LUA_API int (lua_gethookcount) (lua_State *L);
+LUA_API void (lua_setglobalhook) (lua_State *L, lua_Hook func);
+LUA_API lua_Hook (lua_getglobalhook) (lua_State *L);
 
 LUA_API int (lua_setcstacklimit) (lua_State *L, unsigned int limit);
 
diff --git a/lvm.c b/lvm.c
index e7781dbf..37e83e2b 100644
--- a/lvm.c
+++ b/lvm.c
@@ -1044,7 +1044,7 @@ void luaV_finishOp (lua_State *L) {
 
 
 
-#define updatetrap(ci)  (trap = ci->u.l.trap)
+#define updatetrap(ci)  (trap = ci->u.l.trap || G(L)->globalhook)
 
 #define updatebase(ci)	(base = ci->func + 1)
 
@@ -1134,7 +1134,7 @@ void luaV_execute (lua_State *L, CallInfo *ci) {
 #include "ljumptab.h"
 #endif
  tailcall:
-  trap = L->hookmask;
+  trap = L->hookmask || G(L)->globalhook;
   cl = clLvalue(s2v(ci->func));
   k = cl->p->k;
   pc = ci->u.l.savedpc;
diff --git a/manual/manual.of b/manual/manual.of
index 9eeb94aa..4815698e 100644
--- a/manual/manual.of
+++ b/manual/manual.of
@@ -4692,6 +4692,13 @@ this value is always equal to @id{nparams}.)
 
 }
 
+@APIEntry{lua_Hook lua_getglobalhook (lua_State *L);|
+@apii{0,0,-}
+
+Returns the current global hook function.
+
+}
+
 @APIEntry{lua_Hook lua_gethook (lua_State *L);|
 @apii{0,0,-}
 
@@ -4892,6 +4899,30 @@ its equivalent in the standard library.
 
 }
 
+@APIEntry{void lua_setglobalhook (lua_State *L, lua_Hook f);|
+@apii{0,0,-}
+
+Similar to @Lid{lua_sethook}, but with the following differences:
+@itemize{
+
+@item{This hook is global to all threads of the state.
+}
+
+@item{The @id{mask} for the global hook is always this:
+@verbatim{
+LUA_MASKCALL | LUA_MASKRET | LUA_MASKCOUNT
+}
+}
+
+@item{The @id{count} for the global hook is always 1.
+}
+
+}
+
+Hooks are disabled by setting @id{f} to @id{NULL}.
+
+}
+
 @APIEntry{void lua_sethook (lua_State *L, lua_Hook f, int mask, int count);|
 @apii{0,0,-}
 
@@ -8547,6 +8578,15 @@ within any function and so have no direct access to local variables.
 
 }
 
+@LibEntry{debug.getglobalhook ()|
+
+Returns the current global hook function,
+as set by the @Lid{debug.setglobalhook} function.
+
+Returns @fail if there is no active global hook.
+
+}
+
 @LibEntry{debug.gethook ([thread])|
 
 Returns the current hook settings of the thread, as three values:
@@ -8692,6 +8732,27 @@ the call returns the old limit.
 
 }
 
+@LibEntry{debug.setglobalhook (hook)|
+
+Sets the given function as the global debug hook.
+The hook will be called every time Lua calls or returns from a function,
+as well as after every instruction.
+
+When called without arguments,
+@Lid{debug.setglobalhook} turns off the global hook.
+
+When the hook is called, its first parameter is a string
+describing the event that has triggered its call:
+@T{"call"}, @T{"tail call"}, @T{"return"},
+and @T{"count"}.
+Inside a hook,
+you can call @id{getinfo} with @N{level 2} to get more information about
+the running function.
+(@N{Level 0} is the @id{getinfo} function,
+and @N{level 1} is the hook function.)
+
+}
+
 @LibEntry{debug.sethook ([thread,] hook, mask [, count])|
 
 Sets the given function as the debug hook.
-- 
2.25.1