[lldb/Lua] Support loading Lua modules.

[JDevlieghere]

Summary

This implements the command script import command for Lua.

[JDevlieghere]

Rebase after can_reload removal

[JDevlieghere]

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

57

s/ if requested//

[JDevlieghere]

friendly ping

[clayborg]

lgtm. Pavel?

[labath]

Sorry for the delay, I was OOO and I'm still processing the backlog...

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

39

I guess you also ought to validate the extension here. Otherwise "import foo.perl" will end up importing "foo.lua".

49

Embedding the file name this way seems unfortunate ("; os.execute("rm -rf /");), and it probably will not work at all on windows because of all the backslashes. Lua formatting function has a %q format to format strings safely. Accessing that from C is a bit tricky, but we can make a utility function for that.

58

Here we probably ought to validate that module_name is a valid lua identifier.

66

Is there any advantage to using require here? It doesn't seems like it brings much into the game, as its main purpose seems to be to _find_ the module, but here we know exactly where the module is. OTOH, using it has a lot of downsides (needing to quote/validate all strings, mess with the package.loaded variable, potentially loading the wrong file, etc).

Would it be possible to just luaL_loadfile the file and then assign the result of its evaluation to the appropriate variable manually?

[JDevlieghere]

• Validate extension
• Safely double quote strings

[JDevlieghere]

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

49

I've used Lua's %q implementation. Calling string.format() didn't work for the require statement.

66

The advantage is that it's the standard way to import modules in Lua and users should be able to rely on that. If you use luaL_loadfile it will circumvent the runtime. This can have undesirable side effects, for example importing the same module again will reload it (which will not happen if it has already been loaded with require).

[labath]

I think this needs more discussion. The current method of appending to package.path seems to be completely broken. package.path contains patterns (e.g. /?.lua;/usr/share/lua/5.1/?.lua;...). Adding an absolute path to this variable will cause that file to be loaded for _any_ value one passes to the require function:

$ cat /tmp/a.lua 
print("hello from a.lua")
return {}
$ lua
Lua 5.1.5  Copyright (C) 1994-2012 Lua.org, PUC-Rio
> require "a"
stdin:1: module 'a' not found:
	no field package.preload['a']
	no file './a.lua'
	no file '/usr/share/lua/5.1/a.lua'
	no file '/usr/share/lua/5.1/a/init.lua'
	no file '/usr/lib64/lua/5.1/a.lua'
	no file '/usr/lib64/lua/5.1/a/init.lua'
	no file '/usr/share/lua/5.1/a.lua'
	no file '/usr/share/lua/5.1/a/init.lua'
	no file './a.so'
	no file '/usr/lib64/lua/5.1/a.so'
	no file '/usr/lib64/lua/5.1/a.so'
	no file '/usr/lib64/lua/5.1/loadall.so'
stack traceback:
	[C]: in function 'require'
	stdin:1: in main chunk
	[C]: ?
> package.path = package.path .. ';/tmp/a.lua'
> require "a"
hello from a.lua
> require "b"
hello from a.lua
> require "huh?"
hello from a.lua
> require "@$#%^@#%@#%@#%@#"
hello from a.lua

I'm afraid we will need to do something more elaborate (and fiddly, unfortunately) to implement this. I see a couple of options, though none are really ideal. I'm not sure I'll have time to go into them in detail (I'll try to get back to it this evening), but I'm interested in hearing your thoughts about this...

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

18–40

Umm... that's not exactly what I had in mind.. :( I was imagining a utility function to call string.format from lua. Something along the lines of:

push(string);
push("%q");
getfield("format")
pcall(1)
pop(quoted_string);

It could be made even more generic than that because I'm pretty sure that we will soon need a mechanism to call a random lua entity (e.g. for breakpoint callbacks).

66

I'm moving this out of line, as I've discovered even more problems here.

[JDevlieghere]

In D71825#1809825, @labath wrote:

I think this needs more discussion. The current method of appending to package.path seems to be completely broken. package.path contains patterns (e.g. /?.lua;/usr/share/lua/5.1/?.lua;...). Adding an absolute path to this variable will cause that file to be loaded for _any_ value one passes to the require function:

$ cat /tmp/a.lua 
print("hello from a.lua")
return {}
$ lua
Lua 5.1.5  Copyright (C) 1994-2012 Lua.org, PUC-Rio
> require "a"
stdin:1: module 'a' not found:
	no field package.preload['a']
	no file './a.lua'
	no file '/usr/share/lua/5.1/a.lua'
	no file '/usr/share/lua/5.1/a/init.lua'
	no file '/usr/lib64/lua/5.1/a.lua'
	no file '/usr/lib64/lua/5.1/a/init.lua'
	no file '/usr/share/lua/5.1/a.lua'
	no file '/usr/share/lua/5.1/a/init.lua'
	no file './a.so'
	no file '/usr/lib64/lua/5.1/a.so'
	no file '/usr/lib64/lua/5.1/a.so'
	no file '/usr/lib64/lua/5.1/loadall.so'
stack traceback:
	[C]: in function 'require'
	stdin:1: in main chunk
	[C]: ?
> package.path = package.path .. ';/tmp/a.lua'
> require "a"
hello from a.lua
> require "b"
hello from a.lua
> require "huh?"
hello from a.lua
> require "@$#%^@#%@#%@#%@#"
hello from a.lua

I'm afraid we will need to do something more elaborate (and fiddly, unfortunately) to implement this. I see a couple of options, though none are really ideal. I'm not sure I'll have time to go into them in detail (I'll try to get back to it this evening), but I'm interested in hearing your thoughts about this...

Seems like the solution is to use the ?.lua which is also what's used for picking up files in the current directory.

Lua 5.3.5  Copyright (C) 1994-2018 Lua.org, PUC-Rio
> package.path = package.path .. ';/tmp/?.lua'
> require "a"
hello from a.lua
table: 0x7fbd13401a60
> require "b"
stdin:1: module 'b' not found:
        no field package.preload['b']
        no file '/usr/local/share/lua/5.3/b.lua'
        no file '/usr/local/share/lua/5.3/b/init.lua'
        no file '/usr/local/lib/lua/5.3/b.lua'
        no file '/usr/local/lib/lua/5.3/b/init.lua'
        no file './b.lua'
        no file './b/init.lua'
        no file '/tmp/b.lua'
        no file '/usr/local/lib/lua/5.3/b.so'
        no file '/usr/local/lib/lua/5.3/loadall.so'
        no file './b.so'
stack traceback:
        [C]: in function 'require'
        stdin:1: in main chunk
        [C]: in ?

FWIW I just followed the instructions from http://lua-users.org/wiki/ModulesTutorial.

[JDevlieghere]

• Use ?.lua to import modules
• Use Lua runtime to quote strings

[JDevlieghere]

Skip the package loader as per our discussion on IRC

[labath]

Thanks. For posterity, I'm going to summarize my thoughts from that discussion.

I was arguing that "require" is not a good thing to be using here, because it's hard for it to guarantee that it will load a specific file. Now, that does not matter for "normal" uses of "require", but it is pretty unfortunate for the "command script import" command. This command takes an explicit filename argument, and it'd be pretty surprising to have "command script import /bar/foo.lua" load "/baz/foo.lua" just because "/baz" happened to come first in the search path.

Since figuring out what to load is the largest part of the "require" function, and that's exactly the part we don't need here, I think we can just drop "require" and implement the loading ourselves. (The other responsibility of the "require" function is to ensure a module is not loaded twice, but this is something we need to override anyway, as the "command script import" contract states that the module is always reloaded.)

Now back to the current patch: I see that you've dropped the part which assigns the result of evaluating the module to a global variable (and consequently, dropped the "local" keyword from the "mymodule" declaration). That seems unfortunate, as the recommended way for writing modules is to make the variable local, and return the module as a result.

What's the reasoning behind that? I was expecting we would keep that part... All it would take is adding an extra lua_setglobal(m_lua_state, name) call after the pcall stuff.

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

43–46

I guess these are not needed anymore...

[JDevlieghere]

• Remove unused dense map.
• Set the global variable.

[JDevlieghere]

In D71825#1811587, @labath wrote:

Now back to the current patch: I see that you've dropped the part which assigns the result of evaluating the module to a global variable (and consequently, dropped the "local" keyword from the "mymodule" declaration). That seems unfortunate, as the recommended way for writing modules is to make the variable local, and return the module as a result.

What's the reasoning behind that? I was expecting we would keep that part... All it would take is adding an extra lua_setglobal(m_lua_state, name) call after the pcall stuff.

Thanks! I couldn't get it to work yesterday evening because I was vastly overthinking it after looking at the package implementation and getting sidetracked by the meta table. I thought the runtime was doing some magic which I didn't fully understand and that made me overlook the stupidly simply solution of using using the result as a global...

[labath]

This looks fine to me now. Thanks for your patience.

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

43–46

The FormatQuoted declaration can go too...

lldb/test/Shell/ScriptInterpreter/Lua/command_script_import.test

3

Maybe additionally try to load a nonexisting module?

[labath]

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

46

It would be better to put 1 here instead LUA_MULTRET, to ensure we don't end up with an unbalanced stack if the module happens to return more than one value...

[JDevlieghere]

In D71825#1813766, @labath wrote:

This looks fine to me now. Thanks for your patience.

Thank you for the review!

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 "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
 
 #include "lua.hpp"
 
 #include <mutex>
 
 namespace lldb_private {
 
public:
   }
 
   ~Lua() {
     assert(m_lua_state);
     luaL_openlibs(m_lua_state);
   }
 
   llvm::Error Run(llvm::StringRef buffer);
+  llvm::Error LoadModule(llvm::StringRef filename);
 
 private:
   std::mutex m_mutex;
   lua_State *m_lua_state;
+  llvm::StringSet<> m_loaded_modules;
 };
 
 } // 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 "lldb/Host/FileSystem.h"
+#include "lldb/Utility/FileSpec.h"
 #include "llvm/Support/FormatVariadic.h"
 
 using namespace lldb_private;
 
 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::LoadModule(llvm::StringRef filename) {
+  FileSpec file(filename);
+  if (!FileSystem::Instance().Exists(file)) {
+    return llvm::make_error<llvm::StringError>("invalid path",
+                                               llvm::inconvertibleErrorCode());
+  }
+
+  ConstString module_name = file.GetFileNameStrippingExtension();
+  if (!module_name) {
+    return llvm::make_error<llvm::StringError>("invalid module name",
+                                               llvm::inconvertibleErrorCode());
+  }
+
+  // Don't clutter the package path and only add the module path if we haven't
+  // seen it before.
+  if (!m_loaded_modules.count(filename)) {
+    if (llvm::Error e =
+            Run(llvm::formatv("package.path = package.path .. \";{0}\"",
+                              file.GetCString())
+                    .str())) {
+      return e;
+    }
+    m_loaded_modules.insert(filename);
+  }
+
+  // Reload the module if requested.
+  if (llvm::Error e = Run(
+          llvm::formatv("package.loaded.{0} = nil", module_name.GetStringRef())
+              .str())) {
+    return e;
+  }
+
+  // Import the module.
+  return Run(
+      llvm::formatv("{0} = require \"{0}\"", module_name.GetStringRef()).str());
+}

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

public:
   ~ScriptInterpreterLua() override;
 
   bool ExecuteOneLine(
       llvm::StringRef command, CommandReturnObject *result,
       const ExecuteScriptOptions &options = ExecuteScriptOptions()) override;
 
   void ExecuteInterpreterLoop() override;
 
+  virtual bool
+  LoadScriptingModule(const char *filename, bool init_session,
+                      lldb_private::Status &error,
+                      StructuredData::ObjectSP *module_sp = nullptr) override;
+
   // Static Functions
   static void Initialize();
 
   static void Terminate();
 
   static lldb::ScriptInterpreterSP CreateInstance(Debugger &debugger);
 
   static lldb_private::ConstString GetPluginNameStatic();

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

void ScriptInterpreterLua::ExecuteInterpreterLoop() {
 
   if (!debugger.GetInputFile().IsValid())
     return;
 
   IOHandlerSP io_handler_sp(new IOHandlerLuaInterpreter(debugger, *this));
   debugger.PushIOHandler(io_handler_sp);
 }
 
+bool ScriptInterpreterLua::LoadScriptingModule(
+    const char *filename, bool init_session, lldb_private::Status &error,
+    StructuredData::ObjectSP *module_sp) {
+
+  if (llvm::Error e = m_lua->LoadModule(filename)) {
+    error.SetErrorStringWithFormatv("lua failed to import '{0}': {1}\n",
+                                    filename, llvm::toString(std::move(e)));
+    return false;
+  }
+  return true;
+}
+
 void ScriptInterpreterLua::Initialize() {
   static llvm::once_flag g_once_flag;
 
   llvm::call_once(g_once_flag, []() {
     PluginManager::RegisterPlugin(GetPluginNameStatic(),
                                   GetPluginDescriptionStatic(),
                                   lldb::eScriptLanguageLua, CreateInstance);
   });

lldb/test/Shell/ScriptInterpreter/Lua/Inputs/testmodule.lua

+local mymodule = {}
+
+function mymodule.foo()
+  print("Hello World!")
+end
+
+return mymodule

lldb/test/Shell/ScriptInterpreter/Lua/command_script_import.test

+# REQUIRES: lua
+# RUN: %lldb --script-language lua -o 'command script import %S/Inputs/testmodule.lua' -o 'script testmodule.foo()' 2>&1 | FileCheck %s
+# RUN: %lldb --script-language lua -o 'command script import %S/Inputs/testmodule.lua' -o 'command script import %S/Inputs/testmodule.lua' -o 'script testmodule.foo()'  2>&1 | FileCheck %s
+# CHECK: Hello World!