[lldb/test] Fix data racing issue in TestStackCoreScriptedProcess

[mib]

Summary

This patch should fix an nondeterministic error in TestStackCoreScriptedProcess.

In order to test both the multithreading capability and shared library
loading in Scripted Processes, the test would create multiple threads
that would take the same variable as a reference.

The first thread would alter the value and the second thread would
monitor the value until it gets altered. This assumed a certain ordering
regarding the std::thread spawning, however the ordering was not
always guaranteed at runtime.

To fix that, the test now makes use of a std::condition_variable
shared between the each thread. On the former, it will notify the other
thread when the variable gets initialized or updated and on the latter,
it will wait until the variable it receives a new notification.

This should fix the data racing issue while preserving the testing
coverage.

rdar://98678134

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

[mib]

Run clang-format

[bulbazord]

I think that the idea you've laid out in the summary makes sense but the implementation seems off.
My understanding of std::condition_variable is that combined with std::mutex it is used to facilitate inter-thread synchronization. That would make sense if thread_1 and thread_2 both used a mutex and condition_variable to communicate with each other about the status of n but it looks like only thread_1 actually uses and manages the mutex/condition_variable. Why is call_and_wait invoking notify_one when it's also what invokes baz (which is what is invoking wait)? This seems somewhat sketchy because the thread that is performing the notifications is the same thread that is performing the wait.

To accomplish your goal, I think it would probably look more like this:

1. compute_pow should acquire the lock on the mutex, modify n, unlock, and then invoke cv.notify_one().
2. call_and_wait should only invoke baz. I don't think that the implementation of baz would require any modifications. Maybe thread_2 could run baz directly instead?

Perhaps I'm mistaken about some details but I think this could probably be written in a clearer way.

[mib]

Address @bulbazord comments

[bulbazord]

lldb/test/API/functionalities/scripted_process/main.cpp

18

Why do you need this initial notification?

21–23

Why does this need to be in a while loop? The condition_variable's wait function should guarantee that baz only ever returns 42, no? Unless I'm missing something.

[mib]

Update test program and test

[bulbazord]

LGTM, small nit

lldb/test/API/functionalities/scripted_process/main.cpp

22

nit: no need to relock, you're done modifying n. The lock will get freed right after anyway.

[JDevlieghere]

lldb/test/API/functionalities/scripted_process/main.cpp

15–16

Unless I misunderstand the code, I don't think you need this while loop anymore. baz should block until the condition variable is changed.

[mib]

Address comments from @JDevlieghere & @bulbazord

lldb/test/API/functionalities/scripted_process/Makefile

 CXX_SOURCES := main.cpp
 ENABLE_THREADS := YES
 LD_EXTRAS := -L. -lbaz -I.
 
 override ARCH := $(shell uname -m)
 
 all: libbaz.dylib a.out
 
-libbaz.dylib: baz.c
+libbaz.dylib: baz.cpp
   $(MAKE) -f $(MAKEFILE_RULES) ARCH=$(ARCH) \
-    DYLIB_ONLY=YES DYLIB_NAME=baz DYLIB_C_SOURCES=baz.c
+    DYLIB_ONLY=YES DYLIB_NAME=baz DYLIB_CXX_SOURCES=baz.cpp
 
 include Makefile.rules

lldb/test/API/functionalities/scripted_process/TestStackCoreScriptedProcess.py

 
 class StackCoreScriptedProcesTestCase(TestBase):
 
     NO_DEBUG_INFO_TESTCASE = True
 
     def create_stack_skinny_corefile(self, file):
         self.build()
         target, process, thread, _ = lldbutil.run_to_source_breakpoint(self, "// break here",
-                                                                       lldb.SBFileSpec("baz.c"))
+                                                                       lldb.SBFileSpec("baz.cpp"))
         self.assertTrue(process.IsValid(), "Process is invalid.")
         # FIXME: Use SBAPI to save the process corefile.
         self.runCmd("process save-core -s stack  " + file)
         self.assertTrue(os.path.exists(file), "No stack-only corefile found.")
         self.assertTrue(self.dbg.DeleteTarget(target), "Couldn't delete target")
 
     def get_module_with_name(self, target, name):
         for module in target.modules:
def test_launch_scripted_process_stack_frames(self):
 
         self.assertEqual(thread.GetNumFrames(), 5)
         frame = thread.GetSelectedFrame()
         self.assertTrue(frame, "Invalid frame.")
         func = frame.GetFunction()
         self.assertTrue(func, "Invalid function.")
 
         self.assertIn("baz", frame.GetFunctionName())
-        self.assertEqual(frame.vars.GetSize(), 2)
-        self.assertEqual(int(frame.vars.GetFirstValueByName('j').GetValue()), 42 * 42)
+        self.assertGreater(frame.vars.GetSize(), 0)
         self.assertEqual(int(frame.vars.GetFirstValueByName('k').GetValue()), 42)
+        self.assertEqual(int(frame.vars.GetFirstValueByName('j').GetValue()), 42 * 42)
 
         corefile_dylib = self.get_module_with_name(corefile_target, 'libbaz.dylib')
         self.assertTrue(corefile_dylib, "Dynamic library libbaz.dylib not found.")
         scripted_dylib = self.get_module_with_name(target, 'libbaz.dylib')
         self.assertTrue(scripted_dylib, "Dynamic library libbaz.dylib not found.")
         self.assertEqual(scripted_dylib.GetObjectFileHeaderAddress().GetLoadAddress(target),
                          corefile_dylib.GetObjectFileHeaderAddress().GetLoadAddress(target))

lldb/test/API/functionalities/scripted_process/baz.h

 #pragma once
 
-int baz(int j);
+#include <condition_variable>
+#include <mutex>
+
+int baz(int j, std::mutex& mutex, std::condition_variable& cv);

lldb/test/API/functionalities/scripted_process/baz.c

-#include "baz.h"
-
-#include <math.h>
-
-int baz(int j) {
-  int k = sqrt(j);
-  return k; // break here
-}

lldb/test/API/functionalities/scripted_process/baz.cpp

+#include "baz.h"
+
+#include <math.h>
+
+int baz(int j, std::mutex& mutex, std::condition_variable& cv) {
+  std::unique_lock<std::mutex> lock(mutex);
+  cv.wait(lock, [&j]{return j == 42 * 42;});
+  int k = sqrt(j);
+  return k; // break here
+}

lldb/test/API/functionalities/scripted_process/main.cpp

 #include <iostream>
-#include <mutex>
 #include <thread>
 
-extern "C" {
-int baz(int);
-}
+#include "baz.h"
+
+std::condition_variable cv;
+std::mutex mutex;
 
 int bar(int i) {
   int j = i * i;
   return j;
 }
 
 int foo(int i) { return bar(i); }
 
-void call_and_wait(int &n) {
-  std::cout << "waiting for computation!" << std::endl;
-  while (baz(n) != 42)
-    ;
-  std::cout << "finished computation!" << std::endl;
+void compute_pow(int &n) {
+  n = foo(n);
 }
 
-void compute_pow(int &n) { n = foo(n); }
+void call_and_wait(int &n, std::mutex& mutex, std::condition_variable& cv) {
+  cv.notify_one();
+  while (baz(n, mutex, cv) != 42) {
+    cv.notify_one();
+  }
+}
 
 int main() {
   int n = 42;
-  std::mutex mutex;
-  std::unique_lock<std::mutex> lock(mutex);
 
-  std::thread thread_1(call_and_wait, std::ref(n));
+  std::thread thread_1(call_and_wait, std::ref(n), std::ref(mutex), std::ref(cv));
   std::thread thread_2(compute_pow, std::ref(n));
-  lock.unlock();
 
   thread_1.join();
   thread_2.join();
 
   return 0;
 }