This is an archive of the discontinued LLVM Phabricator instance.

Paths

Table of Contentst

-
lldb/
-
bindings/lua/
-
lua/
-
lua-swigsafecast.swig
5/7
lua-wrapper.swig
-
lua.swig
-
source/Plugins/ScriptInterpreter/Lua/
-
Plugins/
-
ScriptInterpreter/
-
Lua/
1/1
Lua.h
2/6
Lua.cpp
-
ScriptInterpreterLua.h
1/4
ScriptInterpreterLua.cpp
-
test/Shell/ScriptInterpreter/Lua/
-
Shell/
-
ScriptInterpreter/
-
Lua/
3/5
breakpoint_oneline_callback.test
-
unittests/ScriptInterpreter/Lua/
-
ScriptInterpreter/
-
Lua/
-
LuaTests.cpp

Differential D91508

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

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

Download Raw Diff

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

Repository: rG LLVM Github Monorepo

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

@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

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.

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?

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.

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

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?

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

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.

Redesign of the API

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

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.

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.

Remove unnecessary wrappers/checks

Harbormaster completed remote builds in B80009: Diff 307440.Nov 24 2020, 1:44 PM

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
22–23	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...
26–27	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
6	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?

Addressing review comments

lldb/bindings/lua/lua-wrapper.swig
22–23	`SBFrame` and `SBBreakpointLocation` only provide copy constructors. I can't see the difference between the two strategies, could you elaborate?
26–27	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
6	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
22–23	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
6	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)

Addressing comments

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.

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

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?

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.

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

Path

Size

lldb/

bindings/

lua/

lua-swigsafecast.swig

15 lines

lua-wrapper.swig

46 lines

lua.swig

4 lines

source/

Plugins/

ScriptInterpreter/

Lua/

Lua.h

6 lines

Lua.cpp

48 lines

ScriptInterpreterLua.h

17 lines

ScriptInterpreterLua.cpp

46 lines

test/

Shell/

ScriptInterpreter/

Lua/

breakpoint_oneline_callback.test

18 lines

unittests/

ScriptInterpreter/

Lua/

LuaTests.cpp

24 lines

Diff 308349

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,
				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);

				// Push the Lua wrappers
				labathUnsubmitted Not Done Reply Inline 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 Reply Inline 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 Reply Inline 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…
				PushSBClass(L, &sb_frame);
				PushSBClass(L, &sb_bp_loc);

				// Call into the Lua callback passing 'sb_frame' and 'sb_bp_loc'.
				labathUnsubmitted Done Reply Inline 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 Reply Inline 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…
				// Expects a boolean return.
				labathUnsubmitted Done Reply Inline 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…
				if (lua_pcall(L, 2, 1, 0) != LUA_OK) {
				llvm::Error E = llvm::make_error<llvm::StringError>(
				JDevlieghereUnsubmitted Done Reply Inline 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.
				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"
				#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 Reply Inline 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 -----------------------------------------------------------===//
//		//
// 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)
		#pragma warning (disable : 4190)
		#endif

		extern "C" llvm::Expected<bool>
		LLDBSwigLuaBreakpointCallbackFunction(lua_State *L,
		lldb::StackFrameSP stop_frame_sp,
		lldb::BreakpointLocationSP bp_loc_sp);

		#if _MSC_VER
		#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 Lines	llvm::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, 2);
		labathUnsubmitted Done Reply Inline 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 Reply Inline 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 Reply Inline 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 Reply Inline 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 Reply Inline 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 Reply Inline 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.
		lua_pushlightuserdata(m_lua_state, baton);
		lua_gettable(m_lua_state, LUA_REGISTRYINDEX);
		return LLDBSwigLuaBreakpointCallbackFunction(m_lua_state, 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"
		#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 Lines	public:
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 Lines	llvm::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 Reply Inline Actions How is `lua_interpreter` different from `this`? labath: How is `lua_interpreter` different from `this`?
		tammelaAuthorUnsubmitted Not Done Reply Inline 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 Reply Inline 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 Reply Inline 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 Reply Inline 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 'return false'
				run
				labathUnsubmitted Done Reply Inline 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 Reply Inline 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 Reply Inline Actions Sounds good. labath: Sounds good.
				# CHECK: Process {{[0-9]+}} exited with status = 0
				breakpoint command add -s lua -o 'print(bacon)'
				run
				# CHECK: bacon
				# CHECK: Process {{[0-9]+}} exited with status = 0
				labathUnsubmitted Not Done Reply Inline 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…
				breakpoint command add -s lua -o "return true"
				run
				# CHECK: Process {{[0-9]+}} stopped
				breakpoint command add -s lua -o 'error("my error message")'
				run
				# CHECK: my error message
				# CHECK: Process {{[0-9]+}} stopped

lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp

	//===-- LuaTests.cpp ------------------------------------------------------===//			//===-- LuaTests.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 "Plugins/ScriptInterpreter/Lua/Lua.h"			#include "Plugins/ScriptInterpreter/Lua/Lua.h"
	#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)
				#pragma warning (disable : 4190)
				#endif

				extern "C" llvm::Expected<bool>
				LLDBSwigLuaBreakpointCallbackFunction(lua_State *L,
				lldb::StackFrameSP stop_frame_sp,
				lldb::BreakpointLocationSP bp_loc_sp) {
				return false;
				}

				#if _MSC_VER
				#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");
	}			}

This is an archive of the discontinued LLVM Phabricator instance.

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

Details

Diff Detail

Event Timeline

Revision Contents

Diff 308349

lldb/bindings/lua/lua-swigsafecast.swig

lldb/bindings/lua/lua-wrapper.swig

lldb/bindings/lua/lua.swig

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

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

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

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

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

lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp

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