This is an archive of the discontinued LLVM Phabricator instance.

Differential D91508

[LLDB/Lua] add support for one-liner breakpoint callback
ClosedPublic

Authored by tammela on Nov 15 2020, 1:01 PM.

Details

Reviewers
JDevlieghere
labath
Commits
rGa0d7406ae800: [LLDB/Lua] add support for one-liner breakpoint callback
Summary

These callbacks are set using the following:

breakpoint command add -s lua -o "print('hello world!')"

The user supplied script is executed as:

function (frame, bp_loc, ...)
   <body>
end

So the local variables 'frame', 'bp_loc' and vararg are all accessible.
Any global variables declared will persist in the Lua interpreter.
A user should never hold 'frame' and 'bp_loc' in a global variable as
these userdatas are context dependent.

Diff Detail

Event Timeline

tammela created this revision.Nov 15 2020, 1:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2020, 1:01 PM
Herald added a subscriber: lldb-commits. · View Herald Transcript
tammela requested review of this revision.Nov 15 2020, 1:01 PM
Herald added a subscriber: JDevlieghere. · View Herald TranscriptNov 15 2020, 1:01 PM
tammela added a reviewer: JDevlieghere.Nov 15 2020, 1:02 PM
tammela added a comment.Nov 15 2020, 1:09 PM

@JDevlieghere

When writing this patch I noticed that there is no mechanism in-place to remove the Python/Lua function when the breakpoint is removed or when the callback function is replaced.
The class that sets the callback provides ClearCallback() but it never calls into the ScriptInterpreter to clean up.
Although the real world impact is not that big, it's crucial for a small memory footprint. Any thoughts on this?

Harbormaster completed remote builds in B78901: Diff 305387.Nov 15 2020, 1:39 PM
tammela updated this revision to Diff 305427.Nov 16 2020, 1:11 AM

Clean up includes

Harbormaster completed remote builds in B78930: Diff 305427.Nov 16 2020, 1:40 AM
labath added a subscriber: labath.Nov 16 2020, 1:48 AM
labath added inline comments.
lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
83–101

I'm confused. Why use lua to call a c callback, when you could just do calllback()?

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

How is lua_interpreter different from this?

206
if (llvm::Error E = lua.Run(callback)) {
  debugger.GetErrorStream() << toString(std::move(E));
  stop = true;
}

would be less callback-y.

lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
4

Print something more unique so this doesn't match accidentally?

tammela added inline comments.Nov 17 2020, 1:30 AM
lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
83–101

Some Lua API functions throw errors, if there's any it will abort() the program if no panic handler or protected call is provided.
To guarantee that the callback always runs in a protected environment and it has error handling, we do the above.
Delegating this to the callback itself makes it cumbersome to write.

JDevlieghere added a comment.Nov 17 2020, 9:13 AM
In D91508#2396392, @tammela wrote:

@JDevlieghere

When writing this patch I noticed that there is no mechanism in-place to remove the Python/Lua function when the breakpoint is removed or when the callback function is replaced.
The class that sets the callback provides ClearCallback() but it never calls into the ScriptInterpreter to clean up.
Although the real world impact is not that big, it's crucial for a small memory footprint. Any thoughts on this?

I see, it looks like we just clean up the LLDB side but never tell the runtimes about it. What exactly are we leaking in that case? Do you think it's worth it to wire that up?

JDevlieghere added a comment.Nov 17 2020, 9:19 AM

This looks good with Pavel's comments addressed and an integration test added for the breakpoint callback. (Watchpoints are not supported everywhere and it's hard to determine whether they are from a Shell test).

lldb/bindings/lua/lua-wrapper.swig
30

Might be nice to add a comment to explain the "magic values" here.

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h
28

Let's move this into the Lua class (and rename it to Callback) so we don't taint every CU that includes this file.

labath requested changes to this revision.Nov 17 2020, 9:34 AM

I'll have more questions about this, but probably won't get around to asking them today.

This revision now requires changes to proceed.Nov 17 2020, 9:34 AM
tammela updated this revision to Diff 305874.Nov 17 2020, 12:15 PM
tammela marked 4 inline comments as done and an inline comment as not done.

Adressing comments

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

Are you asking why I didn't go for m_script_interpreter?

This function is static (on the class declaration), perhaps it's clearer a static keyword here as well?

tammela added a comment.Nov 17 2020, 12:20 PM
In D91508#2400222, @JDevlieghere wrote:
In D91508#2396392, @tammela wrote:

@JDevlieghere

When writing this patch I noticed that there is no mechanism in-place to remove the Python/Lua function when the breakpoint is removed or when the callback function is replaced.
The class that sets the callback provides ClearCallback() but it never calls into the ScriptInterpreter to clean up.
Although the real world impact is not that big, it's crucial for a small memory footprint. Any thoughts on this?

I see, it looks like we just clean up the LLDB side but never tell the runtimes about it. What exactly are we leaking in that case? Do you think it's worth it to wire that up?

For both interpreters, the function objects itself. I think it's worth it, specially for long running debugging sessions.

Harbormaster completed remote builds in B79168: Diff 305874.Nov 17 2020, 12:43 PM
labath added inline comments.Nov 18 2020, 2:05 AM
lldb/bindings/lua/lua-wrapper.swig
27–28

This name made sense for python, as the functions actually returned the wrappers. But here, the name makes it pretty unobvious that these functions actually push the object on the lua stack. It'd be better if this was called something like PushSBType or something

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
83–101

Aha, I see.

So, if I remember my lua correctly (I wrote a c++ lua wrapper once, but that was years ago..), whenever there's a lua exception inside this (c++) callback, lua will longjmp(3) back to the lua_pcall call on line 68, skipping any destructors that should normally be invoked. Is that so?

If that's the case, then I think this is a dangerous api, that should at the very least get a big fat warning, but that ideally shouldn't exist at all.

What's the part that makes delegating this to the callback "cumersome to write"? And why can't that be overcome by a suitable utility function which wraps lua_pcall or whatever else is needed?

The approach that we've chosen in python is to have very little code interacting with the python C API directly. Instead, code generally works with our C++ wrappers defined in PythonDataObject.h. These functions try to hide the python exception magic as much as possible, and present a c++-y version of the interface.

Now, since lua is stack-based, something like LuaDataObjects.h would probably not work. However, that doesn't meant we should give up on the c++ wrapper idea altogether. During the intitial reviews, my intention was for the Lua class to serve this purpose. I still think this can be achieved if we make the callback functions take Lua& instead of lua_State* as an argument, and then ensure the class contains whatever is needed to make the callbacks not cumerbsome to write.

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

I just missed the fact that this is a static function (I hate these static baton functions in lldb). Putting static on an out-of-line member function definition is not valid c++.

However, this does raise a different issue. Debugger::GetScriptInterpreter will return the default script interpreter, and that may not be the lua one. I think you should pass eScriptLanguageLua here explicitly.

tammela planned changes to this revision.Nov 18 2020, 11:41 AM
tammela added inline comments.Nov 18 2020, 12:16 PM
lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
83–101

So, if I remember my lua correctly (I wrote a c++ lua wrapper once, but that was years ago..), whenever there's a lua exception inside this (c++) callback, lua will longjmp(3) back to the lua_pcall call on line 68, skipping any destructors that should normally be invoked. Is that so?

If that's the case, then I think this is a dangerous api, that should at the very least get a big fat warning, but that ideally shouldn't exist at all.

You are right. This escaped me completely.
Lua can be compiled to use try/catch instead of longjmp, but that is an exception (no pun intended). Since we rely on the host's library, we need to assume that it's using longjmp.

What's the part that makes delegating this to the callback "cumersome to write"? And why can't that be overcome by a suitable utility function which wraps lua_pcall or whatever else is needed?

The approach that we've chosen in python is to have very little code interacting with the python C API directly. Instead, code generally works with our C++ wrappers defined in PythonDataObject.h. These functions try to hide the python exception magic as much as possible, and present a c++-y version of the interface.

Now, since lua is stack-based, something like LuaDataObjects.h would probably not work. However, that doesn't meant we should give up on the c++ wrapper idea altogether. During the intitial reviews, my intention was for the Lua class to serve this purpose. I still think this can be achieved if we make the callback functions take Lua& instead of lua_State* as an argument, and then ensure the class contains whatever is needed to make the callbacks not cumerbsome to write.

Any C++ code that runs in a lua_pcall seems to face this issue. I checked a very complete C++ Lua wrapper called sol2 and they seem to have the same issue. I don't think there's a way out of it except code discipline.

Lua provides lua_atpanic as a way to recover from unprotected throws. Perhaps we can leverage this to throw an exception, guaranteeing stack unwinding and avoiding a call to abort(). We would then let the callback run unprotected and delegate any calls to lua_pcall to the callback.

Changes proposed:

  • Register the panic function on Lua ctor
  • Let the Callback run unprotected
  • Wrap the Callback call in a try catch block
  • Change the Callback signature to receive Lua& and return llvm::Error

It looks like it ticks all goals.

  • Unprotected Lua errors do not cause lldb to abort.
  • Stack unwinding is guaranteed for C++ code interacting with the Lua state.
  • Errors are propagated
labath added a comment.Nov 19 2020, 1:02 AM

Continuing the discussion from the inline comment:

Lua can be compiled to use try/catch instead of longjmp, but that is an exception (no pun intended). Since we rely on the host's library, we need to assume that it's using longjmp.

Right. In fact even if it had exceptions enabled, that still wouldn't help as llvm itself is also compiled without exceptions (it can be, but it's an exception :P).

Any C++ code that runs in a lua_pcall seems to face this issue. I checked a very complete C++ Lua wrapper called sol2 and they seem to have the same issue. I don't think there's a way out of it except code discipline.

I'm not sure what you mean by that. Can you elaborate?

The way I see it, the problem is not about which environment is the c++ code running in, as it can't throw an exception (in llvm). The problem is what happens when we call lua from c++, and the lua code throws. And this is a problem regardless of the environment the C(++) code is in -- the difference is "only" in whether lua will panic or jump over the C code, but something bad will happen anyway.

I think that could be solved by ensuring we always call lua in a protected environment. That way lua will never unwind through the C code (regardless of what environment it is in) because it would always stop right at the boundary.

That would essentially mean, lua_call is banned (except maybe as an optimization, if you can be sure that the code you're about to call will not throw), and all C->lua transitions should go through lua_pcall. And we can provide a wrapper that handles catches those errors and converts them to llvm::Error/Expected, similar to how the python things do it.

Lua provides lua_atpanic as a way to recover from unprotected throws. Perhaps we can leverage this to throw an exception, guaranteeing stack unwinding and avoiding a call to abort(). We would then let the callback run unprotected and delegate any calls to lua_pcall to the callback.

I'm afraid that won't work, because exceptions are not allowed in llvm. But I think something similar to what I propose above should do the trick.

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
83–101

I'll move this out-of-line, as it's getting rather long.

tammela updated this revision to Diff 307202.Nov 23 2020, 2:37 PM

Redesign of the API

tammela added a comment.Nov 23 2020, 2:47 PM

I'm not sure what you mean by that. Can you elaborate?

Sure, it's was just a nod to your previous comment about that running callbacks (C++ lambdas) inside a pcall is a dangerous API. Although possible, it requires that the developer be very cautious about implicit throws.

The way I see it, the problem is not about which environment is the c++ code running in, as it can't throw an exception (in llvm). The problem is what happens when we call lua from c++, and the lua code throws. And this is a problem regardless of the environment the C(++) code is in -- the difference is "only" in whether lua will panic or jump over the C code, but something bad will happen anyway.

I agree 100% with your analysis of the problem.

I think that could be solved by ensuring we always call lua in a protected environment. That way lua will never unwind through the C code (regardless of what environment it is in) because it would always stop right at the boundary.

That would essentially mean, lua_call is banned (except maybe as an optimization, if you can be sure that the code you're about to call will not throw), and all C->lua transitions should go through lua_pcall. And we can provide a wrapper that handles catches those errors and converts them to llvm::Error/Expected, similar to how the python things do it.

I have posted a redesign based on your comments. The lua_push* functions are all safe-ish. The Lua implementation guarantees at least 20 stack slots when the lua_State is created so I've added the stack checks for sanity rather than necessity.

Harbormaster completed remote builds in B79872: Diff 307202.Nov 23 2020, 3:04 PM
labath added a comment.Nov 24 2020, 5:45 AM
In D91508#2412510, @tammela wrote:

I'm not sure what you mean by that. Can you elaborate?

Sure, it's was just a nod to your previous comment about that running callbacks (C++ lambdas) inside a pcall is a dangerous API. Although possible, it requires that the developer be very cautious about implicit throws.

Right. That's why I'd like to have good wrappers, which make it easy to do the right thing, and hard to do the wrong one.

I don't think we're quite there yet, but before I comment on the API, I want to understand one other thing.

I am puzzled by all the wrapping that's happening inside the PushSBClass functions. What is that protecting us from? I would hope that pushing a swig wrapper on the stack is a safe operation...

The Lua implementation guarantees at least 20 stack slots when the lua_State is created so I've added the stack checks for sanity rather than necessity.

So, IIUC, this can only fail if we are running out of memory? If that's the case, then I would remove these checks, as (for better or worse) llvm is not robust against memory allocation errors, and they add a fair amount of cruft to the code.

tammela marked an inline comment as done.Nov 24 2020, 6:59 AM
In D91508#2413582, @labath wrote:

Right. That's why I'd like to have good wrappers, which make it easy to do the right thing, and hard to do the wrong one.

I don't think we're quite there yet, but before I comment on the API, I want to understand one other thing.

I am puzzled by all the wrapping that's happening inside the PushSBClass functions. What is that protecting us from? I would hope that pushing a swig wrapper on the stack is a safe operation...

I thought that too, but internally it's a naked call to lua_newuserdata() which might throw in case of a memory error.

So, IIUC, this can only fail if we are running out of memory? If that's the case, then I would remove these checks, as (for better or worse) llvm is not robust against memory allocation errors, and they add a fair amount of cruft to the code.

Fair enough. Will remove those.
Since this seems to be a fact of life for LLVM, perhaps wrapping potential memory errors turns out to be just bloat. If that's the case, then the wrapping in PushSBClass is not needed and the abort() call that Lua does is honest.

tammela updated this revision to Diff 307440.Nov 24 2020, 1:15 PM

Remove unnecessary wrappers/checks

Harbormaster completed remote builds in B80009: Diff 307440.Nov 24 2020, 1:44 PM
labath added a comment.Nov 25 2020, 5:24 AM

I think we're converging on something. Now to iron out the details...

In D91508#2413789, @tammela wrote:

I am puzzled by all the wrapping that's happening inside the PushSBClass functions. What is that protecting us from? I would hope that pushing a swig wrapper on the stack is a safe operation...

I thought that too, but internally it's a naked call to lua_newuserdata() which might throw in case of a memory error.

So, IIUC, this can only fail if we are running out of memory? If that's the case, then I would remove these checks, as (for better or worse) llvm is not robust against memory allocation errors, and they add a fair amount of cruft to the code.

Fair enough. Will remove those.
Since this seems to be a fact of life for LLVM, perhaps wrapping potential memory errors turns out to be just bloat. If that's the case, then the wrapping in PushSBClass is not needed and the abort() call that Lua does is honest.

Right. I'm not worried about OOM errors (well.. I am slightly, but I don't think it's worthwhile to do something about them without addressing the bigger problem).

lldb/bindings/lua/lua-wrapper.swig
21–22

What's up with the copying? Is it to ensure the callback does not modify the caller's values? If so, you could just drop the & from the signature, and have the compiler copy these for you...

25–26

Is there a reason this function has to be in this file? It would be nice all the baton/registry handling was happening in the same place...

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
92

Shouldn't this also pop the baton ?

lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
5

Could we also have a test where calling the callback raises an error? And for the different values of the "stop" argument that can be returned from the callback? And when compiling the callback expression results in an error?

tammela updated this revision to Diff 308029.Nov 27 2020, 5:51 AM
tammela marked 3 inline comments as done.

Addressing review comments

lldb/bindings/lua/lua-wrapper.swig
21–22

SBFrame and SBBreakpointLocation only provide copy constructors. I can't see the difference between the two strategies, could you elaborate?

25–26

My reasoning was to have the lua_pcall on the same function that pushes the Lua callback but I changed to your version as it's clearer who is handling the baton (Lua class)

lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
5

Apparently when the user sets the breakpoint and it fails to compile the code lldb will not report any errors and it will fail silently. I will address this particular test case in another patch since it affects Python as well.

Harbormaster completed remote builds in B80338: Diff 308029.Nov 27 2020, 6:21 AM
labath added inline comments.Nov 30 2020, 12:42 AM
lldb/bindings/lua/lua-wrapper.swig
21–22

Ok, let's try this.

My main point was that a function which receives an argument as a (non-const) reference, and then does not modify the referenced object is weird. One just doesn't do that. It should either take a const reference or a plain value. Using a plain value is simpler, particularly in cases where you need to locally modify the object you received, but you don't want the modifications to affect the callers value. That's because in this case the compiler will do the copying for you:

void foo(SBFrame foo, SBFrame &bar, const SBFrame &baz) {
  // I don't have to worry about modifying foo, because it's my local copy
  foo = ...;
  
  // Modifications to bar will be visible in the caller
  bar = ...;

  // I can't modify baz. If I really need to do that, I need to make a local copy
  SBFrame baz_copy = baz;
  baz_copy = ...;
}
lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test
5

Sounds good.

11

"frame #0" will also get printed if the process stops for any reason. I'd suggest printing a completely random string instead (print("bacon"), for example)

tammela updated this revision to Diff 308319.Nov 30 2020, 4:16 AM

Addressing comments

labath accepted this revision.Nov 30 2020, 4:17 AM

I think this is ok now. Thanks for your patience.

This revision is now accepted and ready to land.Nov 30 2020, 4:17 AM
Harbormaster completed remote builds in B80510: Diff 308319.Nov 30 2020, 5:02 AM
This revision was landed with ongoing or failed builds.Nov 30 2020, 6:12 AM
Closed by commit rGa0d7406ae800: [LLDB/Lua] add support for one-liner breakpoint callback (authored by tammela). · Explain Why
This revision was automatically updated to reflect the committed changes.
tammela added a commit: rGa0d7406ae800: [LLDB/Lua] add support for one-liner breakpoint callback.
teemperor added a subscriber: teemperor.Apr 19 2021, 8:24 AM

I think this might have broken the Windows build as it seems the LLDBSwigLuaBreakpointCallbackFunction return type is an error for MSVC (and not a warning which we seem to ignore locally).

To quote someone from the Discord server:

I'm getting the following build error when attempting to build 12.0.0 using Microsoft CL 19.28.29913 for x64:
C:\llvm-build\llvm-project-12.0.0\lldb\source\Plugins\ScriptInterpreter\Lua\Lua.cpp(29): error C2526: 'LLDBSwigLuaBreakpointCallbackFunction': C linkage function cannot return C++ class 'llvm::Expected<bool>'
C:\llvm-build\llvm-project-12.0.0\lldb\source\Plugins\ScriptInterpreter\Lua\Lua.h(36): note: see declaration of 'llvm::Expected<bool>'
C:\llvm-build\llvm-project-12.0.0\lldb\source\Plugins\ScriptInterpreter\Lua\Lua.cpp(112): error C2440: 'return': cannot convert from 'void' to 'llvm::Expected<bool>'
JDevlieghere added a subscriber: kastiglione.Apr 19 2021, 8:49 AM
In D91508#2698704, @teemperor wrote:

I think this might have broken the Windows build as it seems the LLDBSwigLuaBreakpointCallbackFunction return type is an error for MSVC (and not a warning which we seem to ignore locally).

To quote someone from the Discord server:

I'm getting the following build error when attempting to build 12.0.0 using Microsoft CL 19.28.29913 for x64:
C:\llvm-build\llvm-project-12.0.0\lldb\source\Plugins\ScriptInterpreter\Lua\Lua.cpp(29): error C2526: 'LLDBSwigLuaBreakpointCallbackFunction': C linkage function cannot return C++ class 'llvm::Expected<bool>'
C:\llvm-build\llvm-project-12.0.0\lldb\source\Plugins\ScriptInterpreter\Lua\Lua.h(36): note: see declaration of 'llvm::Expected<bool>'
C:\llvm-build\llvm-project-12.0.0\lldb\source\Plugins\ScriptInterpreter\Lua\Lua.cpp(112): error C2440: 'return': cannot convert from 'void' to 'llvm::Expected<bool>'

@kastiglione isn't this the error you fixed for Python? Should we do the same for Lua?

jbsolomon added a subscriber: jbsolomon.EditedApr 20 2021, 9:26 AM
In D91508#2698780, @JDevlieghere wrote:

@kastiglione isn't this the error you fixed for Python? Should we do the same for Lua?

Correct me if I'm wrong, but I think the Python integration uses the same pragma approach and return type. I have it disabled, since I don't have a reliable Python environment on this machine, so I don't know for sure that it suffers from the same compilation error, but it appears to use the same syntax.

mstorsjo added a subscriber: mstorsjo.Oct 15 2021, 9:59 AM

FWIW, I had debugged the issue described here and posted about it on Discord, but never got around to posting it here. I think the MSVC build error above gets fixed by this patch:

diff --git a/lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp b/lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
index f14e2732f6eb..788072cffd87 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp
@@ -24,6 +24,7 @@ using namespace lldb;
 #if _MSC_VER
 #pragma warning (push)
 #pragma warning (disable : 4190)
+llvm::Expected<bool> unusedFuncForInstantiatingExpectedBool() { return true; }
 #endif

 extern "C" llvm::Expected<bool> LLDBSwigLuaBreakpointCallbackFunction(

Here's a minimal showcase of the root issue:

$ cat cpp-ret-c.cpp 
template<class T> class MyClass {
public:
    T a;
};
extern "C" MyClass<int> unmangledFunction();
$ cl -c cpp-ret-c.cpp 
cpp-ret-c.cpp
cpp-ret-c.cpp(5): error C2526: 'unmangledFunction': C linkage function cannot return C++ class 'MyClass<int>' 
cpp-ret-c.cpp(5): note: see declaration of 'MyClass<int>'
$ cat cpp-ret-c2.cpp 
template<class T> class MyClass {
public:
    T a;
};
MyClass<int> unusedFunc() { return MyClass<int>{0}; }
extern "C" MyClass<int> unmangledFunction();
$ cl -c cpp-ret-c2.cpp
cpp-ret-c2.cpp

So this construct, in MSVC, requires that the problematic return type template already has been instantiated somewhere else first. The case in the python plugin works, but it has been copied to the Lua plugin too, where it fails.

Revision Contents

PathSize
lldb/
bindings/
lua/
15 lines
51 lines
4 lines
source/
Plugins/
ScriptInterpreter/
Lua/
6 lines
46 lines
17 lines
46 lines
test/
Shell/
ScriptInterpreter/
Lua/
7 lines
unittests/
ScriptInterpreter/
Lua/
24 lines

Diff 307440

lldb/bindings/lua/lua-swigsafecast.swig

  • This file was added.
template <typename SBClass>
void
PushSBClass (lua_State* L, SBClass* obj);
void
PushSBClass (lua_State* L, lldb::SBFrame* frame_sb)
{
SWIG_NewPointerObj(L, frame_sb, SWIGTYPE_p_lldb__SBFrame, 0);
}
void
PushSBClass (lua_State* L, lldb::SBBreakpointLocation* breakpoint_location_sb)
{
SWIG_NewPointerObj(L, breakpoint_location_sb, SWIGTYPE_p_lldb__SBBreakpointLocation, 0);
}

lldb/bindings/lua/lua-wrapper.swig

  • This file was added.
%header %{
template <typename T>
void
PushSBClass(lua_State* L, T* obj);
%}
%wrapper %{
// This function is called from Lua::CallBreakpointCallback
SWIGEXPORT llvm::Expected<bool>
LLDBSwigLuaBreakpointCallbackFunction
(
lua_State *L,
void *baton,
lldb::StackFrameSP &stop_frame_sp,
lldb::BreakpointLocationSP &bp_loc_sp
)
{
lldb::SBFrame sb_frame(stop_frame_sp);
lldb::SBBreakpointLocation sb_bp_loc(bp_loc_sp);
labathUnsubmitted
Not Done
ReplyInline Actions

What's up with the copying? Is it to ensure the callback does not modify the caller's values? If so, you could just drop the & from the signature, and have the compiler copy these for you...

labath: What's up with the copying? Is it to ensure the callback does not modify the caller's values?
tammelaAuthorUnsubmitted
Done
ReplyInline Actions

SBFrame and SBBreakpointLocation only provide copy constructors. I can't see the difference between the two strategies, could you elaborate?

tammela: `SBFrame` and `SBBreakpointLocation` only provide copy constructors. I can't see the difference…
labathUnsubmitted
Not Done
ReplyInline Actions

Ok, let's try this.

My main point was that a function which receives an argument as a (non-const) reference, and then does not modify the referenced object is weird. One just doesn't do that. It should either take a const reference or a plain value. Using a plain value is simpler, particularly in cases where you need to locally modify the object you received, but you don't want the modifications to affect the callers value. That's because in this case the compiler will do the copying for you:

void foo(SBFrame foo, SBFrame &bar, const SBFrame &baz) {
  // I don't have to worry about modifying foo, because it's my local copy
  foo = ...;
  
  // Modifications to bar will be visible in the caller
  bar = ...;

  // I can't modify baz. If I really need to do that, I need to make a local copy
  SBFrame baz_copy = baz;
  baz_copy = ...;
}
labath: Ok, let's try this. My main point was that a function which receives an argument as a (non…
// Get the Lua callback
lua_pushlightuserdata(L, baton);
lua_gettable(L, LUA_REGISTRYINDEX);
labathUnsubmitted
Done
ReplyInline Actions

Is there a reason this function has to be in this file? It would be nice all the baton/registry handling was happening in the same place...

labath: Is there a reason this function has to be in this file? It would be nice all the baton/registry…
tammelaAuthorUnsubmitted
Done
ReplyInline Actions

My reasoning was to have the lua_pcall on the same function that pushes the Lua callback but I changed to your version as it's clearer who is handling the baton (Lua class)

tammela: My reasoning was to have the `lua_pcall` on the same function that pushes the Lua callback but…
// Push the Lua wrappers
labathUnsubmitted
Done
ReplyInline Actions

This name made sense for python, as the functions actually returned the wrappers. But here, the name makes it pretty unobvious that these functions actually push the object on the lua stack. It'd be better if this was called something like PushSBType or something

labath: This name made sense for python, as the functions actually returned the wrappers. But here, the…
PushSBClass(L, &sb_frame);
PushSBClass(L, &sb_bp_loc);
JDevlieghereUnsubmitted
Done
ReplyInline Actions

Might be nice to add a comment to explain the "magic values" here.

JDevlieghere: Might be nice to add a comment to explain the "magic values" here.
// Call into the Lua callback passing 'sb_frame' and 'sb_bp_loc'.
// Expects a boolean return.
if (lua_pcall(L, 2, 1, 0) != LUA_OK) {
llvm::Error E = llvm::make_error<llvm::StringError>(
llvm::formatv("{0}\n", lua_tostring(L, -1)),
llvm::inconvertibleErrorCode());
// Pop error message from the stack.
lua_pop(L, 1);
return std::move(E);
}
// Boolean return from the callback
bool stop = lua_toboolean(L, -1);
lua_pop(L, 1);
return stop;
}
%}

lldb/bindings/lua/lua.swig

/* /*
lldb.swig lldb.swig
This is the input file for SWIG, to create the appropriate C++ wrappers and This is the input file for SWIG, to create the appropriate C++ wrappers and
functions for various scripting languages, to enable them to call the functions for various scripting languages, to enable them to call the
liblldb Script Bridge functions. liblldb Script Bridge functions.
*/ */
%module lldb %module lldb
%include <std_string.i> %include <std_string.i>
%include "lua-typemaps.swig" %include "lua-typemaps.swig"
%include "macros.swig" %include "macros.swig"
%include "headers.swig" %include "headers.swig"
%{ %{
#include "llvm/Support/Error.h"
#include "llvm/Support/FormatVariadic.h"
#include "../bindings/lua/lua-swigsafecast.swig"
using namespace lldb_private; using namespace lldb_private;
using namespace lldb; using namespace lldb;
%} %}
%include "interfaces.swig" %include "interfaces.swig"
%include "lua-wrapper.swig"

lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h

//===-- ScriptInterpreterLua.h ----------------------------------*- C++ -*-===// //===-- ScriptInterpreterLua.h ----------------------------------*- C++ -*-===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#ifndef liblldb_Lua_h_ #ifndef liblldb_Lua_h_
#define liblldb_Lua_h_ #define liblldb_Lua_h_
#include "lldb/API/SBBreakpointLocation.h"
Lint: Pre-merge checks
Inline Actions

clang-tidy: error: 'lldb/API/SBBreakpointLocation.h' file not found [clang-diagnostic-error]
not useful
clang-tidy: error: 'lldb/API/SBBreakpointLocation.h' file not found [clang-diagnostic-error]
not useful

Lint: Pre-merge checks: clang-tidy: error: 'lldb/API/SBBreakpointLocation.h' file not found [clang-diagnostic-error]…
#include "lldb/API/SBFrame.h"
#include "lldb/lldb-types.h" #include "lldb/lldb-types.h"
#include "llvm/ADT/StringRef.h" #include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h" #include "llvm/Support/Error.h"
#include "lua.hpp" #include "lua.hpp"
#include <mutex> #include <mutex>
namespace lldb_private { namespace lldb_private {
extern "C" { extern "C" {
int luaopen_lldb(lua_State *L); int luaopen_lldb(lua_State *L);
} }
class Lua { class Lua {
JDevlieghereUnsubmitted
Done
ReplyInline Actions

Let's move this into the Lua class (and rename it to Callback) so we don't taint every CU that includes this file.

JDevlieghere: Let's move this into the `Lua` class (and rename it to `Callback`) so we don't taint every CU…
public: public:
Lua(); Lua();
~Lua(); ~Lua();
llvm::Error Run(llvm::StringRef buffer); llvm::Error Run(llvm::StringRef buffer);
llvm::Error RegisterBreakpointCallback(void *baton, const char *body);
llvm::Expected<bool>
CallBreakpointCallback(void *baton, lldb::StackFrameSP &stop_frame_sp,
lldb::BreakpointLocationSP &bp_loc_sp);
llvm::Error LoadModule(llvm::StringRef filename); llvm::Error LoadModule(llvm::StringRef filename);
llvm::Error ChangeIO(FILE *out, FILE *err); llvm::Error ChangeIO(FILE *out, FILE *err);
private: private:
lua_State *m_lua_state; lua_State *m_lua_state;
}; };
} // namespace lldb_private } // namespace lldb_private
#endif // liblldb_Lua_h_ #endif // liblldb_Lua_h_

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

//===-- Lua.cpp -----------------------------------------------------------===// //===-- Lua.cpp -----------------------------------------------------------===//
Lint: Lint
Inline Actions

clang-format suggested style edits found:

Lint: Lint: clang-format suggested style edits found:
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "Lua.h" #include "Lua.h"
#include "lldb/Host/FileSystem.h" #include "lldb/Host/FileSystem.h"
#include "lldb/Utility/FileSpec.h" #include "lldb/Utility/FileSpec.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FormatVariadic.h" #include "llvm/Support/FormatVariadic.h"
using namespace lldb_private; using namespace lldb_private;
using namespace lldb; using namespace lldb;
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
// Disable warning C4190: 'LLDBSwigPythonBreakpointCallbackFunction' has
// C-linkage specified, but returns UDT 'llvm::Expected<bool>' which is
// incompatible with C
#if _MSC_VER
#pragma warning (push)
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-#pragma warning (push)
-#pragma warning (disable : 4190)
+#pragma warning(push)
+#pragma warning(disable : 4190)
Lint: Pre-merge checks: clang-format: please reformat the code ``` -#pragma warning (push) -#pragma warning (disable…
#pragma warning (disable : 4190)
#endif
extern "C" llvm::Expected<bool>
LLDBSwigLuaBreakpointCallbackFunction(lua_State *L, void *baton,
lldb::StackFrameSP &stop_frame_sp,
lldb::BreakpointLocationSP &bp_loc_sp);
#if _MSC_VER
#pragma warning (pop)
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-#pragma warning (pop)
+#pragma warning(pop)
Lint: Pre-merge checks: clang-format: please reformat the code ``` -#pragma warning (pop) +#pragma warning(pop) ```
#endif
#pragma clang diagnostic pop
static int lldb_print(lua_State *L) { static int lldb_print(lua_State *L) {
int n = lua_gettop(L); int n = lua_gettop(L);
lua_getglobal(L, "io"); lua_getglobal(L, "io");
lua_getfield(L, -1, "stdout"); lua_getfield(L, -1, "stdout");
lua_getfield(L, -1, "write"); lua_getfield(L, -1, "write");
for (int i = 1; i <= n; i++) { for (int i = 1; i <= n; i++) {
lua_pushvalue(L, -1); // write() lua_pushvalue(L, -1); // write()
lua_pushvalue(L, -3); // io.stdout lua_pushvalue(L, -3); // io.stdout
Show All 27 Linesllvm::Error Lua::Run(llvm::StringRef buffer) {
llvm::Error e = llvm::make_error<llvm::StringError>( llvm::Error e = llvm::make_error<llvm::StringError>(
llvm::formatv("{0}\n", lua_tostring(m_lua_state, -1)), llvm::formatv("{0}\n", lua_tostring(m_lua_state, -1)),
llvm::inconvertibleErrorCode()); llvm::inconvertibleErrorCode());
// Pop error message from the stack. // Pop error message from the stack.
lua_pop(m_lua_state, 1); lua_pop(m_lua_state, 1);
return e; return e;
} }
llvm::Error Lua::RegisterBreakpointCallback(void *baton, const char *body) {
lua_pushlightuserdata(m_lua_state, baton);
const char *fmt_str = "return function(frame, bp_loc, ...) {0} end";
std::string func_str = llvm::formatv(fmt_str, body).str();
if (luaL_dostring(m_lua_state, func_str.c_str()) != LUA_OK) {
llvm::Error e = llvm::make_error<llvm::StringError>(
llvm::formatv("{0}\n", lua_tostring(m_lua_state, -1)),
llvm::inconvertibleErrorCode());
// Pop error message from the stack.
lua_pop(m_lua_state, 1);
labathUnsubmitted
Done
ReplyInline Actions

Shouldn't this also pop the baton ?

labath: Shouldn't this also pop the `baton` ?
return e;
}
lua_settable(m_lua_state, LUA_REGISTRYINDEX);
return llvm::Error::success();
}
llvm::Expected<bool>
Lua::CallBreakpointCallback(void *baton, lldb::StackFrameSP &stop_frame_sp,
lldb::BreakpointLocationSP &bp_loc_sp) {
labathUnsubmitted
Not Done
ReplyInline Actions

I'm confused. Why use lua to call a c callback, when you could just do calllback()?

labath: I'm confused. Why use lua to call a c callback, when you could just do `calllback()`?
tammelaAuthorUnsubmitted
Not Done
ReplyInline Actions

Some Lua API functions throw errors, if there's any it will abort() the program if no panic handler or protected call is provided.
To guarantee that the callback always runs in a protected environment and it has error handling, we do the above.
Delegating this to the callback itself makes it cumbersome to write.

tammela: Some Lua API functions throw errors, if there's any it will `abort()` the program if no panic…
labathUnsubmitted
Not Done
ReplyInline Actions

Aha, I see.

So, if I remember my lua correctly (I wrote a c++ lua wrapper once, but that was years ago..), whenever there's a lua exception inside this (c++) callback, lua will longjmp(3) back to the lua_pcall call on line 68, skipping any destructors that should normally be invoked. Is that so?

If that's the case, then I think this is a dangerous api, that should at the very least get a big fat warning, but that ideally shouldn't exist at all.

What's the part that makes delegating this to the callback "cumersome to write"? And why can't that be overcome by a suitable utility function which wraps lua_pcall or whatever else is needed?

The approach that we've chosen in python is to have very little code interacting with the python C API directly. Instead, code generally works with our C++ wrappers defined in PythonDataObject.h. These functions try to hide the python exception magic as much as possible, and present a c++-y version of the interface.

Now, since lua is stack-based, something like LuaDataObjects.h would probably not work. However, that doesn't meant we should give up on the c++ wrapper idea altogether. During the intitial reviews, my intention was for the Lua class to serve this purpose. I still think this can be achieved if we make the callback functions take Lua& instead of lua_State* as an argument, and then ensure the class contains whatever is needed to make the callbacks not cumerbsome to write.

labath: Aha, I see. So, if I remember my lua correctly (I wrote a c++ lua wrapper once, but that was…
tammelaAuthorUnsubmitted
Done
ReplyInline Actions

So, if I remember my lua correctly (I wrote a c++ lua wrapper once, but that was years ago..), whenever there's a lua exception inside this (c++) callback, lua will longjmp(3) back to the lua_pcall call on line 68, skipping any destructors that should normally be invoked. Is that so?

If that's the case, then I think this is a dangerous api, that should at the very least get a big fat warning, but that ideally shouldn't exist at all.

You are right. This escaped me completely.
Lua can be compiled to use try/catch instead of longjmp, but that is an exception (no pun intended). Since we rely on the host's library, we need to assume that it's using longjmp.

What's the part that makes delegating this to the callback "cumersome to write"? And why can't that be overcome by a suitable utility function which wraps lua_pcall or whatever else is needed?

The approach that we've chosen in python is to have very little code interacting with the python C API directly. Instead, code generally works with our C++ wrappers defined in PythonDataObject.h. These functions try to hide the python exception magic as much as possible, and present a c++-y version of the interface.

Now, since lua is stack-based, something like LuaDataObjects.h would probably not work. However, that doesn't meant we should give up on the c++ wrapper idea altogether. During the intitial reviews, my intention was for the Lua class to serve this purpose. I still think this can be achieved if we make the callback functions take Lua& instead of lua_State* as an argument, and then ensure the class contains whatever is needed to make the callbacks not cumerbsome to write.

Any C++ code that runs in a lua_pcall seems to face this issue. I checked a very complete C++ Lua wrapper called sol2 and they seem to have the same issue. I don't think there's a way out of it except code discipline.

Lua provides lua_atpanic as a way to recover from unprotected throws. Perhaps we can leverage this to throw an exception, guaranteeing stack unwinding and avoiding a call to abort(). We would then let the callback run unprotected and delegate any calls to lua_pcall to the callback.

Changes proposed:

  • Register the panic function on Lua ctor
  • Let the Callback run unprotected
  • Wrap the Callback call in a try catch block
  • Change the Callback signature to receive Lua& and return llvm::Error

It looks like it ticks all goals.

  • Unprotected Lua errors do not cause lldb to abort.
  • Stack unwinding is guaranteed for C++ code interacting with the Lua state.
  • Errors are propagated
tammela: > So, if I remember my lua correctly (I wrote a c++ lua wrapper once, but that was years ago..)…
labathUnsubmitted
Not Done
ReplyInline Actions

I'll move this out-of-line, as it's getting rather long.

labath: I'll move this out-of-line, as it's getting rather long.
return LLDBSwigLuaBreakpointCallbackFunction(m_lua_state, baton,
stop_frame_sp, bp_loc_sp);
}
llvm::Error Lua::LoadModule(llvm::StringRef filename) { llvm::Error Lua::LoadModule(llvm::StringRef filename) {
FileSpec file(filename); FileSpec file(filename);
if (!FileSystem::Instance().Exists(file)) { if (!FileSystem::Instance().Exists(file)) {
return llvm::make_error<llvm::StringError>("invalid path", return llvm::make_error<llvm::StringError>("invalid path",
llvm::inconvertibleErrorCode()); llvm::inconvertibleErrorCode());
} }
ConstString module_extension = file.GetFileNameExtension(); ConstString module_extension = file.GetFileNameExtension();
▲ Show 20 Lines • Show All 52 Lines • Show Last 20 Lines

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h

//===-- ScriptInterpreterLua.h ----------------------------------*- C++ -*-===// //===-- ScriptInterpreterLua.h ----------------------------------*- C++ -*-===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#ifndef liblldb_ScriptInterpreterLua_h_ #ifndef liblldb_ScriptInterpreterLua_h_
#define liblldb_ScriptInterpreterLua_h_ #define liblldb_ScriptInterpreterLua_h_
#include "lldb/Interpreter/ScriptInterpreter.h" #include "lldb/Interpreter/ScriptInterpreter.h"
Lint: Pre-merge checks
Inline Actions

clang-tidy: error: 'lldb/Interpreter/ScriptInterpreter.h' file not found [clang-diagnostic-error]
not useful
clang-tidy: error: 'lldb/Interpreter/ScriptInterpreter.h' file not found [clang-diagnostic-error]
not useful

Lint: Pre-merge checks: clang-tidy: error: 'lldb/Interpreter/ScriptInterpreter.h' file not found [clang-diagnostic…
#include "lldb/Utility/Status.h"
#include "lldb/lldb-enumerations.h"
namespace lldb_private { namespace lldb_private {
class Lua; class Lua;
class ScriptInterpreterLua : public ScriptInterpreter { class ScriptInterpreterLua : public ScriptInterpreter {
public: public:
class CommandDataLua : public BreakpointOptions::CommandData {
public:
CommandDataLua() : BreakpointOptions::CommandData() {
interpreter = lldb::eScriptLanguageLua;
}
};
ScriptInterpreterLua(Debugger &debugger); ScriptInterpreterLua(Debugger &debugger);
~ScriptInterpreterLua() override; ~ScriptInterpreterLua() override;
bool ExecuteOneLine( bool ExecuteOneLine(
llvm::StringRef command, CommandReturnObject *result, llvm::StringRef command, CommandReturnObject *result,
const ExecuteScriptOptions &options = ExecuteScriptOptions()) override; const ExecuteScriptOptions &options = ExecuteScriptOptions()) override;
Show All 10 Linespublic:
static void Terminate(); static void Terminate();
static lldb::ScriptInterpreterSP CreateInstance(Debugger &debugger); static lldb::ScriptInterpreterSP CreateInstance(Debugger &debugger);
static lldb_private::ConstString GetPluginNameStatic(); static lldb_private::ConstString GetPluginNameStatic();
static const char *GetPluginDescriptionStatic(); static const char *GetPluginDescriptionStatic();
static bool BreakpointCallbackFunction(void *baton,
StoppointCallbackContext *context,
lldb::user_id_t break_id,
lldb::user_id_t break_loc_id);
// PluginInterface protocol // PluginInterface protocol
lldb_private::ConstString GetPluginName() override; lldb_private::ConstString GetPluginName() override;
uint32_t GetPluginVersion() override; uint32_t GetPluginVersion() override;
Lua &GetLua(); Lua &GetLua();
llvm::Error EnterSession(lldb::user_id_t debugger_id); llvm::Error EnterSession(lldb::user_id_t debugger_id);
llvm::Error LeaveSession(); llvm::Error LeaveSession();
Status SetBreakpointCommandCallback(BreakpointOptions *bp_options,
const char *command_body_text) override;
private: private:
std::unique_ptr<Lua> m_lua; std::unique_ptr<Lua> m_lua;
bool m_session_is_active = false; bool m_session_is_active = false;
}; };
} // namespace lldb_private } // namespace lldb_private
#endif // liblldb_ScriptInterpreterLua_h_ #endif // liblldb_ScriptInterpreterLua_h_

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

//===-- ScriptInterpreterLua.cpp ------------------------------------------===// //===-- ScriptInterpreterLua.cpp ------------------------------------------===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "ScriptInterpreterLua.h" #include "ScriptInterpreterLua.h"
#include "Lua.h" #include "Lua.h"
#include "lldb/Breakpoint/StoppointCallbackContext.h"
#include "lldb/Core/Debugger.h" #include "lldb/Core/Debugger.h"
#include "lldb/Core/PluginManager.h" #include "lldb/Core/PluginManager.h"
#include "lldb/Core/StreamFile.h" #include "lldb/Core/StreamFile.h"
#include "lldb/Interpreter/CommandReturnObject.h" #include "lldb/Interpreter/CommandReturnObject.h"
#include "lldb/Target/ExecutionContext.h"
#include "lldb/Utility/Stream.h" #include "lldb/Utility/Stream.h"
#include "lldb/Utility/StringList.h" #include "lldb/Utility/StringList.h"
#include "lldb/Utility/Timer.h" #include "lldb/Utility/Timer.h"
#include "llvm/Support/FormatAdapters.h" #include "llvm/Support/FormatAdapters.h"
#include <memory>
using namespace lldb; using namespace lldb;
using namespace lldb_private; using namespace lldb_private;
LLDB_PLUGIN_DEFINE(ScriptInterpreterLua) LLDB_PLUGIN_DEFINE(ScriptInterpreterLua)
class IOHandlerLuaInterpreter : public IOHandlerDelegate, class IOHandlerLuaInterpreter : public IOHandlerDelegate,
public IOHandlerEditline { public IOHandlerEditline {
▲ Show 20 Lines • Show All 142 Lines • ▼ Show 20 Linesllvm::Error ScriptInterpreterLua::LeaveSession() {
llvm::StringRef str = "lldb.debugger = nil; " llvm::StringRef str = "lldb.debugger = nil; "
"lldb.target = nil; " "lldb.target = nil; "
"lldb.process = nil; " "lldb.process = nil; "
"lldb.thread = nil; " "lldb.thread = nil; "
"lldb.frame = nil"; "lldb.frame = nil";
return m_lua->Run(str); return m_lua->Run(str);
} }
bool ScriptInterpreterLua::BreakpointCallbackFunction(
void *baton, StoppointCallbackContext *context, user_id_t break_id,
user_id_t break_loc_id) {
assert(context);
ExecutionContext exe_ctx(context->exe_ctx_ref);
Target *target = exe_ctx.GetTargetPtr();
if (target == nullptr)
return true;
StackFrameSP stop_frame_sp(exe_ctx.GetFrameSP());
BreakpointSP breakpoint_sp = target->GetBreakpointByID(break_id);
BreakpointLocationSP bp_loc_sp(breakpoint_sp->FindLocationByID(break_loc_id));
Debugger &debugger = target->GetDebugger();
ScriptInterpreterLua *lua_interpreter = static_cast<ScriptInterpreterLua *>(
labathUnsubmitted
Not Done
ReplyInline Actions

How is lua_interpreter different from this?

labath: How is `lua_interpreter` different from `this`?
tammelaAuthorUnsubmitted
Not Done
ReplyInline Actions

Are you asking why I didn't go for m_script_interpreter?

This function is static (on the class declaration), perhaps it's clearer a static keyword here as well?

tammela: Are you asking why I didn't go for `m_script_interpreter`? This function is static (on the…
labathUnsubmitted
Not Done
ReplyInline Actions

I just missed the fact that this is a static function (I hate these static baton functions in lldb). Putting static on an out-of-line member function definition is not valid c++.

However, this does raise a different issue. Debugger::GetScriptInterpreter will return the default script interpreter, and that may not be the lua one. I think you should pass eScriptLanguageLua here explicitly.

labath: I just missed the fact that this is a static function (I hate these static baton functions in…
debugger.GetScriptInterpreter(true, eScriptLanguageLua));
Lua &lua = lua_interpreter->GetLua();
llvm::Expected<bool> BoolOrErr =
lua.CallBreakpointCallback(baton, stop_frame_sp, bp_loc_sp);
if (llvm::Error E = BoolOrErr.takeError()) {
debugger.GetErrorStream() << toString(std::move(E));
return true;
}
return *BoolOrErr;
labathUnsubmitted
Done
ReplyInline Actions
if (llvm::Error E = lua.Run(callback)) {
  debugger.GetErrorStream() << toString(std::move(E));
  stop = true;
}

would be less callback-y.

labath: ``` if (llvm::Error E = lua.Run(callback)) { debugger.GetErrorStream() << toString(std::move…
}
Status ScriptInterpreterLua::SetBreakpointCommandCallback(
BreakpointOptions *bp_options, const char *command_body_text) {
Status error;
auto data_up = std::make_unique<CommandDataLua>();
error = m_lua->RegisterBreakpointCallback(data_up.get(), command_body_text);
if (error.Fail())
return error;
auto baton_sp =
std::make_shared<BreakpointOptions::CommandBaton>(std::move(data_up));
bp_options->SetCallback(ScriptInterpreterLua::BreakpointCallbackFunction,
baton_sp);
return error;
}
lldb::ScriptInterpreterSP lldb::ScriptInterpreterSP
ScriptInterpreterLua::CreateInstance(Debugger &debugger) { ScriptInterpreterLua::CreateInstance(Debugger &debugger) {
return std::make_shared<ScriptInterpreterLua>(debugger); return std::make_shared<ScriptInterpreterLua>(debugger);
} }
lldb_private::ConstString ScriptInterpreterLua::GetPluginNameStatic() { lldb_private::ConstString ScriptInterpreterLua::GetPluginNameStatic() {
static ConstString g_name("script-lua"); static ConstString g_name("script-lua");
return g_name; return g_name;
Show All 13 Lines

lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test

  • This file was added.
# REQUIRES: lua
# RUN: echo "int main() { return 0; }" | %clang_host -x c - -o %t
# RUN: %lldb -s %s --script-language lua %t 2>&1 | FileCheck %s
b main
labathUnsubmitted
Done
ReplyInline Actions

Print something more unique so this doesn't match accidentally?

labath: Print something more unique so this doesn't match accidentally?
breakpoint command add -s lua -o 'print(frame)'
labathUnsubmitted
Done
ReplyInline Actions

Could we also have a test where calling the callback raises an error? And for the different values of the "stop" argument that can be returned from the callback? And when compiling the callback expression results in an error?

labath: Could we also have a test where calling the callback raises an error? And for the different…
tammelaAuthorUnsubmitted
Done
ReplyInline Actions

Apparently when the user sets the breakpoint and it fails to compile the code lldb will not report any errors and it will fail silently. I will address this particular test case in another patch since it affects Python as well.

tammela: Apparently when the user sets the breakpoint and it fails to compile the code lldb will not…
labathUnsubmitted
Not Done
ReplyInline Actions

Sounds good.

labath: Sounds good.
run
# CHECK: frame #0
labathUnsubmitted
Not Done
ReplyInline Actions

"frame #0" will also get printed if the process stops for any reason. I'd suggest printing a completely random string instead (print("bacon"), for example)

labath: "frame #0" will also get printed if the process stops for any reason. I'd suggest printing a…

lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp

//===-- LuaTests.cpp ------------------------------------------------------===// //===-- LuaTests.cpp ------------------------------------------------------===//
Lint: Lint
Inline Actions

clang-format suggested style edits found:

Lint: Lint: clang-format suggested style edits found:
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "Plugins/ScriptInterpreter/Lua/Lua.h" #include "Plugins/ScriptInterpreter/Lua/Lua.h"
Lint: Pre-merge checks
Inline Actions

clang-tidy: error: 'Plugins/ScriptInterpreter/Lua/Lua.h' file not found [clang-diagnostic-error]
not useful

Lint: Pre-merge checks: clang-tidy: error: 'Plugins/ScriptInterpreter/Lua/Lua.h' file not found [clang-diagnostic…
#include "gtest/gtest.h" #include "gtest/gtest.h"
using namespace lldb_private; using namespace lldb_private;
extern "C" int luaopen_lldb(lua_State *L) { return 0; } extern "C" int luaopen_lldb(lua_State *L) { return 0; }
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wreturn-type-c-linkage"
// Disable warning C4190: 'LLDBSwigPythonBreakpointCallbackFunction' has
// C-linkage specified, but returns UDT 'llvm::Expected<bool>' which is
// incompatible with C
#if _MSC_VER
#pragma warning (push)
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-#pragma warning (push)
-#pragma warning (disable : 4190)
+#pragma warning(push)
+#pragma warning(disable : 4190)
Lint: Pre-merge checks: clang-format: please reformat the code ``` -#pragma warning (push) -#pragma warning (disable…
#pragma warning (disable : 4190)
#endif
extern "C" llvm::Expected<bool>
LLDBSwigLuaBreakpointCallbackFunction(lua_State *L, void *baton,
lldb::StackFrameSP &stop_frame_sp,
lldb::BreakpointLocationSP &bp_loc_sp) {
return false;
}
#if _MSC_VER
#pragma warning (pop)
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-#pragma warning (pop)
+#pragma warning(pop)
Lint: Pre-merge checks: clang-format: please reformat the code ``` -#pragma warning (pop) +#pragma warning(pop) ```
#endif
#pragma clang diagnostic pop
TEST(LuaTest, RunValid) { TEST(LuaTest, RunValid) {
Lua lua; Lua lua;
llvm::Error error = lua.Run("foo = 1"); llvm::Error error = lua.Run("foo = 1");
EXPECT_FALSE(static_cast<bool>(error)); EXPECT_FALSE(static_cast<bool>(error));
} }
TEST(LuaTest, RunInvalid) { TEST(LuaTest, RunInvalid) {
Lua lua; Lua lua;
llvm::Error error = lua.Run("nil = foo"); llvm::Error error = lua.Run("nil = foo");
EXPECT_TRUE(static_cast<bool>(error)); EXPECT_TRUE(static_cast<bool>(error));
EXPECT_EQ(llvm::toString(std::move(error)), EXPECT_EQ(llvm::toString(std::move(error)),
"[string \"buffer\"]:1: unexpected symbol near 'nil'\n"); "[string \"buffer\"]:1: unexpected symbol near 'nil'\n");
} }