[lldb] Deal gracefully with concurrency in the API instrumentation.

[JDevlieghere]

Summary

Prevent lldb from crashing when multiple threads are concurrently accessing the SB API with reproducer capture enabled. The API instrumentation records both the input arguments and the return value, but it cannot block for the duration of the API call. Therefore we guarantee that the function id and its arguments are emitted as a single unit and that the return value is emitted as a single unit. Every API call is given a monotonically increasing index, which correlates the function with its return value.

Using the index, we can detect situations where the return value does not succeed the function call, in which case we print an error saying that concurrency is not (currently) supported. In the future we might attempt to be smarter and read ahead until we've found the return value matching the current call.

[friss]

The change itself looks fine, but I'm wondering about the test. There is nothing that really guarantees that the SB calls and return values are going to be intertwined, is there? Does it rely solely on the fact that it's very very unlikely to succeed?

[JDevlieghere]

Change test to unit test.

[friss]

This LGTM.
There is one thing that you might want to address, but I'll leave it up to you (and even if you do it can be a different commit): with the introduction of idx variables, it becomes easy to get confused between id and idx(Or CheckID and CheckIndex) in the codebase. Not sure how to disambiguate, or if it's really worth it.

[JDevlieghere]

In D92820#2446091, @friss wrote:

This LGTM.
There is one thing that you might want to address, but I'll leave it up to you (and even if you do it can be a different commit): with the introduction of idx variables, it becomes easy to get confused between id and idx(Or CheckID and CheckIndex) in the codebase. Not sure how to disambiguate, or if it's really worth it.

Agreed, it's even more confusing because I also use idx for the object index. Let me rename this to sequence.

lldb/include/lldb/Utility/ReproducerInstrumentation.h

 #ifdef LLDB_REPRO_INSTR_TRACE
     llvm::errs() << "Deserializing with " << LLVM_PRETTY_FUNCTION << " -> "
                  << stringify_args(t) << "\n";
 #endif
     return t;
   }
 
   template <typename T> const T &HandleReplayResult(const T &t) {
+    CheckIndex(Deserialize<unsigned>());
     unsigned result = Deserialize<unsigned>();
     if (is_trivially_serializable<T>::value)
       return t;
     // We need to make a copy as the original object might go out of scope.
     return *m_index_to_object.AddObjectForIndex(result, new T(t));
   }
 
   /// Store the returned value in the index-to-object mapping.
   template <typename T> T &HandleReplayResult(T &t) {
+    CheckIndex(Deserialize<unsigned>());
     unsigned result = Deserialize<unsigned>();
     if (is_trivially_serializable<T>::value)
       return t;
     // We need to make a copy as the original object might go out of scope.
     return *m_index_to_object.AddObjectForIndex(result, new T(t));
   }
 
   /// Store the returned value in the index-to-object mapping.
   template <typename T> T *HandleReplayResult(T *t) {
+    CheckIndex(Deserialize<unsigned>());
     unsigned result = Deserialize<unsigned>();
     if (is_trivially_serializable<T>::value)
       return t;
     return m_index_to_object.AddObjectForIndex(result, t);
   }
 
   /// All returned types are recorded, even when the function returns a void.
   /// The latter requires special handling.
   void HandleReplayResultVoid() {
+    CheckIndex(Deserialize<unsigned>());
     unsigned result = Deserialize<unsigned>();
     assert(result == 0);
     (void)result;
   }
 
   std::vector<void *> GetAllObjects() const {
     return m_index_to_object.GetAllObjects();
   }
 
+  void SetExpectedIndex(unsigned idx) { m_expected_idx = idx; }
+
 private:
   template <typename T> T Read(ValueTag) {
     assert(HasData(sizeof(T)));
     T t;
     std::memcpy(reinterpret_cast<char *>(&t), m_buffer.data(), sizeof(T));
     m_buffer = m_buffer.drop_front(sizeof(T));
     return t;
   }
private:
   /// We read its value, allocate a copy on the heap, and return a reference to
   /// the copy.
   template <typename T> T Read(FundamentalReferenceTag) {
     // If this is a reference to a fundamental type we just read its value.
     typedef typename std::remove_reference<T>::type UnderlyingT;
     return *(new UnderlyingT(Deserialize<UnderlyingT>()));
   }
 
+  /// Verify that the given index matches the expected index if set.
+  void CheckIndex(unsigned idx);
+
   /// Mapping of indices to objects.
   IndexToObject m_index_to_object;
 
   /// Buffer containing the serialized data.
   llvm::StringRef m_buffer;
+
+  /// Expected index of the result.
+  llvm::Optional<unsigned> m_expected_idx;
 };
 
 /// Partial specialization for C-style strings. We read the string value
 /// instead of treating it as pointer.
 template <> const char *Deserializer::Deserialize<const char *>();
 template <> const char **Deserializer::Deserialize<const char **>();
 template <> const uint8_t *Deserializer::Deserialize<const uint8_t *>();
 template <> const void *Deserializer::Deserialize<const void *>();
public:
   Recorder();
   Recorder(llvm::StringRef pretty_func, std::string &&pretty_args = {});
   ~Recorder();
 
   /// Records a single function call.
   template <typename Result, typename... FArgs, typename... RArgs>
   void Record(Serializer &serializer, Registry &registry, Result (*f)(FArgs...),
               const RArgs &... args) {
+    std::lock_guard<std::mutex> lock(g_mutex);
     m_serializer = &serializer;
     if (!ShouldCapture())
       return;
 
     unsigned id = registry.GetID(uintptr_t(f));
 
 #ifdef LLDB_REPRO_INSTR_TRACE
     Log(id);
 #endif
 
+    serializer.SerializeAll(m_index);
     serializer.SerializeAll(id);
     serializer.SerializeAll(args...);
 
     if (std::is_class<typename std::remove_pointer<
             typename std::remove_reference<Result>::type>::type>::value) {
       m_result_recorded = false;
     } else {
+      serializer.SerializeAll(m_index);
       serializer.SerializeAll(0);
       m_result_recorded = true;
     }
   }
 
   /// Records a single function call.
   template <typename... Args>
   void Record(Serializer &serializer, Registry &registry, void (*f)(Args...),
               const Args &... args) {
     m_serializer = &serializer;
     if (!ShouldCapture())
       return;
 
+    std::lock_guard<std::mutex> lock(g_mutex);
     unsigned id = registry.GetID(uintptr_t(f));
 
 #ifdef LLDB_REPRO_INSTR_TRACE
     Log(id);
 #endif
 
+    serializer.SerializeAll(m_index);
     serializer.SerializeAll(id);
     serializer.SerializeAll(args...);
 
     // Record result.
+    serializer.SerializeAll(m_index);
     serializer.SerializeAll(0);
     m_result_recorded = true;
   }
 
   /// Specializations for the no-argument methods. These are passed an empty
   /// dummy argument so the same variadic macro can be used. These methods
   /// strip the arguments before forwarding them.
   template <typename Result>
Result RecordResult(Result &&r, bool update_boundary) {
     // update the boundary so we capture the copy constructor. However, when
     // called to record the this pointer of the (copy) constructor, the
     // boundary should not be toggled, because it is called from the
     // LLDB_RECORD_CONSTRUCTOR macro, which might be followed by other API
     // calls.
     if (update_boundary)
       UpdateBoundary();
     if (m_serializer && ShouldCapture()) {
+      std::lock_guard<std::mutex> lock(g_mutex);
       assert(!m_result_recorded);
+      m_serializer->SerializeAll(m_index);
       m_serializer->SerializeAll(r);
       m_result_recorded = true;
     }
     return std::forward<Result>(r);
   }
 
   template <typename Result, typename T>
   Result Replay(Deserializer &deserializer, Registry &registry, uintptr_t addr,
                 bool update_boundary) {
+    deserializer.SetExpectedIndex(deserializer.Deserialize<unsigned>());
     unsigned actual_id = registry.GetID(addr);
     unsigned id = deserializer.Deserialize<unsigned>();
     registry.CheckID(id, actual_id);
     return ReplayResult<Result>(
         static_cast<DefaultReplayer<T> *>(registry.GetReplayer(id))
             ->Replay(deserializer),
         update_boundary);
   }
 
   void Replay(Deserializer &deserializer, Registry &registry, uintptr_t addr) {
+    deserializer.SetExpectedIndex(deserializer.Deserialize<unsigned>());
     unsigned actual_id = registry.GetID(addr);
     unsigned id = deserializer.Deserialize<unsigned>();
     registry.CheckID(id, actual_id);
     registry.GetReplayer(id)->operator()(deserializer);
   }
 
   template <typename Result>
   Result ReplayResult(Result &&r, bool update_boundary) {
     if (update_boundary)
       UpdateBoundary();
     return std::forward<Result>(r);
   }
 
   bool ShouldCapture() { return m_local_boundary; }
 
   /// Mark the current thread as a private thread and pretend that everything
   /// on this thread is behind happening behind the API boundary.
   static void PrivateThread() { g_global_boundary = true; }
 
 private:
+  static uint64_t GetNextIndex() { return g_index++; }
+
   template <typename T> friend struct replay;
   void UpdateBoundary() {
     if (m_local_boundary)
       g_global_boundary = false;
   }
 
 #ifdef LLDB_REPRO_INSTR_TRACE
   void Log(unsigned id) {
#endif
   std::string m_pretty_args;
 
   /// Whether this function call was the one crossing the API boundary.
   bool m_local_boundary;
 
   /// Whether the return value was recorded explicitly.
   bool m_result_recorded;
 
+  /// The current index.
+  unsigned m_index;
+
   /// Whether we're currently across the API boundary.
   static thread_local bool g_global_boundary;
+
+  /// Global mutex to protect concurrent access.
+  static std::mutex g_mutex;
+
+  /// Global index.
+  static std::atomic<unsigned> g_index;
 };
 
 /// To be used as the "Runtime ID" of a constructor. It also invokes the
 /// constructor when called.
 template <typename Signature> struct construct;
 template <typename Class, typename... Args> struct construct<Class(Args...)> {
   static Class *handle(lldb_private::repro::InstrumentationData data,
                        lldb_private::repro::Recorder &recorder, Class *c,
struct invoke_char_ptr<Result (Class::*)(Args...) const> {
   template <Result (Class::*m)(Args...) const> struct method {
     static Result record(Class *c, char *s, size_t l) {
       char *buffer = reinterpret_cast<char *>(calloc(l, sizeof(char)));
       return (c->*m)(buffer, l);
     }
 
     static Result replay(Recorder &recorder, Deserializer &deserializer,
                          Registry &registry, char *str) {
+      deserializer.SetExpectedIndex(deserializer.Deserialize<unsigned>());
       deserializer.Deserialize<unsigned>();
       Class *c = deserializer.Deserialize<Class *>();
       deserializer.Deserialize<const char *>();
       size_t l = deserializer.Deserialize<size_t>();
       return recorder.ReplayResult(
           std::move(deserializer.HandleReplayResult((c->*m)(str, l))), true);
     }
   };
 };
 
 template <typename Signature> struct invoke_char_ptr;
 template <typename Result, typename Class, typename... Args>
 struct invoke_char_ptr<Result (Class::*)(Args...)> {
   template <Result (Class::*m)(Args...)> struct method {
     static Result record(Class *c, char *s, size_t l) {
       char *buffer = reinterpret_cast<char *>(calloc(l, sizeof(char)));
       return (c->*m)(buffer, l);
     }
 
     static Result replay(Recorder &recorder, Deserializer &deserializer,
                          Registry &registry, char *str) {
+      deserializer.SetExpectedIndex(deserializer.Deserialize<unsigned>());
       deserializer.Deserialize<unsigned>();
       Class *c = deserializer.Deserialize<Class *>();
       deserializer.Deserialize<const char *>();
       size_t l = deserializer.Deserialize<size_t>();
       return recorder.ReplayResult(
           std::move(deserializer.HandleReplayResult((c->*m)(str, l))), true);
     }
   };
 };
 
 template <typename Result, typename... Args>
 struct invoke_char_ptr<Result (*)(Args...)> {
   template <Result (*m)(Args...)> struct method {
     static Result record(char *s, size_t l) {
       char *buffer = reinterpret_cast<char *>(calloc(l, sizeof(char)));
       return (*m)(buffer, l);
     }
 
     static Result replay(Recorder &recorder, Deserializer &deserializer,
                          Registry &registry, char *str) {
+      deserializer.SetExpectedIndex(deserializer.Deserialize<unsigned>());
       deserializer.Deserialize<unsigned>();
       deserializer.Deserialize<const char *>();
       size_t l = deserializer.Deserialize<size_t>();
       return recorder.ReplayResult(
           std::move(deserializer.HandleReplayResult((*m)(str, l))), true);
     }
   };
 };
 /// }
 
 } // namespace repro
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_REPRODUCERINSTRUMENTATION_H

lldb/source/Utility/ReproducerInstrumentation.cpp

 //===-- ReproducerInstrumentation.cpp -------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Utility/ReproducerInstrumentation.h"
 #include "lldb/Utility/Reproducer.h"
+#include <limits>
 #include <stdio.h>
 #include <stdlib.h>
 #include <thread>
 
 using namespace lldb_private;
 using namespace lldb_private::repro;
 
 void *IndexToObject::GetObjectForIndexImpl(unsigned idx) {
if (size == 0)
     return nullptr;
   const char **r =
       reinterpret_cast<const char **>(calloc(size + 1, sizeof(char *)));
   for (size_t i = 0; i < size; ++i)
     r[i] = Deserialize<const char *>();
   return r;
 }
 
+void Deserializer::CheckIndex(unsigned idx) {
+  if (m_expected_idx && *m_expected_idx != idx)
+    llvm::report_fatal_error(
+        "The result does not match the preceding "
+        "function. This is probably the result of concurrent "
+        "use of the SB API during capture, which is currently not "
+        "supported.");
+  m_expected_idx.reset();
+}
+
 bool Registry::Replay(const FileSpec &file) {
   auto error_or_file = llvm::MemoryBuffer::getFile(file.GetPath());
   if (auto err = error_or_file.getError())
     return false;
 
   return Replay((*error_or_file)->getBuffer());
 }
 
 bool Registry::Replay(llvm::StringRef buffer) {
   Deserializer deserializer(buffer);
   return Replay(deserializer);
 }
 
 bool Registry::Replay(Deserializer &deserializer) {
 #ifndef LLDB_REPRO_INSTR_TRACE
   Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_API);
 #endif
 
   // Disable buffering stdout so that we approximate the way things get flushed
   // during an interactive session.
   setvbuf(stdout, nullptr, _IONBF, 0);
 
   while (deserializer.HasData(1)) {
+    unsigned idx = deserializer.Deserialize<unsigned>();
     unsigned id = deserializer.Deserialize<unsigned>();
 
 #ifndef LLDB_REPRO_INSTR_TRACE
     LLDB_LOG(log, "Replaying {0}: {1}", id, GetSignature(id));
 #else
     llvm::errs() << "Replaying " << id << ": " << GetSignature(id) << "\n";
 #endif
 
+    deserializer.SetExpectedIndex(idx);
     GetReplayer(id)->operator()(deserializer);
   }
 
   // Add a small artificial delay to ensure that all asynchronous events have
   // completed before we exit.
   std::this_thread::sleep_for(std::chrono::milliseconds(100));
 
   return true;
unsigned ObjectToIndex::GetIndexForObjectImpl(const void *object) {
   auto it = m_mapping.find(object);
   if (it == m_mapping.end())
     m_mapping[object] = index;
   return m_mapping[object];
 }
 
 Recorder::Recorder()
     : m_serializer(nullptr), m_pretty_func(), m_pretty_args(),
-      m_local_boundary(false), m_result_recorded(true) {
+      m_local_boundary(false), m_result_recorded(true),
+      m_index(std::numeric_limits<unsigned>::max()) {
   if (!g_global_boundary) {
     g_global_boundary = true;
     m_local_boundary = true;
+    m_index = GetNextIndex();
   }
 }
 
 Recorder::Recorder(llvm::StringRef pretty_func, std::string &&pretty_args)
     : m_serializer(nullptr), m_pretty_func(pretty_func),
       m_pretty_args(pretty_args), m_local_boundary(false),
-      m_result_recorded(true) {
+      m_result_recorded(true), m_index(std::numeric_limits<unsigned>::max()) {
   if (!g_global_boundary) {
     g_global_boundary = true;
     m_local_boundary = true;
-
+    m_index = GetNextIndex();
     LLDB_LOG(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API), "{0} ({1})",
              m_pretty_func, m_pretty_args);
   }
 }
 
 Recorder::~Recorder() {
   assert(m_result_recorded && "Did you forget LLDB_RECORD_RESULT?");
   UpdateBoundary();

 }
 
 llvm::Optional<InstrumentationData> &InstrumentationData::InstanceImpl() {
   static llvm::Optional<InstrumentationData> g_instrumentation_data;
   return g_instrumentation_data;
 }
 
 thread_local bool lldb_private::repro::Recorder::g_global_boundary = false;
+std::atomic<unsigned> lldb_private::repro::Recorder::g_index;
+std::mutex lldb_private::repro::Recorder::g_mutex;

lldb/unittests/Utility/ReproducerInstrumentationTest.cpp

 
 TEST(SerializationRountripTest, SerializeDeserializeObjectPointer) {
   Foo foo;
   Bar bar;
 
   std::string str;
   llvm::raw_string_ostream os(str);
 
+  unsigned index = 0;
+
   Serializer serializer(os);
-  serializer.SerializeAll(static_cast<unsigned>(1), static_cast<unsigned>(2));
+  serializer.SerializeAll(index, static_cast<unsigned>(1));
+  serializer.SerializeAll(index, static_cast<unsigned>(2));
   serializer.SerializeAll(&foo, &bar);
 
   llvm::StringRef buffer(os.str());
   Deserializer deserializer(buffer);
 
   deserializer.HandleReplayResult(&foo);
   deserializer.HandleReplayResult(&bar);
 
   EXPECT_EQ(foo, *deserializer.Deserialize<Foo *>());
   EXPECT_EQ(bar, *deserializer.Deserialize<Bar *>());
 }
 
 TEST(SerializationRountripTest, SerializeDeserializeObjectReference) {
   Foo foo;
   Bar bar;
 
   std::string str;
   llvm::raw_string_ostream os(str);
 
+  unsigned index = 0;
+
   Serializer serializer(os);
-  serializer.SerializeAll(static_cast<unsigned>(1), static_cast<unsigned>(2));
+  serializer.SerializeAll(index, static_cast<unsigned>(1));
+  serializer.SerializeAll(index, static_cast<unsigned>(2));
   serializer.SerializeAll(foo, bar);
 
   llvm::StringRef buffer(os.str());
   Deserializer deserializer(buffer);
 
   deserializer.HandleReplayResult(&foo);
   deserializer.HandleReplayResult(&bar);
 
std::string buffer = os.str();
     EXPECT_NEAR(foo.GetE(), 400.4, 0.01);
     EXPECT_EQ(foo.GetF(), true);
 
     bar.SetInstrumentedFoo(foo);
     bar.SetInstrumentedFoo(&foo);
     bar.Validate();
   }
 }
+
+TEST(RecordReplayTest, ValidIndex) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  {
+    auto data = TestInstrumentationDataRAII::GetRecordingData(os);
+
+    unsigned index = 1;
+    int (*f)() = &lldb_private::repro::invoke<int (*)()>::method<
+        InstrumentedFoo::F>::record;
+    unsigned id = g_registry->GetID(uintptr_t(f));
+    g_serializer->SerializeAll(index, id);
+
+    unsigned result = 0;
+    g_serializer->SerializeAll(index, result);
+  }
+
+  TestingRegistry registry;
+  Deserializer deserializer(os.str());
+  registry.Replay(deserializer);
+}
+
+TEST(RecordReplayTest, InvalidIndex) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  {
+    auto data = TestInstrumentationDataRAII::GetRecordingData(os);
+
+    unsigned index = 1;
+    int (*f)() = &lldb_private::repro::invoke<int (*)()>::method<
+        InstrumentedFoo::F>::record;
+    unsigned id = g_registry->GetID(uintptr_t(f));
+    g_serializer->SerializeAll(index, id);
+
+    unsigned result = 0;
+    unsigned invalid_index = 2;
+    g_serializer->SerializeAll(invalid_index, result);
+  }
+
+  TestingRegistry registry;
+  Deserializer deserializer(os.str());
+  EXPECT_DEATH(registry.Replay(deserializer), "");
+}