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

Thu Nov 19 01:02:41 PST 2020

labath added a comment.

Continuing the discussion from the inline comment:

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

Right. In fact even if it had exceptions enabled, that still wouldn't help as llvm itself is also compiled without exceptions (it can be, but it's an exception :P).

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

I'm not sure what you mean by that. Can you elaborate?

The way I see it, the problem is not about which environment is the c++ code running in, as it can't throw an exception (in llvm). The problem is what happens when we call lua from c++, and the lua code throws. And this is a problem regardless of the environment the C(++) code is in -- the difference is "only" in whether lua will panic or jump over the C code, but something bad will happen anyway.

I think that could be solved by ensuring we always call lua in a protected environment. That way lua will never unwind through the C code (regardless of what environment it is in) because it would always stop right at the boundary.

That would essentially mean, lua_call is banned (except maybe as an optimization, if you can be sure that the code you're about to call will not throw), and all C->lua transitions should go through lua_pcall. And we can provide a wrapper that handles catches those errors and converts them to llvm::Error/Expected, similar to how the python things do it.

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

I'm afraid that won't work, because exceptions are not allowed in llvm. But I think something similar to what I propose above should do the trick.

================
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);
----------------
tammela wrote:
> 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
I'll move this out-of-line, as it's getting rather long.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91508