Are these still relevant to the code that was landed recently?
For this one, please wait @JDevlieghere's review.

You will have to rebase to main.

Just one last thing and I think we are done!

Hi Siger,

Trying to run the tests in my system failed with the following:

LGTM

LGTM

I think we are getting there, just some more details

Missing test cases!
Either a script that test it all, individual unit tests or a combination of both.

I tested locally and it seems fine!

Clang format complained a bit, please fix those.

@JDevlieghere Addressed all your comments.

Rebasing and unwrapping the SBStructuredData test case

Typos

Addressing comments

In D94937#2511222, @JDevlieghere wrote:

I replied before I actually tried to understand what your'e trying to achieve, and it's still not entirely clear to me. I think what you need is something similar like python-typemaps.swig which helps swig understand char** types and such.

For context, originally SWIG wasn't great at handling std::strings (which has since been fixed) and the designers of the SB API were worried about the ABI stability of the std::string class. None of these are concerns anymore today, but the ABI being stable means we're stuck with it for now. If we ever do an SB API v2, this is definitely one of the things we'd fix, but currently there are no plans for that.

In D94937#2505676, @JDevlieghere wrote:

We guarantee that the SB API is ABI stable so you cannot change the signature of existing functions. Would an overload do the trick?

In D94937#2505676, @JDevlieghere wrote:

We guarantee that the SB API is ABI stable so you cannot change the signature of existing functions. Would an overload do the trick?

Removing spurious line

Addressing comment

ping

ping

Addressing comments

Addressing comments

Addressing comments

Addressing comments

In D92249#2422205, @labath wrote:

In D92249#2421639, @JDevlieghere wrote:

It appears there are many similar calls in this file, can you update those as well?

If there are many of these calls, adding a static helper function for this purpose might be a good idea as well...

Addressing review comments

Remove unnecessary wrappers/checks

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.

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

Redesign of the API

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?

Adressing comments

Clean up includes

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?

Add test case, fix line column

Add test case