[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you. I think this looks much better now.

It occurred to me that the (de)serializer classes are fully standalone. Since they already have a comprehensive test suite, do you think it would make sense to split them off into a separate patch, which can be committed separately?



================
Comment at: include/lldb/API/SBReproducer.h:19
+public:
+  bool Replay() const;
+};
----------------
Should this be static?


================
Comment at: include/lldb/Utility/ReproducerInstrumentation.h:290-291
+protected:
+  /// Initialize the registry by registering function.
+  virtual void Init() = 0;
+
----------------
Is this function necessary? It seems like the subclass could easily achieve this by just calling `Register` from its constructor. (I also don't see it being called anywhere).


================
Comment at: include/lldb/Utility/ReproducerInstrumentation.h:294
+  /// Register the given replayer for a function (and the ID mapping).
+  void DoRegister(uintptr_t RunID, Replayer *replayer);
+
----------------
How about having this function take a unique_ptr, and then store that unique_ptr in (e.g.) the `m_ids` map. It'd be nice not to leak _every_ object we create in this patch.


================
Comment at: include/lldb/Utility/ReproducerInstrumentation.h:298
+  /// Mapping of function addresses to replayers and their ID.
+  std::map<uintptr_t, std::pair<Replayer *, unsigned>> m_sbreplayers;
+
----------------
s/m_sbreplayers/m_replayers`


================
Comment at: include/lldb/Utility/ReproducerInstrumentation.h:393
+  void Serialize(void *v) {
+    // Do nothing.
+  }
----------------
Since you don't support `void *`, maybe this should be `llvm_unreachable`, or `=delete`?


================
Comment at: include/lldb/Utility/ReproducerInstrumentation.h:432
+    if (Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_API))
+      LLDB_LOG(log, "#%u '%s'", id, m_pretty_func);
+
----------------
I guess I forgot to mention that LLDB_LOG uses the `llvm::formatv` syntax for substitutions (which is why it's able to handle `llvm::StringRef`, and non-null-terminated strings. So this would just be `LLDB_LOG(log, "#{0} '{1}'", id, m_pretty_func)`.


================
Comment at: source/API/SBReproducer.cpp:42-59
+static void SetInputFileHandleRedirect(SBDebugger *t, FILE *, bool) {
+  repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
+  if (!loader)
+    return;
+
+  FileSpec fs = loader->GetFile<SBInfo>();
+  if (!fs)
----------------
Unused functions.


================
Comment at: source/API/SBReproducerPrivate.h:121
+public:
+  SBDeserializer(llvm::StringRef buffer = {}) : m_buffer(buffer), m_offset(0) {}
+
----------------
JDevlieghere wrote:
> labath wrote:
> > Instead of the m_offset variable, what do you think about just `drop_front`ing the appropriate amount of bytes when you are done with the? It doesn't look like you'll ever need to go back to an earlier point in the stream...
> We don't go back, but the buffer is the backing memory for deserialized c-strings.
Well, the buffer is a StringRef, so it doesn't really "back" anything :), but if you think this is more understandable then I can live with it.


================
Comment at: source/API/SBReproducerPrivate.h:182
+    typedef typename std::remove_reference<T>::type UnderlyingT;
+    // If this is a reference to a fundamental type we just read its value.
+    return *m_index_to_object.template GetObjectForIndex<UnderlyingT>(
----------------
JDevlieghere wrote:
> labath wrote:
> > Is this correct? I would expect the fundamental references to not go through this overload at all...
> Pretty sure, but please elaborate on why you think that. 
I am referring to the comment in the method (the implementation seems fine). It says "If this is a reference to a fundamental type, ...", but fundamental types should go through the `FundamentalReferenceTag` overload, right?


================
Comment at: source/API/SBReproducerPrivate.h:453
+public:
+  SBSerializer(llvm::raw_ostream &stream = llvm::outs()) : m_stream(stream) {}
+
----------------
JDevlieghere wrote:
> labath wrote:
> > Is this default value used for anything? It don't see it being used, and it doesn't seem particularly useful anyway, as you'd just get a lot of binary junk on stdout.
> I'm 100% sure it's needed because I got rid of it only to find out we couldn't do without it. I'm not entirely sure anymore, but I think we didn't know if the function was going to have a (useful) return value or not.
I don't see how the default stream argument could have anything to do with whether a function has a return value. Are you sure you replied to the right comment?


================
Comment at: source/API/SBReproducerPrivate.h:525-533
+  void RecordOmittedResult() {
+    assert(!m_expect_result_recored && "Did you forget SB_RECORD_RESULT?");
+    if (m_result_recorded)
+      return;
+    if (!ShouldCapture())
+      return;
+
----------------
Instead of this function, could you just do the following:
- have the `void` version of `Record` automatically write the extra `0` (and set `m_result_recorded` to `true`)
- have the destructor `assert(m_result_recorded)`

I'm not 100% sure it would work, but it looks like it would be a lot simpler if it did.


================
Comment at: source/Utility/ReproducerInstrumentation.cpp:47
+    unsigned id = deserializer.Deserialize<unsigned>();
+    if (log)
+      LLDB_LOG(log, "Replaying function #%u", id);
----------------
(LLDB_LOG already checks for null-ness of the log argument)


================
Comment at: tools/driver/Driver.cpp:913-917
+  SBReproducer reproducer;
+  if (reproducer.Replay()) {
+    return 0;
+  }
+
----------------
JDevlieghere wrote:
> labath wrote:
> > The driver should know whether it is replaying things or not, right? Can we have it call `Replay` only if replay mode has actually been requested?
> Replay will check if we're in replay mode, which I think is a more canonical way to check it. Happy to change this if you disagree.
I find it weird because my intuitive understanding of the api would be that `Replay` should return an error if the replay itself failed, and not simply because replay was never enabled in the first place. And it doesn't seem like the error path should be on the common case. If we wanted to have the canonical way to check this live on the other side of the API boundary, then we should have something like `SBReproducer.GetMode`. However, I don't think that's needed, as this should already known implicitly from the way the API is used:

```
SBInitializerOptions opt;
opt.SetOptionsForReplayMode(...);
if (!SBDebugger::Initialize(opt)) {
  puts("something went wrong during initialization");
  return -1;
}
// From this point on, we know we're in replay mode.
if (!SBReproducer.Replay()) {
  puts("Error running the reproducer");
  return -1;
}
```


================
Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:37-41
+class TestingDeserializer : public Deserializer {
+public:
+  using Deserializer::Deserializer;
+  using Deserializer::GetIndexToObject;
+};
----------------
It looks like `HandleReplayResult` is not covered by these tests. Since this is also the official public way to add objects to the internal index, maybe you could kill two birds with one stone and change the test which pokes at the internal object map to use `HandleReplayResult` instead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56322/new/

https://reviews.llvm.org/D56322