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