[Lldb-commits] [PATCH] D77602: [lldb/Reproducers] Support new replay mode: passive replay

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D77602#1973804 <https://reviews.llvm.org/D77602#1973804>, @JDevlieghere wrote:

> - 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 <https://godbolt.org/z/oBeTbT>:

  #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)



================
Comment at: lldb/source/API/SBReproducer.cpp:125
+  repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
+  loader->SetPassiveReplay(true);
   return nullptr;
----------------
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` ?)


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

https://reviews.llvm.org/D77602