[lldb/Reproducers] Fix passive replay for functions returning string as (char*, size_t).

[JDevlieghere]

Summary

Several SB API functions return strings a (char*, size_t) pair. During capture, we serialize an empty string for the char* because the memory can be uninitialized. During replay, we have custom replay redirects that ensure that we don't override the buffer from which we're reading, but rather a buffer on the heap with the given length. This is sufficient for the "regular" reproducer use case, where we only care about the side effects of the API calls, not the actual return values.

This is not sufficient for passive replay. For passive replay, we ignore all the incoming arguments, and re-execute the current function with the arguments serialized in the reproducer. This means that these function will update the deserialized copy of the arguments, rather than whatever was passed in by the SWIG wrapper. To solve this problem, I've extended the reproducer instrumentation with special case replayers and corresponding macros. They ignore the replayer in the registry and the incoming char pointer, and instead reinvoke the current method on the deserialized class, and populate the output argument. It's unfortunate that this needs to be special cased, but I don't see a better solution.

[JDevlieghere]

Rebase + add unit tests

[labath]

The thing which bugs me here (besides the duplication, which I think we now know how to handle) is that the definition of the behavior for the char*+size functions is spread out over REGISTER and RECORD macros -- one define the behavior of active replay, one for passive.

I sort of get that this is because the two modes are fairly different, but I think it still would be nice to centralize that, and I think that with some thought, that might be possible. As I understand it, tight now, the "active" redirection works by passing a pointer which completely replaces the behavior of the called method. In reality the replacing function will eventually call the replaced function anyway, with some fixups applied. What if, instead of replacing the function completely, we passed a pair of callbacks which would run before+after the original function. The first callback could apply any necessary fixups to the deserialized arguments, and the second one could (if in passive mode) translate the modifications done by the function into the "real" arguments. That way, we wouldn't need to do anything special in the RECORD macro...

Other ideas may be possible too...

[JDevlieghere]

Remove duplication similar to D77602.

Honestly, at this point I'm not convinced that the difference between active and passive replay warrants special handling. Other than the slightly different macros, which are unfortunate but OTOH also a good indicator that something special is happening, there's not that much more to support active/passive replay than there is for the generic case.

[labath]

Hmm... well, I like how this (active&passive) is unified at the template level. It still feels like there's too much code to implement this thing (coming from the duplication for const, non-const and static variants), but I'm not sure what can be done about that. I'll need to ponder this a bit more

lldb/include/lldb/Utility/ReproducerInstrumentation.h

1179

this forward decl is superfluous.

[labath]

In D77759#1988419, @labath wrote:

Hmm... well, I like how this (active&passive) is unified at the template level. It still feels like there's too much code to implement this thing (coming from the duplication for const, non-const and static variants), but I'm not sure what can be done about that. I'll need to ponder this a bit more

After sleeping on this a couple of times, I've come to believe that the problem here is that we've let the redirection functions inherit the c++ function pointer weirdness, which resulted in a need to write template goo for const, non-const and static methods (and maybe sometimes even constructors). The goo is (more or less) unavoidable if we want to have compiler-generated "identifiers" for all these functions, but there's a difference between writing that once, and once for each redirection we need to make. Particularly as it doesn't seem like there's a compelling reason for that. There no reason that the "redirected" function in the replayer machinery needs be globally unique in the same way as the "original" functions do. In fact I think it doesn't even have to be a template at all -- it could just take the function-to-wrap as a regular argument instead of a templated one. I believe char_ptr_redirect<goo>::method<&goo>::record could have been written as:

// this handles const and non-const methods
template<typename Result, typename This> // Maybe "Result" not even needed, if this is always the same type.
Result char_ptr_redirect(Result (*f)(This *, char *, size_t), This *this_, char *ptr, size_t len) {
  char *buffer = reinterpret_cast<char *>(calloc(len, sizeof(char)));
  return f(this_, buffer, len)
}
// and another overload without This for static functions

Similar thing could be applied to the "replay" functions introduced here. Reduction from three overloads to two may not sound like much, but given that it would also remove the need for a lot of template goo (and reduce code size), I still find it interesting. It might be possible to also merge the two remaining overloads if we try hard enough.

Nonetheless, as long as we have only one kind of redirects (char_ptr thingy), it's benefit is not very clear, so I'm not going to insist on that. However, if more of these crop up, I think we should definitely revisit this.

[JDevlieghere]

In D77759#1992252, @labath wrote:

In D77759#1988419, @labath wrote:

Hmm... well, I like how this (active&passive) is unified at the template level. It still feels like there's too much code to implement this thing (coming from the duplication for const, non-const and static variants), but I'm not sure what can be done about that. I'll need to ponder this a bit more

After sleeping on this a couple of times, I've come to believe that the problem here is that we've let the redirection functions inherit the c++ function pointer weirdness, which resulted in a need to write template goo for const, non-const and static methods (and maybe sometimes even constructors). The goo is (more or less) unavoidable if we want to have compiler-generated "identifiers" for all these functions, but there's a difference between writing that once, and once for each redirection we need to make. Particularly as it doesn't seem like there's a compelling reason for that. There no reason that the "redirected" function in the replayer machinery needs be globally unique in the same way as the "original" functions do. In fact I think it doesn't even have to be a template at all -- it could just take the function-to-wrap as a regular argument instead of a templated one.

Yeah, I've come to the same conclusion while working on this. Hindsight is 20/20 and changing all that might be a significant amount of work.

I believe char_ptr_redirect<goo>::method<&goo>::record could have been written as:

// this handles const and non-const methods
template<typename Result, typename This> // Maybe "Result" not even needed, if this is always the same type.
Result char_ptr_redirect(Result (*f)(This *, char *, size_t), This *this_, char *ptr, size_t len) {
  char *buffer = reinterpret_cast<char *>(calloc(len, sizeof(char)));
  return f(this_, buffer, len)
}
// and another overload without This for static functions

Similar thing could be applied to the "replay" functions introduced here. Reduction from three overloads to two may not sound like much, but given that it would also remove the need for a lot of template goo (and reduce code size), I still find it interesting. It might be possible to also merge the two remaining overloads if we try hard enough.

Agreed and while it would be a small thing it would also help compile times: this file is by far the slowest to compile in lldb. That and the templates have a significant cognitive overhead.

Nonetheless, as long as we have only one kind of redirects (char_ptr thingy), it's benefit is not very clear, so I'm not going to insist on that. However, if more of these crop up, I think we should definitely revisit this.

Thanks, I totally share your concerns and really appreciate the pragmatism.

[labath]

In D77759#1992670, @JDevlieghere wrote:

In D77759#1992252, @labath wrote:

In D77759#1988419, @labath wrote:

Hmm... well, I like how this (active&passive) is unified at the template level. It still feels like there's too much code to implement this thing (coming from the duplication for const, non-const and static variants), but I'm not sure what can be done about that. I'll need to ponder this a bit more

After sleeping on this a couple of times, I've come to believe that the problem here is that we've let the redirection functions inherit the c++ function pointer weirdness, which resulted in a need to write template goo for const, non-const and static methods (and maybe sometimes even constructors). The goo is (more or less) unavoidable if we want to have compiler-generated "identifiers" for all these functions, but there's a difference between writing that once, and once for each redirection we need to make. Particularly as it doesn't seem like there's a compelling reason for that. There no reason that the "redirected" function in the replayer machinery needs be globally unique in the same way as the "original" functions do. In fact I think it doesn't even have to be a template at all -- it could just take the function-to-wrap as a regular argument instead of a templated one.

Yeah, I've come to the same conclusion while working on this. Hindsight is 20/20 and changing all that might be a significant amount of work.

That last part makes me thing that we have not come to the same conclusion. :P I'm not talking about changing the entire mechanism of how we assign "ids" to functions and stuff. While it is true that it's not working out as well as I originally hoped, I still think that's ok, and having a bit of template goo to enable that is not bad. I'm just talking about changing how the "redirection" mechanism works -- the redirected functions don't need to be uniquely identifiable in the same way, as they can just be linked to the original function id, and so we should be able to come up with a less goo-ey mechanism for writing those. While that would certainly be "work" I don't think it would be a "significant" amount of work, as we only have a handful of redirected functions.

lldb/include/lldb/Utility/ReproducerInstrumentation.h

if (lldb_private::repro::Serializer *_serializer =                         \
       if (_recorder.ShouldCapture()) {                                         \
         return lldb_private::repro::replay<Result (*)()>::method_static<(      \
             &Class::Method)>::doit(_recorder, *_deserializer,                  \
                                    _data.GetRegistry(), LLVM_PRETTY_FUNCTION); \
       }                                                                        \
     }                                                                          \
   }
 
+#define LLDB_RECORD_CHAR_PTR_METHOD(Result, Class, Method, Signature, StrOut,  \
+                                    ...)                                       \
+  lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION,                \
+                                          stringify_args(*this, __VA_ARGS__)); \
+  if (lldb_private::repro::InstrumentationData _data =                         \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    if (lldb_private::repro::Serializer *_serializer =                         \
+            _data.GetSerializer()) {                                           \
+      _recorder.Record(                                                        \
+          *_serializer, _data.GetRegistry(),                                   \
+          &lldb_private::repro::invoke<Result(Class::*) Signature>::method<(   \
+              &Class::Method)>::doit,                                          \
+          this, __VA_ARGS__);                                                  \
+    } else if (lldb_private::repro::Deserializer *_deserializer =              \
+                   _data.GetDeserializer()) {                                  \
+      if (_recorder.ShouldCapture()) {                                         \
+        return lldb_private::repro::replay_char_ptr<                           \
+            Result(Class::*)                                                   \
+                Signature>::method<&Class::Method>::doit(_recorder,            \
+                                                         *_deserializer,       \
+                                                         _data.GetRegistry(),  \
+                                                         LLVM_PRETTY_FUNCTION, \
+                                                         StrOut);              \
+      }                                                                        \
+    }                                                                          \
+  }
+
+#define LLDB_RECORD_CHAR_PTR_METHOD_CONST(Result, Class, Method, Signature,    \
+                                          StrOut, ...)                         \
+  lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION,                \
+                                          stringify_args(*this, __VA_ARGS__)); \
+  if (lldb_private::repro::InstrumentationData _data =                         \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    if (lldb_private::repro::Serializer *_serializer =                         \
+            _data.GetSerializer()) {                                           \
+      _recorder.Record(                                                        \
+          *_serializer, _data.GetRegistry(),                                   \
+          &lldb_private::repro::invoke<Result(Class::*) Signature const>::     \
+              method_const<(&Class::Method)>::doit,                            \
+          this, __VA_ARGS__);                                                  \
+    } else if (lldb_private::repro::Deserializer *_deserializer =              \
+                   _data.GetDeserializer()) {                                  \
+      if (_recorder.ShouldCapture()) {                                         \
+        return lldb_private::repro::replay_char_ptr<Result(                    \
+            Class::*) Signature const>::method_const<&Class::Method>::         \
+            doit(_recorder, *_deserializer, _data.GetRegistry(),               \
+                 LLVM_PRETTY_FUNCTION, StrOut);                                \
+      }                                                                        \
+    }                                                                          \
+  }
+
+#define LLDB_RECORD_CHAR_PTR_STATIC_METHOD(Result, Class, Method, Signature,   \
+                                           StrOut, ...)                        \
+  lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION,                \
+                                          stringify_args(__VA_ARGS__));        \
+  if (lldb_private::repro::InstrumentationData _data =                         \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    if (lldb_private::repro::Serializer *_serializer =                         \
+            _data.GetSerializer()) {                                           \
+      _recorder.Record(*_serializer, _data.GetRegistry(),                      \
+                       static_cast<Result(*) Signature>(&Class::Method),       \
+                       __VA_ARGS__);                                           \
+    } else if (lldb_private::repro::Deserializer *_deserializer =              \
+                   _data.GetDeserializer()) {                                  \
+      if (_recorder.ShouldCapture()) {                                         \
+        return lldb_private::repro::replay_char_ptr<Result(*) Signature>::     \
+            method_static<(&Class::Method)>::doit(                             \
+                _recorder, *_deserializer, _data.GetRegistry(),                \
+                LLVM_PRETTY_FUNCTION, StrOut);                                 \
+      }                                                                        \
+    }                                                                          \
+  }
+
 #define LLDB_RECORD_RESULT(Result) _recorder.RecordResult(Result, true);
 
 /// The LLDB_RECORD_DUMMY macro is special because it doesn't actually record
 /// anything. It's used to track API boundaries when we cannot record for
 /// technical reasons.
 #define LLDB_RECORD_DUMMY(Result, Class, Method, Signature, ...)               \
   lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION,                \
                                           stringify_args(__VA_ARGS__));
static void doit(Recorder &recorder, Deserializer &deserializer,
                      Registry &registry, llvm::StringRef signature) {
       unsigned id = deserializer.Deserialize<unsigned>();
       registry.CheckSignature(signature, id);
       registry.GetReplayer(id)->operator()(deserializer);
     }
   };
 };
 
+/// Special handling for functions returning strings as (char*, size_t).
+/// {
+
+/// For inline replay, we ignore the arguments and use the ones from the
+/// serializer instead. This doesn't work for methods that use a char* and a
+/// size to return a string. For one these functions have a custom replayer to
+/// prevent override the input buffer. Furthermore, the template-generated
+/// deserialization is not easy to hook into.
+///
+/// The specializations below hand-implement the serialization logic for the
+/// inline replay. Instead of using the function from the registry, it uses the
+/// one passed into the macro.
+template <typename Signature> struct replay_char_ptr;
+template <typename Result, typename Class, typename... Args>
+struct replay_char_ptr<Result (Class::*)(Args...) const> {
+  template <Result (Class::*m)(Args...) const> struct method_const {
+    static Result doit(Recorder &recorder, Deserializer &deserializer,
+                       Registry &registry, llvm::StringRef signature,
+                       char *str) {
+      unsigned id = deserializer.Deserialize<unsigned>();
+      registry.CheckSignature(signature, id);
+
+      Class *c = deserializer.Deserialize<Class *>();
+      deserializer.Deserialize<const char *>();
+      size_t l = deserializer.Deserialize<size_t>();
+      return recorder.ReplayResult<Result>(
+          std::move(deserializer.HandleReplayResult<Result>((c->*m)(str, l))),
+          true);
+    }
+  };
+};
+template <typename Signature> struct replay_char_ptr;
+template <typename Result, typename Class, typename... Args>
+struct replay_char_ptr<Result (Class::*)(Args...)> {
+  template <Result (Class::*m)(Args...)> struct method {
+    static Result doit(Recorder &recorder, Deserializer &deserializer,
+                       Registry &registry, llvm::StringRef signature,
+                       char *str) {
+      unsigned id = deserializer.Deserialize<unsigned>();
+      registry.CheckSignature(signature, id);
+
+      Class *c = deserializer.Deserialize<Class *>();
+      deserializer.Deserialize<const char *>();
+      size_t l = deserializer.Deserialize<size_t>();
+      return recorder.ReplayResult<Result>(
+          std::move(deserializer.HandleReplayResult<Result>((c->*m)(str, l))),
+          true);
+    }
+  };
+};
+template <typename Result, typename... Args>
+struct replay_char_ptr<Result (*)(Args...)> {
+  template <Result (*m)(Args...)> struct method_static {
+    static Result doit(Recorder &recorder, Deserializer &deserializer,
+                       Registry &registry, llvm::StringRef signature,
+                       char *str) {
+      unsigned id = deserializer.Deserialize<unsigned>();
+      registry.CheckSignature(signature, id);
+
+      deserializer.Deserialize<const char *>();
+      size_t l = deserializer.Deserialize<size_t>();
+      return recorder.ReplayResult<Result>(
+          std::move(deserializer.HandleReplayResult<Result>((*m)(str, l))),
+          true);
+    }
+  };
+};
+
 template <typename Signature> struct char_ptr_redirect;
 template <typename Result, typename Class>
 struct char_ptr_redirect<Result (Class::*)(char *, size_t) const> {
   template <Result (Class::*m)(char *, size_t) const> struct method_const {
     static Result doit(Class *c, char *s, size_t l) {
       char *buffer = reinterpret_cast<char *>(calloc(l, sizeof(char)));
       return (c->*m)(buffer, l);
     }
   };
 };
 template <typename Result, typename Class>
 struct char_ptr_redirect<Result (Class::*)(char *, size_t)> {
   template <Result (Class::*m)(char *, size_t)> struct method {
     static Result doit(Class *c, char *s, size_t l) {
       char *buffer = reinterpret_cast<char *>(calloc(l, sizeof(char)));
       return (c->*m)(buffer, l);
     }
   };
 };
 
 template <typename Result>
 Result char_ptr_redirect_static(Result (*f)(char *, size_t), char *s,
                                 size_t l) {
   char *buffer = reinterpret_cast<char *>(calloc(l, sizeof(char)));
   return f(buffer, l);
 }
+/// }
 
 } // namespace repro
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_REPRODUCERINSTRUMENTATION_H

lldb/source/API/SBDebugger.cpp

SBSourceManager SBDebugger::GetSourceManager() {
   LLDB_RECORD_METHOD_NO_ARGS(lldb::SBSourceManager, SBDebugger,
                              GetSourceManager);
 
   SBSourceManager sb_source_manager(*this);
   return LLDB_RECORD_RESULT(sb_source_manager);
 }
 
 bool SBDebugger::GetDefaultArchitecture(char *arch_name, size_t arch_name_len) {
-  LLDB_RECORD_STATIC_METHOD(bool, SBDebugger, GetDefaultArchitecture,
-                            (char *, size_t), "", arch_name_len);
+  LLDB_RECORD_CHAR_PTR_STATIC_METHOD(bool, SBDebugger, GetDefaultArchitecture,
+                                     (char *, size_t), arch_name, "",
+                                     arch_name_len);
 
   if (arch_name && arch_name_len) {
     ArchSpec default_arch = Target::GetDefaultArchitecture();
 
     if (default_arch.IsValid()) {
       const std::string &triple_str = default_arch.GetTriple().str();
       if (!triple_str.empty())
         ::snprintf(arch_name, arch_name_len, "%s", triple_str.c_str());

lldb/source/API/SBFileSpec.cpp

void SBFileSpec::SetDirectory(const char *directory) {
 
   if (directory && directory[0])
     m_opaque_up->GetDirectory().SetCString(directory);
   else
     m_opaque_up->GetDirectory().Clear();
 }
 
 uint32_t SBFileSpec::GetPath(char *dst_path, size_t dst_len) const {
-  LLDB_RECORD_METHOD_CONST(uint32_t, SBFileSpec, GetPath, (char *, size_t), "",
-                           dst_len);
+  LLDB_RECORD_CHAR_PTR_METHOD_CONST(uint32_t, SBFileSpec, GetPath,
+                                    (char *, size_t), dst_path, "", dst_len);
 
   uint32_t result = m_opaque_up->GetPath(dst_path, dst_len);
 
   if (result == 0 && dst_path && dst_len > 0)
     *dst_path = '\0';
   return result;
 }
 

lldb/source/API/SBProcess.cpp

if (process_sp) {
     Status error;
     ret_val = process_sp->PutSTDIN(src, src_len, error);
   }
 
   return ret_val;
 }
 
 size_t SBProcess::GetSTDOUT(char *dst, size_t dst_len) const {
-  LLDB_RECORD_METHOD_CONST(size_t, SBProcess, GetSTDOUT, (char *, size_t), "",
-                           dst_len);
+  LLDB_RECORD_CHAR_PTR_METHOD_CONST(size_t, SBProcess, GetSTDOUT,
+                                    (char *, size_t), dst, "", dst_len);
 
   size_t bytes_read = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
     Status error;
     bytes_read = process_sp->GetSTDOUT(dst, dst_len, error);
   }
 
   return bytes_read;
 }
 
 size_t SBProcess::GetSTDERR(char *dst, size_t dst_len) const {
-  LLDB_RECORD_METHOD_CONST(size_t, SBProcess, GetSTDERR, (char *, size_t), "",
-                           dst_len);
+  LLDB_RECORD_CHAR_PTR_METHOD_CONST(size_t, SBProcess, GetSTDERR,
+                                    (char *, size_t), dst, "", dst_len);
 
   size_t bytes_read = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
     Status error;
     bytes_read = process_sp->GetSTDERR(dst, dst_len, error);
   }
 
   return bytes_read;
 }
 
 size_t SBProcess::GetAsyncProfileData(char *dst, size_t dst_len) const {
-  LLDB_RECORD_METHOD_CONST(size_t, SBProcess, GetAsyncProfileData,
-                           (char *, size_t), "", dst_len);
+  LLDB_RECORD_CHAR_PTR_METHOD_CONST(size_t, SBProcess, GetAsyncProfileData,
+                                    (char *, size_t), dst, "", dst_len);
 
   size_t bytes_read = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
     Status error;
     bytes_read = process_sp->GetAsyncProfileData(dst, dst_len, error);
   }
 

lldb/source/API/SBStructuredData.cpp

 bool SBStructuredData::GetBooleanValue(bool fail_value) const {
   LLDB_RECORD_METHOD_CONST(bool, SBStructuredData, GetBooleanValue, (bool),
                            fail_value);
 
   return (m_impl_up ? m_impl_up->GetBooleanValue(fail_value) : fail_value);
 }
 
 size_t SBStructuredData::GetStringValue(char *dst, size_t dst_len) const {
-  LLDB_RECORD_METHOD_CONST(size_t, SBStructuredData, GetStringValue,
-                           (char *, size_t), "", dst_len);
+  LLDB_RECORD_CHAR_PTR_METHOD_CONST(size_t, SBStructuredData, GetStringValue,
+                                    (char *, size_t), dst, "", dst_len);
 
   return (m_impl_up ? m_impl_up->GetStringValue(dst, dst_len) : 0);
 }
 
 namespace lldb_private {
 namespace repro {
 
 template <> void RegisterMethods<SBStructuredData>(Registry &R) {

lldb/source/API/SBThread.cpp

if (!info)
     return LLDB_RECORD_RESULT(threads);
 
   threads = process_sp->GetInstrumentationRuntime(type)
                 ->GetBacktracesFromExtendedStopInfo(info);
   return LLDB_RECORD_RESULT(threads);
 }
 
 size_t SBThread::GetStopDescription(char *dst, size_t dst_len) {
-  LLDB_RECORD_METHOD(size_t, SBThread, GetStopDescription, (char *, size_t), "",
-                     dst_len);
+  LLDB_RECORD_CHAR_PTR_METHOD(size_t, SBThread, GetStopDescription,
+                              (char *, size_t), dst, "", dst_len);
 
   std::unique_lock<std::recursive_mutex> lock;
   ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
 
   if (dst)
     *dst = 0;
 
   if (!exe_ctx.HasThreadScope())