[lldb/Lua] add support for multiline scripted breakpoints

[tammela]

Summary

1 - Partial Statements

The interpreter loop runs every line it receives, so partial
Lua statements are not being handled properly. This is a problem for
multiline breakpoint scripts since the interpreter loop, for this
particular case, is just an abstraction to a partially parsed function
body declaration.

This patch addresses this issue and as a side effect improves the
general Lua interpreter loop as well. It's now possible to write partial
statements in the 'script' command.

Example:

(lldb) script
>>>   do
..>   local a = 123
..>   print(a)
..>   end
123

The technique implemented is the same as the one employed by Lua's own REPL implementation.
Partial statements always errors out with the '<eof>' tag in the error
message.

2 - LoadBuffer in Lua.h

In order to support (1), we need an API for just loading string buffers. This
was not available as so far we only needed to run string buffers.

3 - Multiline scripted breakpoints

Finally, with all the base features implemented this feature is
straightforward. The interpreter loop behaves exactly the same, the
difference is that it will aggregate all Lua statements into the body of
the breakpoint function. An explicit 'quit' statement is needed to exit the
interpreter loop.

Example:

(lldb) breakpoint command add -s lua
Enter your Lua command(s). Type 'quit' to end.
The commands are compiled as the body of the following Lua function
function (frame, bp_loc, ...) end
..> print(456)
..> a = 123
..> quit

[JDevlieghere]

Thanks Pedro, it's really great to see the Lua interpreter getting all these improvements!

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

62

Shouldn't this be the same for break- and watchpoints?

81

The "quit" is something lldb-specific. Even though the logic is very simple, can you move it into a little helper function so that it doesn't get duplicated across IOHandlerIsInputComplete and IOHandlerInputComplete.

[tammela]

Addressing comments

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

62

Probably, we can address this when we add the support for watchpoints later

[labath]

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

39

This default value does not seem particularly useful. In fact, I am not sure if this argument should even exist at all. The usage in IOHandlerIsInputComplete is sufficiently unorthodox that it might be a good idea to draw attention to the fact that something funky is happening via an explicit pop (and maybe a comment).

[tammela]

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

39

We can't explicitly pop, as in calling lua_pop(), from ScriptInterpreterLua.cpp because the lua_State is not exposed. Are you suggesting to add a method that wraps lua_pop() as well?

[labath]

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

39

Good question. I didn't realize this aspect of the problem. I think the answer to this depends on the direction we want the Lua class to go in. Back when it was first introduced, my idea was for it to be some sort of a c++ wrapper for the lua (C) api. It would offset a nicer interface to interact with lua, but would otherwise be (more-or-less) lldb-agnostic (kind of like our PythonDataObjects). In that world, it would make sense to expose the lua stack and the pop function (among other things).

However, I don't think that's really how things have worked out. The current Lua interface is much more high-level and much-more debugger-oriented than I originally imagined. Among functions like RegisterBreakpointCallback and ChangeIO, a function like Pop seems misplaced. I'm not sure this is a good position to be in (there's still a need to find a home for the c++ wrapper-like functions, for example to handle exceptions), but if we want to go down that path, then a pop function is not the right answer. However, I think the same applies to the LoadBuffer function, as it cannot be useful (beyond this syntax-check use case) without exposing the lua stack, implicitly or explicitly. In this world it would be better to make the function even more high-level, and have something like CheckSyntax(buffer) (which does pretty much what LoadBuffer does, but it always pops the result). Internally it can be implemented as a call to LoadBuffer, but this function would now be a private implementation detail instead of a part of the interface.

[tammela]

Addressing comments

[tammela]

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

39

I think it's better to keep it as high level as possible. Exposing the lua_State spills the stack control to whom ever has access to the Lua interpreter, which might be undesirable and cumbersome to keep track of.

[tammela]

ping

[labath]

This looks better. I haven't checked the rest of the patch in detail, but it seems ok at a quick glance and Jonas appeared to be happy with it.

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

public:
   ~Lua();
 
   llvm::Error Run(llvm::StringRef buffer);
   llvm::Error RegisterBreakpointCallback(void *baton, const char *body);
   llvm::Expected<bool>
   CallBreakpointCallback(void *baton, lldb::StackFrameSP stop_frame_sp,
                          lldb::BreakpointLocationSP bp_loc_sp);
   llvm::Error LoadModule(llvm::StringRef filename);
+  llvm::Error CheckSyntax(llvm::StringRef buffer);
   llvm::Error ChangeIO(FILE *out, FILE *err);
 
 private:
   lua_State *m_lua_state;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_Lua_h_

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

 Lua::CallBreakpointCallback(void *baton, lldb::StackFrameSP stop_frame_sp,
                             lldb::BreakpointLocationSP bp_loc_sp) {
   lua_pushlightuserdata(m_lua_state, baton);
   lua_gettable(m_lua_state, LUA_REGISTRYINDEX);
   return LLDBSwigLuaBreakpointCallbackFunction(m_lua_state, stop_frame_sp,
                                                bp_loc_sp);
 }
 
+llvm::Error Lua::CheckSyntax(llvm::StringRef buffer) {
+  int error =
+      luaL_loadbuffer(m_lua_state, buffer.data(), buffer.size(), "buffer");
+  if (error == LUA_OK) {
+    // Pop buffer
+    lua_pop(m_lua_state, 1);
+    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::LoadModule(llvm::StringRef filename) {
   FileSpec file(filename);
   if (!FileSystem::Instance().Exists(file)) {
     return llvm::make_error<llvm::StringError>("invalid path",
                                                llvm::inconvertibleErrorCode());
   }
 
   ConstString module_extension = file.GetFileNameExtension();

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

public:
 
   uint32_t GetPluginVersion() override;
 
   Lua &GetLua();
 
   llvm::Error EnterSession(lldb::user_id_t debugger_id);
   llvm::Error LeaveSession();
 
+  void CollectDataForBreakpointCommandCallback(
+      std::vector<BreakpointOptions *> &bp_options_vec,
+      CommandReturnObject &result) override;
+
   Status SetBreakpointCommandCallback(BreakpointOptions *bp_options,
                                       const char *command_body_text) override;
 
 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

 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StringList.h"
 #include "lldb/Utility/Timer.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FormatAdapters.h"
 #include <memory>
+#include <vector>
 
 using namespace lldb;
 using namespace lldb_private;
 
 LLDB_PLUGIN_DEFINE(ScriptInterpreterLua)
 
+enum ActiveIOHandler {
+  eIOHandlerNone,
+  eIOHandlerBreakpoint,
+  eIOHandlerWatchpoint
+};
+
 class IOHandlerLuaInterpreter : public IOHandlerDelegate,
                                 public IOHandlerEditline {
 public:
   IOHandlerLuaInterpreter(Debugger &debugger,
-                          ScriptInterpreterLua &script_interpreter)
+                          ScriptInterpreterLua &script_interpreter,
+                          ActiveIOHandler active_io_handler = eIOHandlerNone)
       : IOHandlerEditline(debugger, IOHandler::Type::LuaInterpreter, "lua",
                           ">>> ", "..> ", true, debugger.GetUseColor(), 0,
                           *this, nullptr),
-        m_script_interpreter(script_interpreter) {
+        m_script_interpreter(script_interpreter),
+        m_active_io_handler(active_io_handler) {
     llvm::cantFail(m_script_interpreter.GetLua().ChangeIO(
         debugger.GetOutputFile().GetStream(),
         debugger.GetErrorFile().GetStream()));
     llvm::cantFail(m_script_interpreter.EnterSession(debugger.GetID()));
   }
 
   ~IOHandlerLuaInterpreter() override {
     llvm::cantFail(m_script_interpreter.LeaveSession());
   }
 
+  void IOHandlerActivated(IOHandler &io_handler, bool interactive) override {
+    const char *instructions = nullptr;
+    switch (m_active_io_handler) {
+    case eIOHandlerNone:
+    case eIOHandlerWatchpoint:
+      break;
+    case eIOHandlerBreakpoint:
+      instructions = "Enter your Lua command(s). Type 'quit' to end.\n"
+                     "The commands are compiled as the body of the following "
+                     "Lua function\n"
+                     "function (frame, bp_loc, ...) end\n";
+      SetPrompt(llvm::StringRef("..> "));
+      break;
+    }
+    if (instructions == nullptr)
+      return;
+    if (interactive)
+      *io_handler.GetOutputStreamFileSP() << instructions;
+  }
+
+  bool IOHandlerIsInputComplete(IOHandler &io_handler,
+                                StringList &lines) override {
+    size_t last = lines.GetSize() - 1;
+    if (IsQuitCommand(lines.GetStringAtIndex(last))) {
+      if (m_active_io_handler == eIOHandlerBreakpoint)
+        lines.DeleteStringAtIndex(last);
+      return true;
+    }
+    StreamString str;
+    lines.Join("\n", str);
+    if (llvm::Error E =
+            m_script_interpreter.GetLua().CheckSyntax(str.GetString())) {
+      std::string error_str = toString(std::move(E));
+      // Lua always errors out to incomplete code with '<eof>'
+      return error_str.find("<eof>") == std::string::npos;
+    }
+    // The breakpoint handler only exits with a explicit 'quit'
+    return m_active_io_handler != eIOHandlerBreakpoint;
+  }
+
   void IOHandlerInputComplete(IOHandler &io_handler,
                               std::string &data) override {
-    if (llvm::StringRef(data).rtrim() == "quit") {
+    switch (m_active_io_handler) {
+    case eIOHandlerBreakpoint: {
+      auto *bp_options_vec = static_cast<std::vector<BreakpointOptions *> *>(
+          io_handler.GetUserData());
+      for (auto *bp_options : *bp_options_vec) {
+        Status error = m_script_interpreter.SetBreakpointCommandCallback(
+            bp_options, data.c_str());
+        if (error.Fail())
+          *io_handler.GetErrorStreamFileSP() << error.AsCString() << '\n';
+      }
+      io_handler.SetIsDone(true);
+    } break;
+    case eIOHandlerWatchpoint:
+      io_handler.SetIsDone(true);
+      break;
+    case eIOHandlerNone:
+      if (IsQuitCommand(data)) {
         io_handler.SetIsDone(true);
         return;
       }
-
-    if (llvm::Error error = m_script_interpreter.GetLua().Run(data)) {
-      *GetOutputStreamFileSP() << llvm::toString(std::move(error));
+      if (llvm::Error error = m_script_interpreter.GetLua().Run(data))
+        *io_handler.GetErrorStreamFileSP() << toString(std::move(error));
+      break;
     }
   }
 
 private:
   ScriptInterpreterLua &m_script_interpreter;
+  ActiveIOHandler m_active_io_handler;
+
+  bool IsQuitCommand(llvm::StringRef cmd) { return cmd.rtrim() == "quit"; }
 };
 
 ScriptInterpreterLua::ScriptInterpreterLua(Debugger &debugger)
     : ScriptInterpreter(debugger, eScriptLanguageLua),
       m_lua(std::make_unique<Lua>()) {}
 
 ScriptInterpreterLua::~ScriptInterpreterLua() {}
 
bool ScriptInterpreterLua::BreakpointCallbackFunction(
   if (llvm::Error E = BoolOrErr.takeError()) {
     debugger.GetErrorStream() << toString(std::move(E));
     return true;
   }
 
   return *BoolOrErr;
 }
 
+void ScriptInterpreterLua::CollectDataForBreakpointCommandCallback(
+    std::vector<BreakpointOptions *> &bp_options_vec,
+    CommandReturnObject &result) {
+  IOHandlerSP io_handler_sp(
+      new IOHandlerLuaInterpreter(m_debugger, *this, eIOHandlerBreakpoint));
+  io_handler_sp->SetUserData(&bp_options_vec);
+  m_debugger.RunIOHandlerAsync(io_handler_sp);
+}
+
 Status ScriptInterpreterLua::SetBreakpointCommandCallback(
     BreakpointOptions *bp_options, const char *command_body_text) {
   Status error;
   auto data_up = std::make_unique<CommandDataLua>();
   error = m_lua->RegisterBreakpointCallback(data_up.get(), command_body_text);
   if (error.Fail())
     return error;
   auto baton_sp =

lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_callback.test

 # REQUIRES: lua
-# RUN: %lldb -s %s --script-language lua 2>&1 | FileCheck %s
+# RUN: echo "int main() { return 0; }" | %clang_host -x c - -o %t
+# RUN: %lldb -s %s --script-language lua %t 2>&1 | FileCheck %s
 b main
 breakpoint command add -s lua
-# CHECK: error: This script interpreter does not support breakpoint callbacks
+local a = 123
+print(a)
+quit
+run
+# CHECK: 123
+breakpoint command add -s lua
+789_nil
+# CHECK: unexpected symbol near '789'

lldb/test/Shell/ScriptInterpreter/Lua/partial_statements.test

+# REQUIRES: lua
+# RUN: %lldb -s %s --script-language lua 2>&1 | FileCheck %s
+script
+do
+local a = 123
+print(a)
+end
+# CHECK: 123
+str = 'hello there!'
+function callme()
+print(str)
+end
+callme()
+# CHECK: hello there!
+# CHECK-NOT: error