[lldb/bindings] Expose the progress reporting machinery to the SWIG interface

[mib]

Summary

This patch defines the SBDebugger::eBroadcastBitProgress enum in the SWIG
interface and exposes the SBDebugger::{GetProgressFromEvent,GetBroadcaster}
methods as well.

This allows to exercise the API from the script interpreter using python.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

Removing the enum from SBDebugger is an ABI breaking change. I think this has been in tree for a while, so if we shipped this like this in the last release, we cannot guarantee that this won't break anyone. Can we avoid the issue by defining the enum in the interface file?

[mib]

In D120100#3331141, @JDevlieghere wrote:

Removing the enum from SBDebugger is an ABI breaking change. I think this has been in tree for a while, so if we shipped this like this in the last release, we cannot guarantee that this won't break anyone. Can we avoid the issue by defining the enum in the interface file?

Please, correct me if I'm wrong but in my understanding, this is partially implemented from the ABI standpoint as it's not defined in the SWIG interface file. Even though it's defined in the C++ liblldb, I don't think it reaches a bigger audience than the Python lldb module. Defining it in the interface file will increase the maintenance burden so I think it's reasonable to "break" the ABI in this case. Do you see any objection @clayborg ?

[clayborg]

So the progress bits are defined inside the class the broadcasts the events to make ownership clear. And we can not break the public API, so we unfortunately can't do this. We don't change these bits often, so although it is kind of a pain, it should remain as it is from the public API standpoint. Also the naming convention of the enumeration in the lldb-enumerations.h loses the ownership and the name would have to be modified to something like eBroadcastBitDebuggerProgress, but since we can't move it due to the API, that is moot.

[clayborg]

We have been quite stable on our public API over the years which allows people to create native tools that link against our public API and we don't regress it so things always compile or would actually still run against an older LLDB library.

In D120100#3331209, @mib wrote:

In D120100#3331141, @JDevlieghere wrote:

Removing the enum from SBDebugger is an ABI breaking change. I think this has been in tree for a while, so if we shipped this like this in the last release, we cannot guarantee that this won't break anyone. Can we avoid the issue by defining the enum in the interface file?

Please, correct me if I'm wrong but in my understanding, this is partially implemented from the ABI standpoint as it's not defined in the SWIG interface file. Even though it's defined in the C++ liblldb, I don't think it reaches a bigger audience than the Python lldb module.

lldb-vscode.cpp links against it, so it is being used. For Swift support, we rely on the fact that an binary like lldb-rpc-server or lldb-vscode can target newer LLDB libraries both from a building against headers perspective and also from a newer executable running with an older LLDB library standpoint.

Defining it in the interface file will increase the maintenance burden so I think it's reasonable to "break" the ABI in this case. Do you see any objection @clayborg ?

So yes, in order to keep our API stable I object.

[labath]

I'm not sure how we ended up with the .i files in the first place, but maybe a good solution to this problem would be to ditch those and generate bindings from .h files directly.

It may mean that we need to put #ifndef SWIG around some code, but I'd say that beats maintaining two separate copies of the interface. (Also, the ifdefs are a pretty good way to document the bits of the "public" API that we do not wish to make available to scripts.)

[JDevlieghere]

In D120100#3331656, @labath wrote:

I'm not sure how we ended up with the .i files in the first place, but maybe a good solution to this problem would be to ditch those and generate bindings from .h files directly.

It may mean that we need to put #ifndef SWIG around some code, but I'd say that beats maintaining two separate copies of the interface. (Also, the ifdefs are a pretty good way to document the bits of the "public" API that we do not wish to make available to scripts.)

I've considered that in the past, but with the docstrings and the inline python code for things like property definitions, I'm not sure it's worth it. I assume we could move the majority of those out of the actual header and do a textual include, but then you're still on the hook for keeping that file in sync.

[mib]

Instead of moving the enum to lldb-enumeration, define it in the SBDebugger SWIG interface file and add a test to exercise the API.

[JDevlieghere]

LGTM

[labath]

In D120100#3332433, @JDevlieghere wrote:

In D120100#3331656, @labath wrote:

I'm not sure how we ended up with the .i files in the first place, but maybe a good solution to this problem would be to ditch those and generate bindings from .h files directly.

It may mean that we need to put #ifndef SWIG around some code, but I'd say that beats maintaining two separate copies of the interface. (Also, the ifdefs are a pretty good way to document the bits of the "public" API that we do not wish to make available to scripts.)

I've considered that in the past, but with the docstrings and the inline python code for things like property definitions, I'm not sure it's worth it. I assume we could move the majority of those out of the actual header and do a textual include, but then you're still on the hook for keeping that file in sync.

Yeah, I forgot about how much code we have there. Putting the docstrings into a separate file is fairly easy, but maybe it would actually be fine to put it into the main file directly -- it is still a documentation of what those methods are supposed to do, even if the syntax is funny. And it would avoid the current situation, where some of our SB methods have (doxygen) documentation in the .h files, and others have swig documentation in .i.

But even in a separate, I think it would be better since in would be "just" the documentation, at least we wouldn't have to maintain two copies of the c++ interfaces.

lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py

22–23

There is a race here, where some (or even all) of the progress events will be generated before the listener actually starts listening for them. You need to ensure that the listener setup happens-before the events are generated. The easiest way to achieve that is to set up the listening on the main thread.

38

will anyone be able to read the results provided through the by-ref arguments? That seems like it would be worth testing.

It might also be worth adapting this interface to be more pythonic (having the python wrapper return a tuple instead of by-ref args).

42

Why?

[mib]

lldb/bindings/interface/SBDebugger.i

126–129

@labath In my understanding, these tell SWIG to return return the reference values as a tuple. They are pre-pending the function return value. I'll write a follow-up patch to test it.

lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py

22–23

Thanks for catching that, I'll update the test.

42

So far, I know that Linux and macOS report some basic progress events when reading DWARF, but I'm not sure other platforms do, so I might be able to only skip windows.

[labath]

lldb/bindings/interface/SBDebugger.i

126–129

Ah, that's cool. So it takes the in value as a regular argument, and provides the out value in the return value.

I don't suppose there's any chance of making this an out-only argument, to avoid the dummy input argument ?

[mib]

@labath I addressed your comments in D120284 :)

lldb/bindings/interface/SBDebugger.i

126–129

I don't think SWIG offers such thing unfortunately.

[labath]

lldb/bindings/interface/SBDebugger.i

126–129

What about this ?

The following typemap rules tell SWIG that pointer is the output value of a function. When used, you do not need to supply the argument when calling the function. <...>

The article speaks about pointer arguments, but in my (simple) test, it seemed to work fine for references as well...

lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py

42

The original progress patch (D97739) has a test running on linux, so I'd expect this should work there too. The test in that patch is disabled on windows, but that might be due to some vscode issues. I don't see why there should be any differences between windows and linux in terms of the dwarf events generated -- both of them don't use accelerator tables by default.

lldb/bindings/interface/SBDebugger.i

to do this is: ::
     target = debugger.CreateTarget('')
     error = lldb.SBError()
     process = target.AttachToProcessWithName(debugger.GetListener(), 'PROCESS_NAME', False, error)
 
 or the equivalent arguments for :py:class:`SBTarget.AttachToProcessWithID` .") SBDebugger;
 class SBDebugger
 {
 public:
+    enum
+    {
+        eBroadcastBitProgress = (1 << 0)
+    };
+
+
+    %apply uint64_t& INOUT { uint64_t& progress_id };
+    %apply uint64_t& INOUT { uint64_t& completed };
+    %apply uint64_t& INOUT { uint64_t& total };
+    %apply bool& INOUT { bool& is_debugger_specific };
+    static const char *GetProgressFromEvent(const lldb::SBEvent &event,
+                                        uint64_t &progress_id,
+                                        uint64_t &completed, uint64_t &total,
+                                        bool &is_debugger_specific);
+
+    SBBroadcaster GetBroadcaster();
 
     static void
     Initialize();
 
     static SBError
     InitializeWithErrorHandling();
 
     static void

lldb/test/API/functionalities/progress_reporting/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py

+"""
+Test that we are able to broadcast and receive progress events from lldb
+"""
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+import threading
+
+class TestProgressReporting(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    eBroadcastBitStopProgressThread = (1 << 0)
+
+    def setUp(self):
+        TestBase.setUp(self)
+        self.progress_events = []
+
+    def fetch_events(self, test_broadcaster):
+        listener = lldb.SBListener("lldb.progress.listener")
+        listener.StartListeningForEvents(test_broadcaster,
+                                         self.eBroadcastBitStopProgressThread)
+
+        progress_broadcaster = self.dbg.GetBroadcaster()
+        progress_broadcaster.AddListener(listener, lldb.SBDebugger.eBroadcastBitProgress)
+
+        event = lldb.SBEvent()
+
+        done = False
+        while not done:
+            if listener.WaitForEvent(1, event):
+                event_mask = event.GetType();
+                if event.BroadcasterMatchesRef(test_broadcaster):
+                    if event_mask & self.eBroadcastBitStopProgressThread:
+                        done = True;
+                elif event.BroadcasterMatchesRef(progress_broadcaster):
+                    message = lldb.SBDebugger().GetProgressFromEvent(event, 0, 0, 0, False);
+                    if message:
+                        self.progress_events.append((message, event))
+
+    @skipUnlessDarwin
+    def test_dwarf_symbol_loading_progress_report(self):
+        """Test that we are able to fetch dwarf symbol loading progress events"""
+        self.build()
+
+        test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
+        listener_thread = threading.Thread(target=self.fetch_events,
+                                           args=[test_broadcaster])
+        listener_thread.start()
+
+        lldbutil.run_to_source_breakpoint(self, 'break here', lldb.SBFileSpec('main.c'))
+
+        test_broadcaster.BroadcastEventByType(self.eBroadcastBitStopProgressThread)
+        listener_thread.join()
+
+        self.assertTrue(len(self.progress_events) > 0)

lldb/test/API/functionalities/progress_reporting/main.c

+int bar(int b) { return b * b; }
+
+int foo(int f) {
+  int b = bar(f); // break here
+  return b;
+}
+
+int main() {
+  int f = foo(42);
+  return f;
+}