[Lldb-commits] [PATCH] D71825: [lldb/Lua] Support loading Lua modules.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:39
+
+  ConstString module_name = file.GetFileNameStrippingExtension();
+  if (!module_name) {
----------------
I guess you also ought to validate the extension here. Otherwise "import foo.perl" will end up importing "foo.lua".


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:49
+    if (llvm::Error e =
+            Run(llvm::formatv("package.path = package.path .. \";{0}\"",
+                              file.GetCString())
----------------
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.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:58
+  // Reload the module if requested.
+  if (llvm::Error e = Run(
+          llvm::formatv("package.loaded.{0} = nil", module_name.GetStringRef())
----------------
Here we probably ought to validate that `module_name` is a valid lua identifier.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:66
+  return Run(
+      llvm::formatv("{0} = require \"{0}\"", module_name.GetStringRef()).str());
+}
----------------
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?


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

https://reviews.llvm.org/D71825