Page MenuHomePhabricator

[lldb/lua] Support external breakpoint callback
Needs RevisionPublic

Authored by siger-young on Dec 17 2021, 2:07 AM.

Details

Summary

This commit supports registering breakpoint callback when using
liblldb in Lua. (able to SetScriptCallback(function(a, b, ...) end))

Diff Detail

Event Timeline

siger-young requested review of this revision.Dec 17 2021, 2:07 AM
siger-young created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2021, 2:07 AM
siger-young edited the summary of this revision. (Show Details)Dec 17 2021, 2:08 AM
siger-young edited the summary of this revision. (Show Details)Dec 17 2021, 2:14 AM

Remove "return" to prevent memory leakage.

labath requested changes to this revision.Dec 17 2021, 2:41 AM
labath added a subscriber: labath.

I actually quite like this, but we should figure out a way to do it while respecting our SB contracts. See inline comments for details.

lldb/bindings/lua/lua-wrapper.swig
119–124

I have (just yesterday) reformatted these files to follow the normal llvm/lldb style. Could you please rebase and reformat the patch so we don't profilerate this weirdness.

lldb/include/lldb/API/SBBreakpointOptionCommon.h
15

The reason this file was in the source folder is because lldb public header cannot depend the internal ones. We'll need to figure out a different way to do that.

I guess that means placing these definitions somewhere where they would be accessible by the swig code, but still inaccessible to the outside world. Would any of the FileSP tricks we do in the python code help?

lldb/include/lldb/API/SBDefines.h
97–99

What's the reason for changing this?

If this were a new API, then I think I might agree with having an SBFrame here, but we'd need a very good reason to change this after the fact. I would assume the sb_frame object can be accessed as thread.GetFrameAtIndex(0), can it not?

This revision now requires changes to proceed.Dec 17 2021, 2:41 AM

I don't know if you've seen this but we have some description of it here https://lldb.llvm.org/design/sbapi.html. The gist of it is:

  • be backward compatible
  • don't depend on other stuff

I don't know if you've seen this but we have some description of it here https://lldb.llvm.org/design/sbapi.html. The gist of it is:

  • be backward compatible
  • don't depend on other stuff

Thanks, I got it. I will stick to these rules.

jingham added a subscriber: jingham.EditedDec 17 2021, 11:04 AM

I don't know if you've seen this but we have some description of it here https://lldb.llvm.org/design/sbapi.html. The gist of it is:

  • be backward compatible
  • don't depend on other stuff

Thanks, I got it. I will stick to these rules.

It's fine to add an API if we decide the one we had originally was unwieldily. I have no idea why one of us (probably me) thought breakpoint callbacks needed a process & thread. But since this is a typedef, you'd have to make a second name, which is probably too awkward.

lldb/include/lldb/API/SBBreakpointOptionCommon.h
17

It's odd to have something called lldb::CallbackData, but have it be specific to breakpoints. We probably need to do the same thing for Watchpoints, for instance, but either there should be a common class for both watchpoints, or they should be distinguishable by name. And other things might have callbacks.