This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Lua] Support loading Lua modules.
ClosedPublic

Authored by JDevlieghere on Dec 22 2019, 8:35 PM.

Details

Reviewers
labath
Group Reviewers
Restricted Project
Commits
rG572b9f468ad6: [lldb/Lua] Support loading Lua modules
Summary

This implements the command script import command for Lua.

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 22 2019, 8:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2019, 8:35 PM

Rebase after can_reload removal

JDevlieghere marked an inline comment as done.Dec 22 2019, 9:44 PM
JDevlieghere added inline comments.
lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
57

s/ if requested//

lgtm. Pavel?

labath added a comment.Jan 7 2020, 1:32 AM

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?

  • Validate extension
  • Safely double quote strings
JDevlieghere marked 5 inline comments as done.Jan 7 2020, 10:23 AM
JDevlieghere added inline comments.
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 requested changes to this revision.Jan 8 2020, 4:45 AM

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.

This revision now requires changes to proceed.Jan 8 2020, 4:45 AM
JDevlieghere planned changes to this revision.Jan 8 2020, 9:28 AM
JDevlieghere marked an inline comment as done.

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.

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

Skip the package loader as per our discussion on IRC

labath added a comment.Jan 9 2020, 2:08 AM

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
42–45

I guess these are not needed anymore...

  • Remove unused dense map.
  • Set the global variable.
JDevlieghere marked an inline comment as done.Jan 9 2020, 8:09 AM

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 accepted this revision.Jan 10 2020, 2:42 AM

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

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h
42–45

The FormatQuoted declaration can go too...

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

Maybe additionally try to load a nonexisting module?

This revision is now accepted and ready to land.Jan 10 2020, 2:42 AM
labath added inline comments.Jan 10 2020, 4:02 AM
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...

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

Thank you for the review!

This revision was automatically updated to reflect the committed changes.