Page MenuHomePhabricator

[lldb/Reproducers] Support new replay mode: passive replay

Authored by JDevlieghere on Apr 6 2020, 3:32 PM.



Support inline replay as described in the RFC [1] on lldb-dev. This extends the LLDB_RECORD macros to re-invoke the current function with arguments deserialized from the reproducer. This relies on the function being called in the exact same order as during replay. It uses the same mechanism to toggle the API boundary as during recording, which guarantees that only boundary crossing calls are replayed.

Another major change is that before this patch we could ignore the result of an API call, because we only cared about the observable behavior. Now we need to be able to return the replayed result to the SWIG bindings.

We reuse a lot of the recording infrastructure, which can be a little confusing at first. I'm open to renaming these things in a future patch, but I refrained from doing so here to make it easier to review.


Diff Detail

Event Timeline

JDevlieghere created this revision.Apr 6 2020, 3:32 PM
labath added a comment.Apr 9 2020, 1:17 AM

I haven't looked at the code in details, but some of my high-level concerns are:

  • the LLDB_RECORD_*** macros are getting rather long and repetitive. We should try to find a way to move some of that stuff into a regular function and keep the macros for the stuff that cannot be done elsewhere. I'm thinking that instead of:
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(),                      \
                      &lldb_private::repro::construct<Class Signature>::doit, \
                      __VA_ARGS__);                                           \
     _recorder.RecordResult(this, false);                                     \
   } else if (lldb_private::repro::Deserializer *_deserializer =              \
                  _data.GetDeserializer()) {                                  \
     if (_recorder.ShouldCapture()) {                                         \
       lldb_private::repro::replay_construct<Class Signature>::doit(          \
           _recorder, *_deserializer, _data.GetRegistry(),                    \
           LLVM_PRETTY_FUNCTION);                                             \
     }                                                                        \
   }                                                                          \

we could have something like:

lldb_private::repro::Recorder _recorder(LLVM_PRETTY_FUNCTION,                \
                                          stringify_args(__VA_ARGS__));        \
if (lldb_private::repro::InstrumentationData _data = LLDB_GET_INSTRUMENTATION_DATA() /* is this needed? could the Recorder object tell us when replay is enabled? */) {
  lldb_private::repro::Record/*?*/<lldb_private::repro::construct<Class Signature>>(_recorder, _data/*?*/, anything_else_you_need, __VA_ARGS__)

and have the Record ( HandleAPI ?) function handle the details of what should happen in different reproducer modes. The above assumes that lldb_private::repro::construct becomes a class which has not one but two member functions -- basically the merge of the construct and replay_construct functions from the current patch, so that the Record function can select the right operation based on the current mode.

  • the chopping of LLVM_PRETTY_FUNCTION in Registry::CheckSignature sounds sub-optimal. It feels like we should be able to obtain the "ids" of both "expected" and "actual" methods and compare those (and have the PRETTY_FUNCTION just be there for error reporting purposes).
  • this functionality should have a unit test just like the existing ReproducerInstrumentation.cpp functionality. At that point it should be possible/reasonable to move this patch to the front of the patch set and have the SB wiring come after the underlying infrastructure is set up.
  • Simplify LLDB_RECORD_CONSTRUCTOR macro. The other macros have return statements that need to be inlined in the caller for replay, and the boundary tracking needs to be updated right, so I'm not convinced the extra complexity is worth the deduplication.
  • Use the replayer ID to check the signature.

I didn't get around to adding the unit tests yet. I noticed that this change breaks something and I'm still debugging it. I think it's specific to the unit test though, as the test suite isn't affected.

Add unit tests

JDevlieghere retitled this revision from [lldb/Reproducers] Support inline replay to [lldb/Reproducers] Support new replay mode: passive replay.Apr 13 2020, 12:36 PM
  • Simplify LLDB_RECORD_CONSTRUCTOR macro. The other macros have return statements that need to be inlined in the caller for replay, and the boundary tracking needs to be updated right, so I'm not convinced the extra complexity is worth the deduplication.

Ok, so how about deduplicating at the macro level then? I.e. something like this:

#define RECORD_(Prototype, Method, ...) \
  do_stuff(invoke<Prototype>:: \
  doit<Method>::record, __VA_ARGS__)

#define RECORD(Result, Class, Method, Signature, ...) \
  RECORD_(Result(Class::*)Signature, &Class::Method, this, __VA_ARGS__)
#define RECORD_CONST(Result, Class, Method, Signature, ...) \
  RECORD_(Result(Class::*)Signature const, &Class::Method, this, __VA_ARGS__)
#define RECORD_NO_ARGS(Result, Class, Method) \
  RECORD_(Result(Class::*)(), &Class::Method, this)
#define RECORD_CONST_NO_ARGS(Result, Class, Method) \
  RECORD_(Result(Class::*)() const, &Class::Method, this)
#define RECORD_STATIC(Result, Class, Method, ...) \
  RECORD_(Result(*)Signature, &Class::Method, __VA_ARGS__)
#define RECORD_STATIC_NO_ARGS(Result, Class, Method) \
  RECORD_(Result(*)(), &Class::Method, 0)

The idea is to make all (most?) macros call a common uber-macro which will contain the nontrivial logic. I believe the scheme above is sufficient to merge all (STATIC_)METHOD(_NO_ARGS) macro flavours. The only slightly non-obvious part is the handling of static no-argument methods. The idea is that the static methods will also pass through the invoke wrapper (although they wouldn't really need to), and that the wrapper for no-argument functions would get an extra bogus argument to keep the preprocessor happy. Then, the no-argument wrapper would get specialized to ignore the extra argument. (a different solution might be to pass std::make_tuple(__VA_ARGS__) or the like, and then unwrap the tuple in the wrapper)

I did not attempt to merge constructors into this flow too. Having two copies of this code (one for constructors, one for regular functions) may not be too bad. OTOH, it may be possible to handle those too with an extra argument or two...

(btw, it would probably be good to set up the macro logic in a separate patch, before implementing all the passive replay stuff)


Is this right? calling a replay function from a Capture method looks a bit out of place. (And regardless of the answer, would it be possible to initialize this via the Loader constructor -- maybe from Reproducer::Initialize ?)

JDevlieghere marked an inline comment as done.


I think we're getting closer, though there is still some duplication that I'd like to eradicate.


Now it looks like nothing is calling this function...


What's up with the const_cast ? Should this maybe take a T &t argument and let T be deduced as const U when needed?


superfluous semicolons


All of these replay functions look very similar. Can they be factored into some method on the Recorder class or something. It looks like the only thing that really changes is uintptr_t ID of the method being replayed. That could be passed in as an extra argument.

JDevlieghere marked 4 inline comments as done.

Address Pavel's comments

JDevlieghere marked 2 inline comments as done.Apr 15 2020, 9:31 AM
JDevlieghere added inline comments.

I need to decode the template instantiation error for the details, but we need to return a non-const T.

labath added inline comments.Apr 16 2020, 1:11 AM

It would be good to at least know the reason for that, because const_casts smell..


are you sure that the lldb_private::repro::construct<Class(Args...)>:: qualifications are necessary here?


Could this be done in the initialization code somewhere (inside Reproducer::Initialize, I guess)? We could avoid static variables and get better error handling that way...


This works only accidentally because the compiler NRVO-s the temporary object. But that's not guaranteed to happen and could cause the TestInstrumentationDataRAII destructor to run twice. The simplest way to ensure the behavior you want is to return a unique_ptr to a constructed object (as well as delete all copy operations)...


Maybe an EXPECT_DEATH test case for what happens when the replayed function calls don't line up?

JDevlieghere marked 8 inline comments as done.
JDevlieghere added inline comments.Apr 16 2020, 2:25 PM

The problem is that you cannot cannot bind a non-const lvalue reference of type T& to an rvalue of type T.


Not if we want to keep the SBProvider in API instead of Utility.

labath added inline comments.Apr 17 2020, 12:49 AM

The usual solution to that is to use "universal" references (<typename T> T &&t) plus careful sprinkling with std::forward. Would that work here?

Or a separate overload for const and non-const references?


Right, of course.

Soo... do it one level higher then? Initialize the object in SBReproducer::(Passive)Replay, and use it from here?

JDevlieghere marked an inline comment as done.
  • Remove const_cast
  • Initialize instrumentation data in SBReproducer.cpp
labath accepted this revision.Apr 20 2020, 12:47 AM

Looks good to me.


bad formatting ?

This revision is now accepted and ready to land.Apr 20 2020, 12:47 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2020, 10:17 AM