[Lldb-commits] [PATCH] D71801: [lldb/Lua] Make lldb.debugger et al available to Lua

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: jingham.
labath added a comment.

+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 <https://docs.python.org/3/c-api/init.html#c.PyInterpreterState_New> and friends).



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:46
+llvm::Error Lua::LeaveSession() {
+  m_session_is_active = false;
+  std::string buffer = "lldb.debugger = nil";
----------------
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
```


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:36
+  ~IOHandlerLuaInterpreter() {
+    llvm::cantFail(m_script_interpreter.GetLua().LeaveSession());
+  }
----------------
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...


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

https://reviews.llvm.org/D71801