[Lldb-commits] [PATCH] D77602: [lldb/Reproducers] Support inline replay

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D77602