[Lldb-commits] [PATCH] D58564: [Reproducers] Add command provider

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D58564#1414410 <https://reviews.llvm.org/D58564#1414410>, @JDevlieghere wrote:

> Thanks Pavel. I've updated the patch with your suggestion. I agree it's a lot better :-)
>
> I implemented the logic in the Debugger rather than the SBDebugger because I think the latter should be a thin wrapper, but let me know if you had a particular reason for this.


Thanks Jonas. This looks even simpler than I anticipated. :)

While I agree that SB layer should be as thin as possible, I would say this deserves an exception. My reasoning behind that is that this command capture mechanism is kind of an extended arm of the SB recorder. Like, if we had an oracle, and were able to "instantly" capture the contents of the FILE* at the SB layer, we would just do that, and avoid this shadowing dance altogether. Unfortunately, we cannot see into the future, so we have to have this recorder "shadow" follow the input FILE* whereever it goes and capture the input as it is being read. The cleanest way to me seems to be to have that shadow start tracking the value as soon as it crosses the SB boundary.

Or, looking at it from a different angle, if somebody other than the SBDebugger calls Debugger::SetInputFileHandle, then we probably don't want to capture that input because it did not come from the outside world. (I have no idea why anybody would want to do that, but it does not seem completely out of the question.). This FILE* then most likely came from the filesystem, in which case it would be captured by the FileSystem recorder. Or if it did come through the SB API, but through a different function, then it should have been shadowed as soon it entered that function.

Apart from this I have some inline comments about error handling, but overall, I am very happy with how this is turning out.



================
Comment at: lldb/include/lldb/Utility/Reproducer.h:120
+
+  operator bool() { return !m_ec.operator bool(); }
+
----------------
Remove `operator bool` and the embedded error code. Instead have a static factory function which checks the error and only returns a DataRecorder if the file was created correctly. I'm thinking of something like
```
static std::unique_ptr<DataRecorder> /*or Expected<...> ?*/ Create(FileSpec f) { 
  std::error_code ec;
  auto rec = make_unique<DataRecorder>(f, ec);
  if (!ec)
    return rec;
  return nullptr; /*or the error code*/
}


================
Comment at: lldb/source/Core/Debugger.cpp:933
+  if (repro::Generator *g = repro::Reproducer::Instance().GetGenerator())
+    g->GetOrCreate<repro::CommandProvider>().GetNewDataRecorder();
+
----------------
Why not store this as a field? This way you still can have problems if two debugger objects are active concurrently.


================
Comment at: lldb/source/Utility/Reproducer.cpp:228-229
+                             .str();
+  m_data_recorders.push_back(llvm::make_unique<DataRecorder>(
+      GetRoot().CopyByAppendingPathComponent(filename)));
+  return m_data_recorders.back().get();
----------------
So what happens if creating the file fails here? We abort when the assert in `Debugger::GetInputRecorder` fails? If we're going to crash, it's probably better to do it here, as that's closer to where the actual failure happened (alternatively, we could log a message and exit slightly more gracefully; or log a message and continue without recording).


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

https://reviews.llvm.org/D58564