[lldb/test] Fix TestProgressReporting.py race issue with the event listener

[mib]

Summary

This patch is a follow-up of D120100 to address some feedbacks from
@labath.

This should mainly fix the race issue with the even listener by moving
the listener setup to the main thread.

This also changes the SBDebugger::GetProgressFromEvent SWIG binding
arguments to be output only, so the user don't have to provide them.

Finally, this updates the test to check it the out arguments are returned
in a tuple and re-enables the test on all platforms.

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

[mib]

Make the interface binding arguments OUTPUT only.
Re-enable test on all platforms.

[MForster]

This fails under MSAN:

==3096==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f0e7899ce18  (/build/work/73ffa3b6f1cbc2218b7b71e21a3dd1e2e53b/google3/runfiles/google3/third_party/py/lldb/dotest_wrapper.par/@0x3a0b9000+0x2e3e18) (BuildId: 4c3940d9ff00226e685dcfb93f4b4753)
    #1 0x7f0e7898fcc8  (/build/work/73ffa3b6f1cbc2218b7b71e21a3dd1e2e53b/google3/runfiles/google3/third_party/py/lldb/dotest_wrapper.par/@0x3a0b9000+0x2d6cc8) (BuildId: 4c3940d9ff00226e685dcfb93f4b4753)
    #2 0x7f0e78973001  (/build/work/73ffa3b6f1cbc2218b7b71e21a3dd1e2e53b/google3/runfiles/google3/third_party/py/lldb/dotest_wrapper.par/@0x3a0b9000+0x2ba001) (BuildId: 4c3940d9ff00226e685dcfb93f4b4753)
    #3 0x7f0e78cd1f10  (/build/work/73ffa3b6f1cbc2218b7b71e21a3dd1e2e53b/google3/runfiles/google3/third_party/py/lldb/dotest_wrapper.par/@0x3a0b9000+0x618f10) (BuildId: 4c3940d9ff00226e685dcfb93f4b4753)
    #4 0x558eaf3ecc70 in cfunction_call_varargs third_party/python_runtime/v3_7/Objects/call.c:770:18
    #5 0x558eaf3ecc70 in PyCFunction_Call third_party/python_runtime/v3_7/Objects/call.c:786:16
    #6 0x558eaf685ff8 in do_call_core third_party/python_runtime/v3_7/Python/ceval.c:4641:9
    #7 0x558eaf685ff8 in _PyEval_EvalFrameDefault third_party/python_runtime/v3_7/Python/ceval.c:3191:22
    #8 0x558eaf6a0691 in PyEval_EvalFrameEx third_party/python_runtime/v3_7/Python/ceval.c:547:12
    #9 0x558eaf6a0691 in _PyEval_EvalCodeWithName third_party/python_runtime/v3_7/Python/ceval.c:3930:14
    #10 0x558eaf3ebaa3 in _PyFunction_FastCallKeywords third_party/python_runtime/v3_7/Objects/call.c:433:12
    #11 0x558eaf69db2d in call_function third_party/python_runtime/v3_7/Python/ceval.c:4616:17
    #12 0x558eaf685844 in _PyEval_EvalFrameDefault third_party/python_runtime/v3_7/Python/ceval.c:3093:23
    #13 0x558eaf67d594 in PyEval_EvalFrameEx third_party/python_runtime/v3_7/Python/ceval.c:547:12
    #14 0x558eaf3ed230 in function_code_fastcall third_party/python_runtime/v3_7/Objects/call.c:283:14
    #15 0x558eaf3ea160 in _PyFunction_FastCallDict third_party/python_runtime/v3_7/Objects/call.c
    #16 0x558eaf3e99a4 in _PyObject_FastCallDict third_party/python_runtime/v3_7/Objects/call.c:98:16
    #17 0x558eaf3ef739 in _PyObject_Call_Prepend third_party/python_runtime/v3_7/Objects/call.c:906:14
    #18 0x558eaf3f462b in method_call third_party/python_runtime/v3_7/Objects/classobject.c:309:12
    #19 0x558eaf3ec51d in PyObject_Call third_party/python_runtime/v3_7/Objects/call.c:245:18
    #20 0x558eaf685d63 in do_call_core third_party/python_runtime/v3_7/Python/ceval.c:4645:16
    #21 0x558eaf685d63 in _PyEval_EvalFrameDefault third_party/python_runtime/v3_7/Python/ceval.c:3191:22
    #22 0x558eaf67d594 in PyEval_EvalFrameEx third_party/python_runtime/v3_7/Python/ceval.c:547:12
    #23 0x558eaf3ed230 in function_code_fastcall third_party/python_runtime/v3_7/Objects/call.c:283:14
    #24 0x558eaf3ebb23 in _PyFunction_FastCallKeywords third_party/python_runtime/v3_7/Objects/call.c
    #25 0x558eaf69db2d in call_function third_party/python_runtime/v3_7/Python/ceval.c:4616:17
    #26 0x558eaf685808 in _PyEval_EvalFrameDefault third_party/python_runtime/v3_7/Python/ceval.c:3110:23
    #27 0x558eaf67d594 in PyEval_EvalFrameEx third_party/python_runtime/v3_7/Python/ceval.c:547:12
    #28 0x558eaf3ed230 in function_code_fastcall third_party/python_runtime/v3_7/Objects/call.c:283:14
    #29 0x558eaf3ebb23 in _PyFunction_FastCallKeywords third_party/python_runtime/v3_7/Objects/call.c
    #30 0x558eaf69db2d in call_function third_party/python_runtime/v3_7/Python/ceval.c:4616:17
    #31 0x558eaf685808 in _PyEval_EvalFrameDefault third_party/python_runtime/v3_7/Python/ceval.c:3110:23
    #32 0x558eaf67d594 in PyEval_EvalFrameEx third_party/python_runtime/v3_7/Python/ceval.c:547:12
    #33 0x558eaf3ed230 in function_code_fastcall third_party/python_runtime/v3_7/Objects/call.c:283:14
    #34 0x558eaf3ea160 in _PyFunction_FastCallDict third_party/python_runtime/v3_7/Objects/call.c
    #35 0x558eaf3e99a4 in _PyObject_FastCallDict third_party/python_runtime/v3_7/Objects/call.c:98:16
    #36 0x558eaf3ef739 in _PyObject_Call_Prepend third_party/python_runtime/v3_7/Objects/call.c:906:14
    #37 0x558eaf3f462b in method_call third_party/python_runtime/v3_7/Objects/classobject.c:309:12
    #38 0x558eaf3ec51d in PyObject_Call third_party/python_runtime/v3_7/Objects/call.c:245:18
    #39 0x558eaecfdc9a in t_bootstrap third_party/python_runtime/v3_7/Modules/_threadmodule.c:994:11
    #40 0x558eaf77ac94 in pythread_wrapper third_party/python_runtime/v3_7/Python/thread_pthread.h:174:5
    #41 0x7f0e7bf0eb54 in start_thread (/usr/grte/v5/lib64/libpthread.so.0+0xbb54) (BuildId: 64752de50ebd1a108f4b3f8d0d7e1a13)
    #42 0x7f0e7be43f7e in clone (/usr/grte/v5/lib64/libc.so.6+0x13cf7e) (BuildId: 7cfed7708e5ab7fcb286b373de21ee76)

I'll revert.

[labath]

The backtrace is pretty much useless, but what is happening is that the LLDB_INSTRUMENT_VA macro inside SBDebugger::GetProgressFromEvent is trying to log the uninitialized output variable, which angers the gods of sanitation.

I'm not entirely sure what's the best fix here. @JDevlieghere, what do you think? Can we just remove the output arguments from the LLDB_INSTRUMENT_VA invocation (given how logging their entry values is pretty useless)?

[MForster]

The backtrace is pretty much useless

Yes, sorry, that's all I had. Just added some evidence.

[JDevlieghere]

I'm not entirely sure what's the best fix here. @JDevlieghere, what do you think? Can we just remove the output arguments from the LLDB_INSTRUMENT_VA invocation (given how logging their entry values is pretty useless)?

Yup, the way I dealt with that for the reproducers was initialize them before the macro, but no point in doing that purely for logging. Let's just remove the macro.

I'll need to think of a way to avoid lldb-instr putting it back. It will ignore functions that start with a macro, so maybe we can add a NOOP macro "LLDB_NO_INSTRUMENT" or something?

[mib]

In D120284#3345994, @JDevlieghere wrote:

I'm not entirely sure what's the best fix here. @JDevlieghere, what do you think? Can we just remove the output arguments from the LLDB_INSTRUMENT_VA invocation (given how logging their entry values is pretty useless)?

Yup, the way I dealt with that for the reproducers was initialize them before the macro, but no point in doing that purely for logging. Let's just remove the macro.

I'll need to think of a way to avoid lldb-instr putting it back. It will ignore functions that start with a macro, so maybe we can add a NOOP macro "LLDB_NO_INSTRUMENT" or something?

Will lldb-instr try to override the macro if not all the function arguments are passed to the LLDB_INSTRUMENT_VA invocation ? If this is not the case, it should be fine for SBDebugger::GetProgressFromEvent since it still takes an SBEvent argument, that will remain in the LLDB_INSTRUMENT_VA invocation.

[labath]

In D120284#3345994, @JDevlieghere wrote:

I'm not entirely sure what's the best fix here. @JDevlieghere, what do you think? Can we just remove the output arguments from the LLDB_INSTRUMENT_VA invocation (given how logging their entry values is pretty useless)?

Yup, the way I dealt with that for the reproducers was initialize them before the macro, but no point in doing that purely for logging. Let's just remove the macro.

I'll need to think of a way to avoid lldb-instr putting it back. It will ignore functions that start with a macro, so maybe we can add a NOOP macro "LLDB_NO_INSTRUMENT" or something?

I thought we could just replace LLDB_INSTRUMENT_VA(event, progress_id, completed, total, is_debugger_specific); with LLDB_INSTRUMENT_VA(event);

Is that not sufficient to dissuade the tool?

[JDevlieghere]

Yup, that works too

lldb/bindings/interface/SBDebugger.i

 {
 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);
+                                        uint64_t &OUTPUT,
+                                        uint64_t &OUTPUT,
+                                        uint64_t &OUTPUT,
+                                        bool &OUTPUT);
 
     SBBroadcaster GetBroadcaster();
 
     static void
     Initialize();
 
     static SBError
     InitializeWithErrorHandling();

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

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)
-
+    def fetch_events(self):
         event = lldb.SBEvent()
 
         done = False
         while not done:
-            if listener.WaitForEvent(1, event):
+            if self.listener.WaitForEvent(1, event):
                 event_mask = event.GetType();
-                if event.BroadcasterMatchesRef(test_broadcaster):
+                if event.BroadcasterMatchesRef(self.test_broadcaster):
                     if event_mask & self.eBroadcastBitStopProgressThread:
                         done = True;
-                elif event.BroadcasterMatchesRef(progress_broadcaster):
-                    message = lldb.SBDebugger().GetProgressFromEvent(event, 0, 0, 0, False);
+                elif event.BroadcasterMatchesRef(self.progress_broadcaster):
+                    ret_args = lldb.SBDebugger().GetProgressFromEvent(event);
+                    self.assertGreater(len(ret_args), 1)
+
+                    message = ret_args[0]
                     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])
+        self.listener = lldb.SBListener("lldb.progress.listener")
+        self.test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
+        self.listener.StartListeningForEvents(self.test_broadcaster,
+                                              self.eBroadcastBitStopProgressThread)
+
+        self.progress_broadcaster = self.dbg.GetBroadcaster()
+        self.progress_broadcaster.AddListener(self.listener, lldb.SBDebugger.eBroadcastBitProgress)
+
+        listener_thread = threading.Thread(target=self.fetch_events)
         listener_thread.start()
 
         lldbutil.run_to_source_breakpoint(self, 'break here', lldb.SBFileSpec('main.c'))
 
-        test_broadcaster.BroadcastEventByType(self.eBroadcastBitStopProgressThread)
+        self.test_broadcaster.BroadcastEventByType(self.eBroadcastBitStopProgressThread)
         listener_thread.join()
 
-        self.assertTrue(len(self.progress_events) > 0)
+        self.assertGreater(len(self.progress_events), 0)