[Lldb-commits] [PATCH] D91508: [LLDB/Lua] add support for one-liner breakpoint callback

[Pedro Tammela via Phabricator via lldb-commits]

tammela added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:60-78
+static int runCallback(lua_State *L) {
+  LuaCallback *callback = static_cast<LuaCallback *>(lua_touserdata(L, -1));
+  return (*callback)(L);
+}
+
+llvm::Error Lua::Run(LuaCallback &callback) {
+  lua_pushcfunction(m_lua_state, runCallback);
----------------
labath wrote:
> tammela wrote:
> > labath wrote:
> > > I'm confused. Why use lua to call a c callback, when you could just do `calllback()`?
> > Some Lua API functions throw errors, if there's any it will `abort()` the program if no panic handler or protected call is provided.
> > To guarantee that the callback always runs in a protected environment and it has error handling, we do the above.
> > Delegating this to the callback itself makes it cumbersome to write.
> Aha, I see.
> 
> So, if I remember my lua correctly (I wrote a c++ lua wrapper once, but that was years ago..), whenever there's a lua exception inside this (c++) callback, lua will longjmp(3) back to the lua_pcall call on line 68, skipping any destructors that should normally be invoked. Is that so?
> 
> If that's the case, then I think this is a dangerous api, that should at the very least get a big fat warning, but that ideally shouldn't exist at all.
> 
> What's the part that makes delegating this to the callback "cumersome to write"? And why can't that be overcome by a suitable utility function which wraps `lua_pcall` or whatever else is needed?
> 
> The approach that we've chosen in python is to have very little code interacting with the python C API directly. Instead, code generally works with our C++ wrappers defined in `PythonDataObject.h`. These functions try to hide the python exception magic as much as possible, and present a c++-y version of the interface.
> 
> Now, since lua is stack-based, something like LuaDataObjects.h would probably not work. However, that doesn't meant we should give up on the c++ wrapper  idea altogether. During the intitial reviews, my intention was for the `Lua` class to serve this purpose. I still think this can be achieved if we make the callback functions take `Lua&` instead of `lua_State*` as an argument, and then ensure the class contains whatever is needed to make the callbacks not cumerbsome to write.
> So, if I remember my lua correctly (I wrote a c++ lua wrapper once, but that was years ago..), whenever there's a lua exception inside this (c++) callback, lua will longjmp(3) back to the lua_pcall call on line 68, skipping any destructors that should normally be invoked. Is that so?
>
> If that's the case, then I think this is a dangerous api, that should at the very least get a big fat warning, but that ideally shouldn't exist at all.

You are right. This escaped me completely.
Lua can be compiled to use `try/catch` instead of `longjmp`, but that is an exception (no pun intended). Since we rely on the host's library, we need to assume that it's using `longjmp`.

> What's the part that makes delegating this to the callback "cumersome to write"? And why can't that be overcome by a suitable utility function which wraps lua_pcall or whatever else is needed?
>
> The approach that we've chosen in python is to have very little code interacting with the python C API directly. Instead, code generally works with our C++ wrappers defined in PythonDataObject.h. These functions try to hide the python exception magic as much as possible, and present a c++-y version of the interface.
>
> Now, since lua is stack-based, something like LuaDataObjects.h would probably not work. However, that doesn't meant we should give up on the c++ wrapper idea altogether. During the intitial reviews, my intention was for the Lua class to serve this purpose. I still think this can be achieved if we make the callback functions take Lua& instead of lua_State* as an argument, and then ensure the class contains whatever is needed to make the callbacks not cumerbsome to write.

Any C++ code that runs in a `lua_pcall` seems to face this issue. I checked a very complete C++ Lua wrapper called `sol2` and they seem to have the same issue. I don't think there's a way out of it except code discipline.

Lua provides `lua_atpanic` as a way to recover from unprotected throws. Perhaps we can leverage this to throw an exception, guaranteeing stack unwinding and avoiding a call to `abort()`. We would then let the callback run unprotected and delegate any calls to `lua_pcall` to the callback.

Changes proposed:
- Register the panic function on `Lua` ctor
- Let the `Callback` run unprotected
- Wrap the `Callback` call in a `try catch` block
- Change the `Callback` signature to receive `Lua&` and return `llvm::Error`

It looks like it ticks all goals.
- Unprotected Lua errors do not cause lldb to abort.
- Stack unwinding is guaranteed for C++ code interacting with the Lua state.
- Errors are propagated


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91508/new/

https://reviews.llvm.org/D91508