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

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

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...



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:66
+  return Run(
+      llvm::formatv("{0} = require \"{0}\"", module_name.GetStringRef()).str());
+}
----------------
JDevlieghere wrote:
> labath wrote:
> > 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?
> 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`). 
I'm moving this out of line, as I've discovered even more problems here.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:18-40
+static std::string FormatQuoted(llvm::StringRef str) {
+  std::string s;
+  llvm::raw_string_ostream oss(s);
+  oss << '"';
+  for (char c : str) {
+    if (c == '"' || c == '\\' || c == '\n') {
+      oss << '\\';
----------------
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).


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

https://reviews.llvm.org/D71825