Page MenuHomePhabricator

[lldb/Lua] Implement a Simple Lua Script Interpreter Prototype
ClosedPublic

Authored by JDevlieghere on Dec 9 2019, 4:55 PM.

Details

Summary

This implements a very elementary Lua script interpreter. It supports running a single command as well as running interactively. It uses editline if available. It's still missing a bunch of stuff though. Some things that I intentionally ingored for now are that I/O isn't properly hooked up (so every print goes to stdout) and the non-editline support which is not handling a bunch of corner cases. The latter is a matter of reusing existing code in the Python interpreter.

Discussion on the mailing list: http://lists.llvm.org/pipermail/lldb-dev/2019-December/015812.html

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 9 2019, 4:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 4:55 PM
Herald added subscribers: abidh, mgorny. · View Herald Transcript
JDevlieghere retitled this revision from [lldb/Lua] Implement a Simple Lua Script Interpreter Protype to [lldb/Lua] Implement a Simple Lua Script Interpreter Prototype.
xiaobai added a subscriber: xiaobai.Dec 9 2019, 5:06 PM
xiaobai added inline comments.
lldb/source/Plugins/ScriptInterpreter/Lua/CMakeLists.txt
10

Nit: I would use target_include_directories here.

https://cmake.org/cmake/help/latest/command/target_include_directories.html

labath added a subscriber: labath.Dec 10 2019, 1:20 AM

It looks like this patch already includes some non-trivial functionality, so I think this is a good time to start figuring out how to test this. Another thing, which may not be needed straight away, but which I think we should consider pretty early is creating some sort of c++ wrappers, similar to the PythonDataObject we have for python. This is so that we don't have C error handling code everywhere, but have things wrapped in a nicer interface with llvm::Expecteds and everything.

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

How much of this class is boiler plate? Can it be shared with something?

159

TBH, I am not sure how useful this error really is. Presumably you will get some sort of an error from the lua interpreter if you just let this pass..

167

So what happens when command is not null terminated? I guess you ought to split this into luaL_loadbuffer && lua_pcall..

171

same here. You can just use AppendErrorWithFormatv..

Reuse IOHandlerEditline

  • Rebase
  • Hide C calls behind C++ abstraction
  • Improve error handling
  • Add unit test
emaste added a subscriber: emaste.Dec 16 2019, 12:51 PM
  • Rebase
  • Add test case

This is starting to look pretty good. I think that pretty soon you'll have to come up with some way of having a more persistent lua context (instead of creating a fresh one for each command), but that doesn't have to happen now.

Does the "multiline" script command do anything already? Is there anything to test with respect to that.

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
19–21

No longer needed?

25

it doesn't look like this is used (but if it is, it would be better off as a regular variable)

57

The constructor initialized this member, and the desctructor asserts it to be not null. It doesn't look like this assignment here is actually helping anything..

72

*GetOutputStreamFileSP() << llvm::toString(std::move(error))

76

delete

192

This if is not needed (new always succeeds)

lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp
53 ↗(On Diff #234637)

I'm not actually opposed to this (and it's nice to know that this could work if we need it), but why do this as a unit test? It seems like this could be tested equally well with a script command through the lldb driver. I was imagining the unit tests would be most useful for the lower level functionality -- roughly corresponding to the Lua class. Right now that class is pretty simple and doesn't really need a unit test, but I expect it will become more complex over time...

JDevlieghere marked 12 inline comments as done.
  • Rebase
  • Feedback Pavel
lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp
53 ↗(On Diff #234637)

Yes, I just wanted to show that it's possible :-) I've also included a test for the Lua class.

labath accepted this revision.Dec 20 2019, 3:45 AM
labath added inline comments.
lldb/unittests/ScriptInterpreter/Lua/CMakeLists.txt
14–15 ↗(On Diff #234798)

I think you wouldn't need these if the other CMakeLists file would use PUBLIC keyword for headers, and INTERFACE for libraries.

lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp
53 ↗(On Diff #234637)

Ok, that's cool.

This revision is now accepted and ready to land.Dec 20 2019, 3:45 AM
This revision was automatically updated to reflect the committed changes.