[lldb/Lua] Make lldb.debugger et al available to Lua

[JDevlieghere]

Summary

The Python script interpreter makes the current debugger, target, process, thread and frame available to interactive scripting sessions through convenience variables. This patch does the same for Lua.

[JDevlieghere]

Test

[labath]

+Jim, for his thoughts on debugger+interpreter relationship

I think this is the time to step back and discuss the relationship between debugger and script interpreter contexts...

So, the way I understand the python code, our intention really was to have each (SB)Debugger be independently scriptable, but achieving this with python was hard, as the python state is very global. That's why the python script interpreter needs to jump through a lot of hoops in order to make the Debuggers *appear* to be independent. Here, you're setting yourself up to do the same with lua. That is not completely unreasonable (it's consistent, at the very least), but:
a) it may not be possible if lua is not sufficiently flexible to enable faking independent global variables

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> foobar = 47
>>> foobar
47
>>> d = lldb.SBDebugger.Create()
>>> d.HandleCommand("script foobar = 42")
>>> foobar
47
>>> d.HandleCommand("script foobar"))))))
42

In particular, a lua context (AFAIK) is completely single-threaded so two debugger objects would never be able to run the lua interpreter concurrently.

b) It is unnecessary, because lua is perfectly capable of creating completely independent contexts.

For these reasons, I'd like to explore the possibility of just creating distinct lua contexts for each (SB)Debugger object. I think we could drop a lot of complexity this way (the weird session_is_active "locking" is just the tip of the iceberg). Doing that will probably require a bit of refactoring, as right now the assumption is that each ScriptInterpreter instance is global, but I don't think that should be too hard (for python we could have a per-debugger shin, backed by a global object).

It may turn out that this is a dead-end, because the lua context will be "too independent", but I'd be sad if we didn't even try that. In particular, if this pans out and we think that's a good design, I think we could do some work to cleanup/simplify python as a result (PyInterpreterState_New and friends).

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp

45

This m_session_is_active business is very confusing, and probably incorrect.
Imagine the following sequence:

A: EnterSession() # sets m_session_is_active to true
B: EnterSession() # noop
B: LeaveSession() # sets m_session_is_active to false
B: EnterSession() # sets m_session_is_active to true, when it probably shouldn't

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp

36

Does this mean that executing something like script lldb = nil will cause a crash? I don't think we necessarily need to protect against that, but I thought it's worth mentioning that...

[JDevlieghere]

Thanks Pavel, I must admit I gave this little thought and just mirrored the Python approach. I *think* it should easy to implement things the way you describe (famous last words).

[JDevlieghere]

• Keep one set of convenience variables per lua state/script interpreter
• Add test case

[jingham]

In D71801#1794758, @labath wrote:

+Jim, for his thoughts on debugger+interpreter relationship

I think this is the time to step back and discuss the relationship between debugger and script interpreter contexts...

I actually have no strong opinion about HOW this should be implemented. Before starting on lldb, I had mostly used Tcl when I was incorporating a scripting language into various tools. In Tcl, it was natural to make separate independent interpreters. For instance, the security model for Tcl involved making a secure interpreter that forwarded messages to a shadow - insecure - interpreter that would implement whatever sandboxing you wanted to do. Tcl interpreters were independent entities with no shared state. That was the model I was working on when we chose to use Python for lldb, but that's not really how Python thinks of interpreters, and as you note, that caused us a bunch of headaches.

I do feel pretty strongly that to the user, every SBDebugger should have a separate interpreter state. That really has to be true. In Xcode, each SBDebugger represents a separate project that the user is debugging, and these often have no relation to one another. Doing something in one script interpreter and having that affect what should be an entirely independent debugging session would be bad. Since it is not uncommon for data formatters and Python command modules to use some global state, this could lead to some really frustrating bugs.

It isn't surprising that we have to do different things for each interpreter. For instance, in Tcl you'd just create a new interpreter and you'd be done.

So my take is that being able to support multiple independent interpreters is a minimum requirement for supporting a scripting language in lldb. If it can't do that, then we shouldn't try to support it.

I don't know enough about Lua to know whether it passes this (fairly low) bar...

So, the way I understand the python code, our intention really was to have each (SB)Debugger be independently scriptable, but achieving this with python was hard, as the python state is very global. That's why the python script interpreter needs to jump through a lot of hoops in order to make the Debuggers *appear* to be independent. Here, you're setting yourself up to do the same with lua. That is not completely unreasonable (it's consistent, at the very least), but:
a) it may not be possible if lua is not sufficiently flexible to enable faking independent global variables

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> foobar = 47
>>> foobar
47
>>> d = lldb.SBDebugger.Create()
>>> d.HandleCommand("script foobar = 42")
>>> foobar
47
>>> d.HandleCommand("script foobar"))))))
42

In particular, a lua context (AFAIK) is completely single-threaded so two debugger objects would never be able to run the lua interpreter concurrently.

b) It is unnecessary, because lua is perfectly capable of creating completely independent contexts.

For these reasons, I'd like to explore the possibility of just creating distinct lua contexts for each (SB)Debugger object. I think we could drop a lot of complexity this way (the weird session_is_active "locking" is just the tip of the iceberg). Doing that will probably require a bit of refactoring, as right now the assumption is that each ScriptInterpreter instance is global, but I don't think that should be too hard (for python we could have a per-debugger shin, backed by a global object).

It may turn out that this is a dead-end, because the lua context will be "too independent", but I'd be sad if we didn't even try that. In particular, if this pans out and we think that's a good design, I think we could do some work to cleanup/simplify python as a result (PyInterpreterState_New and friends).

[labath]

In D71801#1807109, @jingham wrote:

In D71801#1794758, @labath wrote:

+Jim, for his thoughts on debugger+interpreter relationship

I think this is the time to step back and discuss the relationship between debugger and script interpreter contexts...

I actually have no strong opinion about HOW this should be implemented. Before starting on lldb, I had mostly used Tcl when I was incorporating a scripting language into various tools. In Tcl, it was natural to make separate independent interpreters. For instance, the security model for Tcl involved making a secure interpreter that forwarded messages to a shadow - insecure - interpreter that would implement whatever sandboxing you wanted to do. Tcl interpreters were independent entities with no shared state. That was the model I was working on when we chose to use Python for lldb, but that's not really how Python thinks of interpreters, and as you note, that caused us a bunch of headaches.

I do feel pretty strongly that to the user, every SBDebugger should have a separate interpreter state. That really has to be true. In Xcode, each SBDebugger represents a separate project that the user is debugging, and these often have no relation to one another. Doing something in one script interpreter and having that affect what should be an entirely independent debugging session would be bad. Since it is not uncommon for data formatters and Python command modules to use some global state, this could lead to some really frustrating bugs.

It isn't surprising that we have to do different things for each interpreter. For instance, in Tcl you'd just create a new interpreter and you'd be done.

So my take is that being able to support multiple independent interpreters is a minimum requirement for supporting a scripting language in lldb. If it can't do that, then we shouldn't try to support it.

I don't know enough about Lua to know whether it passes this (fairly low) bar...

Thanks for your thoughts, Jim. I'm pretty sure that independent interpreters are possible in lua, and this patch does achieve that. Actually, even the previous version achieved that, but I somehow got confused there -- for some reason I thought that ScriptInterpreter objects are shared between Debuggers, but that is clearly not the case -- and the test that Jonas added demonstrates that.

As for the patch itself, I'm afraid that this version has simplified things too much. :D We can't set the thread (etc.) convenience variables during initialization, as these can change over time. Some EnterSession/LeaveSession logic will still be needed. I guess we will also need something like the m_session_is_active "locking" logic, but for a slightly different reason than I originally thought -- not to protect concurrent sessions in different debuggers, but to handle nested sessions in the same debugger/scriptinterpreter object:

lldb) file /bin/ls
Current executable set to '/bin/ls' (x86_64).
(lldb) file /bin/cat
Current executable set to '/bin/cat' (x86_64).
(lldb) script
>>> print lldb.target, lldb.debugger.GetSelectedTarget()
cat cat
>>> lldb.debugger.SetSelectedTarget(lldb.debugger.GetTargetAtIndex(0))
>>> print lldb.target, lldb.debugger.GetSelectedTarget()
cat ls
>>> lldb.debugger.HandleCommand("script print lldb.target, lldb.debugger.GetSelectedTarget()")
cat ls
>>> print lldb.target, lldb.debugger.GetSelectedTarget()
cat ls
>>> ^D
(lldb) script
>>> print lldb.target, lldb.debugger.GetSelectedTarget()
ls ls

The current python behavior is for nested sessions to not alter the convenience variable values (they preserve the value of the outermost session), so I guess we ought to emulate that (though I think a case could also be made for changing their values on every session entry, as long as the original values are restored when the session terminates).

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp

30–38

I'd put this function into ScriptInterpreterLua. I'd try keep the Lua class relatively generic/low-level, where this thing is all about the specifics of the SB api.

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp

52

I don't think this is right. You should be able to set lldb.debugger once and for all during initialization, but the rest of the variables (thread, process, target) still need to be set upon entering the lua script as these can vary during the lifetime of an Debugger.

[JDevlieghere]

• Address feedback Pavel
• Add test for nested session

[JDevlieghere]

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp

52

I prefer setting/unsetting them together. This also ensures you don't have access to lldb.debugger outside of an interactive session.

[labath]

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp

52

(Un)setting them together is fine, but i don't see why you need to enter a session in the ScriptInterpreterLua constructor now that it is done in IOHandlerLuaInterpreter. I think this part should be deleted.

lldb/test/Shell/ScriptInterpreter/Lua/nested_sessions.test

7–17

Would it be possible to group these by two or otherwise give some context to each line (e.g. inlining nested_sessions.in into this file would help). This way, the checks are very opaque...

[JDevlieghere]

Feedback Pavel

[labath]

Thanks for your patience. Looks fine to me, just remove the dead code..

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp

29–38

This should be dead code now, right?

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h

 //===-- ScriptInterpreterLua.h ----------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_Lua_h_
 #define liblldb_Lua_h_
 
+#include "lldb/lldb-types.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 
 #include "lua.hpp"
 
 #include <mutex>
 
 namespace lldb_private {
Lua() : m_lua_state(luaL_newstate()) {
     luaopen_lldb(m_lua_state);
   }
 
   ~Lua() {
     assert(m_lua_state);
     luaL_openlibs(m_lua_state);
   }
 
+  llvm::Error EnterSession(lldb::user_id_t debugger_id);
   llvm::Error Run(llvm::StringRef buffer);
 
 private:
-  std::mutex m_mutex;
   lua_State *m_lua_state;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_Lua_h_

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp

 //===-- Lua.cpp -----------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Lua.h"
 #include "llvm/Support/FormatVariadic.h"
 
 using namespace lldb_private;
+using namespace lldb;
 
 llvm::Error Lua::Run(llvm::StringRef buffer) {
-  std::lock_guard<std::mutex> lock(m_mutex);
   int error =
       luaL_loadbuffer(m_lua_state, buffer.data(), buffer.size(), "buffer") ||
       lua_pcall(m_lua_state, 0, 0, 0);
   if (!error)
     return llvm::Error::success();
 
   llvm::Error e = llvm::make_error<llvm::StringError>(
       llvm::formatv("{0}\n", lua_tostring(m_lua_state, -1)),
       llvm::inconvertibleErrorCode());
   // Pop error message from the stack.
   lua_pop(m_lua_state, 1);
   return e;
 }
+
+llvm::Error Lua::EnterSession(user_id_t debugger_id) {
+  const char *fmt_str =
+      "lldb.debugger = lldb.SBDebugger.FindDebuggerWithID({0}); "
+      "lldb.target = lldb.debugger:GetSelectedTarget(); "
+      "lldb.process = lldb.target:GetProcess(); "
+      "lldb.thread = lldb.process:GetSelectedThread(); "
+      "lldb.frame = lldb.thread:GetSelectedFrame()";
+  return Run(llvm::formatv(fmt_str, debugger_id).str());
+}

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h

public:
 
   // PluginInterface protocol
   lldb_private::ConstString GetPluginName() override;
 
   uint32_t GetPluginVersion() override;
 
   Lua &GetLua();
 
+  llvm::Error EnterSession(lldb::user_id_t debugger_id);
+  llvm::Error LeaveSession();
+
 private:
   std::unique_ptr<Lua> m_lua;
+  bool m_session_is_active = false;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_ScriptInterpreterLua_h_

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp

 class IOHandlerLuaInterpreter : public IOHandlerDelegate,
                                 public IOHandlerEditline {
 public:
   IOHandlerLuaInterpreter(Debugger &debugger,
                           ScriptInterpreterLua &script_interpreter)
       : IOHandlerEditline(debugger, IOHandler::Type::LuaInterpreter, "lua",
                           ">>> ", "..> ", true, debugger.GetUseColor(), 0,
                           *this, nullptr),
-        m_script_interpreter(script_interpreter) {}
+        m_script_interpreter(script_interpreter) {
+    llvm::cantFail(m_script_interpreter.EnterSession(debugger.GetID()));
+  }
+
+  ~IOHandlerLuaInterpreter() {
+    llvm::cantFail(m_script_interpreter.LeaveSession());
+  }
 
   void IOHandlerInputComplete(IOHandler &io_handler,
                               std::string &data) override {
     if (llvm::Error error = m_script_interpreter.GetLua().Run(data)) {
       *GetOutputStreamFileSP() << llvm::toString(std::move(error));
     }
   }
 
 private:
   ScriptInterpreterLua &m_script_interpreter;
 };
 
 ScriptInterpreterLua::ScriptInterpreterLua(Debugger &debugger)
     : ScriptInterpreter(debugger, eScriptLanguageLua),
-      m_lua(std::make_unique<Lua>()) {}
+      m_lua(std::make_unique<Lua>()) {
+  llvm::cantFail(GetLua().EnterSession(debugger.GetID()));
+}
 
 ScriptInterpreterLua::~ScriptInterpreterLua() {}
 
 bool ScriptInterpreterLua::ExecuteOneLine(llvm::StringRef command,
                                           CommandReturnObject *result,
                                           const ExecuteScriptOptions &options) {
   if (llvm::Error e = m_lua->Run(command)) {
     result->AppendErrorWithFormatv(
llvm::call_once(g_once_flag, []() {
     PluginManager::RegisterPlugin(GetPluginNameStatic(),
                                   GetPluginDescriptionStatic(),
                                   lldb::eScriptLanguageLua, CreateInstance);
   });
 }
 
 void ScriptInterpreterLua::Terminate() {}
 
+llvm::Error ScriptInterpreterLua::EnterSession(user_id_t debugger_id) {
+  if (m_session_is_active)
+    return llvm::Error::success();
+
+  const char *fmt_str =
+      "lldb.debugger = lldb.SBDebugger.FindDebuggerWithID({0}); "
+      "lldb.target = lldb.debugger:GetSelectedTarget(); "
+      "lldb.process = lldb.target:GetProcess(); "
+      "lldb.thread = lldb.process:GetSelectedThread(); "
+      "lldb.frame = lldb.thread:GetSelectedFrame()";
+  return m_lua->Run(llvm::formatv(fmt_str, debugger_id).str());
+}
+
+llvm::Error ScriptInterpreterLua::LeaveSession() {
+  if (!m_session_is_active)
+    return llvm::Error::success();
+
+  m_session_is_active = false;
+
+  llvm::StringRef str = "lldb.debugger = nil; "
+                        "lldb.target = nil; "
+                        "lldb.process = nil; "
+                        "lldb.thread = nil; "
+                        "lldb.frame = nil";
+  return m_lua->Run(str);
+}
+
 lldb::ScriptInterpreterSP
 ScriptInterpreterLua::CreateInstance(Debugger &debugger) {
   return std::make_shared<ScriptInterpreterLua>(debugger);
 }
 
 lldb_private::ConstString ScriptInterpreterLua::GetPluginNameStatic() {
   static ConstString g_name("script-lua");
   return g_name;

lldb/test/Shell/ScriptInterpreter/Lua/Inputs/independent_state.in

+script foobar = 40 + 7
+script print(foobar)
+script d = lldb.SBDebugger.Create()
+script d:HandleCommand("script foobar = 40 + 2")
+script print(foobar)
+script d:HandleCommand("script print(foobar)")

lldb/test/Shell/ScriptInterpreter/Lua/Inputs/nested_sessions.in

+script
+print(lldb.target); print(lldb.debugger:GetSelectedTarget())
+lldb.debugger:SetSelectedTarget(lldb.debugger:GetTargetAtIndex(0))
+print(lldb.target); print(lldb.debugger:GetSelectedTarget())
+lldb.debugger:HandleCommand("script print(lldb.target); print(lldb.debugger:GetSelectedTarget())")
+print(lldb.target); print(lldb.debugger:GetSelectedTarget())

lldb/test/Shell/ScriptInterpreter/Lua/Inputs/nested_sessions_2.in

+script
+print(lldb.target); print(lldb.debugger:GetSelectedTarget())

lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test

+# REQUIRES: lua
+#
+# This tests that the convenience variables are not nil. Given that there is no
+# target we only expect the debugger to be valid.
+#
+# RUN: cat %s | %lldb --script-language lua 2>&1 | FileCheck %s
+script
+print(string.format("lldb.debugger is valid: %s", lldb.debugger:IsValid()))
+print(string.format("lldb.target is valid: %s", lldb.target:IsValid()))
+print(string.format("lldb.process is valid: %s", lldb.process:IsValid()))
+print(string.format("lldb.thread is valid: %s", lldb.thread:IsValid()))
+print(string.format("lldb.frame is valid: %s", lldb.frame:IsValid()))
+# CHECK: debugger is valid: true
+# CHECK: target is valid: false
+# CHECK: process is valid: false
+# CHECK: thread is valid: false
+# CHECK: frame is valid: false

lldb/test/Shell/ScriptInterpreter/Lua/independent_state.test

+# REQUIRES: lua
+#
+# RUN:  %lldb --script-language lua -s %S/Inputs/independent_state.in 2>&1 | FileCheck %s
+# CHECK: 47
+# CHECK: 47
+# CHECK: 42

lldb/test/Shell/ScriptInterpreter/Lua/nested_sessions.test

+# REQUIRES: lua
+# RUN: mkdir -p %t
+# RUN: echo "int main() { return 0; }" | %clang_host -x c - -o %t/foo
+# RUN: echo "int main() { return 0; }" | %clang_host -x c - -o %t/bar
+# RUN:  %lldb --script-language lua -o "file %t/bar" -o "file %t/foo" -s %S/Inputs/nested_sessions.in  -s %S/Inputs/nested_sessions_2.in 2>&1 | FileCheck %s
+# CHECK: script
+# CHECK-NEXT: foo
+# CHECK-NEXT: foo
+# CHECK-NEXT: foo
+# CHECK-NEXT: bar
+# CHECK-NEXT: foo
+# CHECK-NEXT: bar
+# CHECK-NEXT: foo
+# CHECK-NEXT: bar
+# CHECK: script
+# CHECK-NEXT: bar
+# CHECK-NEXT: bar