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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think we're converging on something. Now to iron out the details...

In D91508#2413789 <https://reviews.llvm.org/D91508#2413789>, @tammela wrote:

>> I am puzzled by all the wrapping that's happening inside the `PushSBClass` functions. What is that protecting us from? I would hope that pushing a swig wrapper on the stack is a safe operation...
>
> I thought that too, but internally it's a naked call to `lua_newuserdata()` which might throw in case of a memory error.
>
>> So, IIUC, this can only fail if we are running out of memory? If that's the case, then I would remove these checks, as (for better or worse) llvm is not robust against memory allocation errors, and they add a fair amount of cruft to the code.
>
> Fair enough. Will remove those.
> Since this seems to be a fact of life for LLVM, perhaps wrapping potential memory errors turns out to be just bloat. If that's the case, then the wrapping in `PushSBClass` is not needed and the `abort()` call that Lua does is honest.

Right. I'm not worried about OOM errors (well.. I am slightly, but I don't think it's worthwhile to do something about them without addressing the bigger problem).



================
Comment at: lldb/bindings/lua/lua-wrapper.swig:21-22
+{
+   lldb::SBFrame sb_frame(stop_frame_sp);
+   lldb::SBBreakpointLocation sb_bp_loc(bp_loc_sp);
+
----------------
What's up with the copying? Is it to ensure the callback does not modify the caller's values? If so, you could just drop the `&` from the signature, and have the compiler copy these for you...


================
Comment at: lldb/bindings/lua/lua-wrapper.swig:25-26
+   // Get the Lua callback
+   lua_pushlightuserdata(L, baton);
+   lua_gettable(L, LUA_REGISTRYINDEX);
+
----------------
Is there a reason this function has to be in this file? It would be nice all the baton/registry handling was happening in the same place...


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:92
+    // Pop error message from the stack.
+    lua_pop(m_lua_state, 1);
+    return e;
----------------
Shouldn't this also pop the `baton` ?


================
Comment at: lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test:5
+b main
+breakpoint command add -s lua -o 'print(frame)'
+run
----------------
Could we also have a test where calling the callback raises an error? And for the different values of the "stop" argument that can be returned from the callback? And when compiling the callback expression results in an error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91508