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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/bindings/lua/lua-wrapper.swig:26-27
+
+   SBTypeToSWIGWrapper(L, &sb_frame);
+   SBTypeToSWIGWrapper(L, &sb_bp_loc);
+
----------------
This name made sense for python, as the functions actually returned the wrappers. But here, the name makes it pretty unobvious that these functions actually push the object on the lua stack. It'd be better if this was called something like `PushSBType` or something


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


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:200
+  Debugger &debugger = target->GetDebugger();
+  ScriptInterpreterLua *lua_interpreter =
+      static_cast<ScriptInterpreterLua *>(debugger.GetScriptInterpreter());
----------------
tammela wrote:
> labath wrote:
> > How is `lua_interpreter` different from `this`?
> Are you asking why I didn't go for `m_script_interpreter`?
> 
> This function is static (on the class declaration), perhaps it's clearer a `static` keyword here as well?
I just missed the fact that this is a static function (I hate these static baton functions in lldb). Putting `static` on an out-of-line member function definition is not valid c++.

However, this does raise a different issue. Debugger::GetScriptInterpreter will return the default script interpreter, and that may not be the lua one. I think you should pass eScriptLanguageLua here explicitly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91508