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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
