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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
