[Lldb-commits] [PATCH] D115926: [lldb/lua] Support external breakpoint callback

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: lldb/bindings/lua/lua-wrapper.swig:119-124
+bool SBBreakpointHitCallbackLua(
+   void *baton,
+   SBFrame &sb_frame,
+   SBBreakpointLocation &sb_bp_loc
+)
+{
----------------
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.


================
Comment at: lldb/include/lldb/API/SBBreakpointOptionCommon.h:14
+#include "lldb/API/SBDefines.h"
+#include "lldb/Utility/Baton.h"
+
----------------
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?


================
Comment at: lldb/include/lldb/API/SBDefines.h:97-98
 
-typedef bool (*SBBreakpointHitCallback)(void *baton, SBProcess &process,
-                                        SBThread &thread,
-                                        lldb::SBBreakpointLocation &location);
+typedef bool (*SBBreakpointHitCallback)(void *baton, SBFrame &sb_frame,
+                                        SBBreakpointLocation &sb_bp_loc);
 }
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115926/new/

https://reviews.llvm.org/D115926