Page MenuHomePhabricator

[lldb/Lua] add support for multiline scripted breakpoints
ClosedPublic

Authored by tammela on Dec 17 2020, 11:50 AM.

Details

Summary

1 - Partial Statements

The interpreter loop runs every line it receives, so partial
Lua statements are not being handled properly. This is a problem for
multiline breakpoint scripts since the interpreter loop, for this
particular case, is just an abstraction to a partially parsed function
body declaration.

This patch addresses this issue and as a side effect improves the
general Lua interpreter loop as well. It's now possible to write partial
statements in the 'script' command.

Example:

(lldb) script
>>>   do
..>   local a = 123
..>   print(a)
..>   end
123

The technique implemented is the same as the one employed by Lua's own REPL implementation.
Partial statements always errors out with the '<eof>' tag in the error
message.

2 - LoadBuffer in Lua.h

In order to support (1), we need an API for just loading string buffers. This
was not available as so far we only needed to run string buffers.

3 - Multiline scripted breakpoints

Finally, with all the base features implemented this feature is
straightforward. The interpreter loop behaves exactly the same, the
difference is that it will aggregate all Lua statements into the body of
the breakpoint function. An explicit 'quit' statement is needed to exit the
interpreter loop.

Example:

(lldb) breakpoint command add -s lua
Enter your Lua command(s). Type 'quit' to end.
The commands are compiled as the body of the following Lua function
function (frame, bp_loc, ...) end
..> print(456)
..> a = 123
..> quit

Diff Detail

Unit TestsFailed

TimeTest
70 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll

Event Timeline

tammela requested review of this revision.Dec 17 2020, 11:50 AM
tammela created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 11:50 AM

Thanks Pedro, it's really great to see the Lua interpreter getting all these improvements!

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

Shouldn't this be the same for break- and watchpoints?

81

The "quit" is something lldb-specific. Even though the logic is very simple, can you move it into a little helper function so that it doesn't get duplicated across IOHandlerIsInputComplete and IOHandlerInputComplete.

tammela updated this revision to Diff 312942.Dec 19 2020, 11:10 AM

Addressing comments

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

Probably, we can address this when we add the support for watchpoints later

tammela marked an inline comment as done.Dec 19 2020, 11:10 AM
labath added inline comments.Dec 21 2020, 12:07 PM
lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h
39

This default value does not seem particularly useful. In fact, I am not sure if this argument should even exist at all. The usage in IOHandlerIsInputComplete is sufficiently unorthodox that it might be a good idea to draw attention to the fact that something funky is happening via an explicit pop (and maybe a comment).

tammela added inline comments.Dec 22 2020, 7:29 AM
lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h
39

We can't explicitly pop, as in calling lua_pop(), from ScriptInterpreterLua.cpp because the lua_State is not exposed. Are you suggesting to add a method that wraps lua_pop() as well?

labath added inline comments.Dec 27 2020, 5:25 AM
lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h
39

Good question. I didn't realize this aspect of the problem. I think the answer to this depends on the direction we want the Lua class to go in. Back when it was first introduced, my idea was for it to be some sort of a c++ wrapper for the lua (C) api. It would offset a nicer interface to interact with lua, but would otherwise be (more-or-less) lldb-agnostic (kind of like our PythonDataObjects). In that world, it would make sense to expose the lua stack and the pop function (among other things).

However, I don't think that's really how things have worked out. The current Lua interface is much more high-level and much-more debugger-oriented than I originally imagined. Among functions like RegisterBreakpointCallback and ChangeIO, a function like Pop seems misplaced. I'm not sure this is a good position to be in (there's still a need to find a home for the c++ wrapper-like functions, for example to handle exceptions), but if we want to go down that path, then a pop function is not the right answer. However, I think the same applies to the LoadBuffer function, as it cannot be useful (beyond this syntax-check use case) without exposing the lua stack, implicitly or explicitly. In this world it would be better to make the function even more high-level, and have something like CheckSyntax(buffer) (which does pretty much what LoadBuffer does, but it always pops the result). Internally it can be implemented as a call to LoadBuffer, but this function would now be a private implementation detail instead of a part of the interface.

tammela updated this revision to Diff 313870.Dec 28 2020, 9:49 AM

Addressing comments

tammela added inline comments.Dec 28 2020, 10:08 AM
lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h
39

I think it's better to keep it as high level as possible. Exposing the lua_State spills the stack control to whom ever has access to the Lua interpreter, which might be undesirable and cumbersome to keep track of.

labath accepted this revision.Jan 6 2021, 4:29 AM

This looks better. I haven't checked the rest of the patch in detail, but it seems ok at a quick glance and Jonas appeared to be happy with it.

This revision is now accepted and ready to land.Jan 6 2021, 4:29 AM
This revision was landed with ongoing or failed builds.Jan 6 2021, 4:32 PM
This revision was automatically updated to reflect the committed changes.