[Reproducers] SBReproducer framework

[JDevlieghere]

Summary

To capture and replay SB API calls as part of the reproducer, we need a way to serialize and deserialize all function calls into the SB layer. The SBReproducer framework facilitates this. It transparently serializes and deserializes function calls and their arguments, including pointers and references. Most of the code is synthesized using templates, but it's possible to specify a custom deserialization function.

This patch only contains the framework.

For all the details refer to the RFC on the mailing list: http://lists.llvm.org/pipermail/lldb-dev/2019-January/014530.html

[JDevlieghere]

Prototype 2.0

[JDevlieghere]

Rebase on recent reproducer changes.

[JDevlieghere]

I split off the framework (this revision) and the addition of the macros (D57475).

I plan on updating the patch with tests, but I believe it's ready for review in the meantime.

[JDevlieghere]

Add unit testing for supporting classes

[labath]

I have a lot of comments. The two major ones are:

• i think the way you link the tests is in the UB territory. I explain this in detail in one of the inline comments.
• I believe that your unit tests (not just in this patch) focus too much on testing the behavior of a single method, even when that method does not have an easily observable results. For this you then often need to expose internals of the tested class to be able to test some effect of that method on the internal state. This not all bad (i've done it myself sometimes), and it's definitely better that not writing any unit tests. However, it's generally better to test the public interface of a method/class/entity, and in this case, I believe it should be possible. I'm imagining some tests like having a dummy class with a bunch of methods which are annotated with the SB_RECORD macros. Then, in the test you call the methods of this class with some arguments, have the repro framework serialize the calls to a (in-memory?) stream, and verify the contents of that stream. This will test a lot more code -- the (De)Serialize functions are just fancy ways of invoking memcpy, but if you take all of that together with the SB_RECORD, SB_REGISTER macros, it becomes some pretty deep magic that is interesting to test exhaustively (it's fine, but not really interesting to test that Deserialize<T*> and Deserialize<T&> do the right thing for each of the possible fundamental types.) And if you use the deserializer interface to read out the recorded traces (instead of comparing raw bytes), then you can avoid depending on the endianness of the values.

Conversely, you can write a trace file with the serializer interface, and then tell the replayer to invoke the mock class which will verify that it was called with the right arguments.

source/API/SBReproducer.cpp

1 ↗

(On Diff #184644)

I found it weird to have one cpp file implementing two headers (SBReproducer.h and SBReproducerPrivate.h). Can you split it into two files? (This will come out naturally, if we split this up into modules as I mention in one of the other comments.)

69 ↗

(On Diff #184644)

drop {} (that's what you seem to do elsewhere in this file).

source/API/SBReproducerPrivate.h

44 ↗

(On Diff #184644)

Why do you need to remove_const here? If T is const, then const will be added back by the return statement anyway. If T is not const, then remove_const is a noop.

65–69 ↗

(On Diff #184644)

replace by m_mapping.lookup(idx), at which point you can consider dropping this function entirely.

82–93 ↗

(On Diff #184644)

Just curious: What's the advantage of this over just declaring a bunch of Tag classes directly (struct PointerTag{}; struct ReferenceTag{}; ...)?

121 ↗

(On Diff #184644)

Instead of the m_offset variable, what do you think about just drop_fronting 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...

155–161 ↗

(On Diff #184644)

It sounds to me like this could be solved by passing the deserializer to the operator() of the SBReplayers instead of having it a member variable set by the constructor. This design also makes more sense to me, as theoretically there is no reason why all replay calls would have to come from the same deserializer object. You may even want to have a separate deserializer for each thread when you get around to replaying multithreaded recordings.

After this, it may even be possible to make the deserializer object be a local variable in the SBRegistry::Replay function.

167 ↗

(On Diff #184644)

I guess if we don't consider the recording to be "input" then we don't have to handle the case of not having enough bytes in the file extremely gracefully here, but it would still be nice to at least assert that we are not running off the end of the buffer here.

169 ↗

(On Diff #184644)

reinterpret_cast

182 ↗

(On Diff #184644)

Is this correct? I would expect the fundamental references to not go through this overload at all...

303 ↗

(On Diff #184644)

Shouldn't this call g then?

307–309 ↗

(On Diff #184644)

If you gave these more meaningful names, then the comments would be unneeded.

370 ↗

(On Diff #184644)

It looks like this id argument isn't really needed here, as the DoRegister function can just synthesize the id on its own. In fact, the m_id is probably not needed either, as you can just use m_sbreplayers.size()+1 or something.

371 ↗

(On Diff #184644)

assert(m_sbreplayers.find(RunID) == end()) to make sure our RunID magic didn't do something funny?

419–448 ↗

(On Diff #184644)

What's the threading scenario in which you expect this to be called? If multiple threads can call GetIndexForObject simultaneously (even if the objects differ), then I think you need to guard m_mapping with the mutex too, not just the integer variable.

BTW: The m_index can also just be m_mapping.size()+1

453 ↗

(On Diff #184644)

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.

482 ↗

(On Diff #184644)

Will this ever not be an infinite recursion? I am guessing this works because all fundamental types are picked up by the specific overloads below before this function is even invoked.

Maybe if you just inline the m_stream.write(reinterpret_cast<const char *>(&t), sizeof(Type)) thingy here, you can avoid having to define a special Serialize function for all fundamental types.

484 ↗

(On Diff #184644)

It looks like GetIndexForObject returns unsigned

562–563 ↗

(On Diff #184644)

If you wanted to make the updates to g_global_boundary thread-safe, this is not the way to achieve that. This is still going to be racy, so you should at least use the atomic compare-and-exchange operations. However, I don't believe this will still be right thing to do in the multithreaded case, so it may be best do just drop the atomicity until you implement proper threading support.

573–574 ↗

(On Diff #184644)

Move this to the constructor, and use llvm::Optional in the SB_RECORD macros?

This will also avoid having this class muck around with g_global_boundary when we are not recording.

578 ↗

(On Diff #184644)

Inconsistent braces around a single statement.

584–585 ↗

(On Diff #184644)

if you used the LLDB_LOG macro, you wouldn't have to rely on m_pretty_func being implicitly null terminated.

618–626 ↗

(On Diff #184644)

I'm wondering if we shouldn't embed some additional safeties here to make sure the result is always recorded when it should be. It seems to be this class should know whether it should expect an explicit RecordResult call (based on whether we called the void version of Record or not). So, we could have this class assert if we reached the destructor without recording anything, instead of silently recording zero. WDYT?

659 ↗

(On Diff #184644)

__PRETTY_FUNCTION__ is not portable. I think you want LLVM_PRETTY_FUNCTION here.

tools/driver/Driver.cpp

913–917 ↗

(On Diff #184644)

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?

unittests/API/CMakeLists.txt

4–12 ↗

(On Diff #184644)

Hmm.. I'm not sure this is going to work everywhere, but it's certainly going to be weird. liblldb is a shared library which deliberately firewalls all lldb_private symbols. So any reproducer symbols you defined there will not be accessible to your unit tests. I guess the reason include_directories trick kind of works is that most of the reproducer frameworks is implemented in the header, and including that header from the test will cause another copy of all of these symbols to be defined. However, I am still surprised that it does work, as you still have some chunks of this implemented in the cpp file. I guess you were just lucky to not need those symbols in the tests that you have written. Nonetheless, this is going into very murky waters, and I think the fact that you had to link in lldbHost and lldbCore in addition to liblldb (even though they are be included in there) demonstrates it.

I think the best way to address this is to move the core of the repro engine into some other library, which can be safely used from unit tests. After all, the core is not really tied to the SB API, and you could easily use it to record any other interface, if you needed to do that for whatever reason. I think this should go into the Utility module, as it doesn't have any other dependencies. I see that you're including FileSystem.h, but it looks like this is only used in the SetInputFileHandleRedirect function, which is not actually used anywhere! Even when it is used, I think it would make sense for it to live elsewhere, as this is not a part of the replay core, but rather how a particular api wants to replay a particular function signature.

unittests/API/SBReproducerTest.cpp

37–41 ↗

(On Diff #184644)

All of these buffers hard-code endiannes of integers and the representation of floats.

83–85 ↗

(On Diff #184644)

I find this behaviour confusing. Intuitively, I'd expect HasData(n) if buffer has at least n bytes of data left. So in this case I'd expect true in both calls. (In fact I'd probably expect HasData() to be a synonym for HasData(1)). It looks like you're only calling HasData in one place, and you're hardcoding 1 there, so maybe you just want an EOF() function?

[JDevlieghere]

In D56322#1380442, @labath wrote:

I have a lot of comments. The two major ones are:

• i think the way you link the tests is in the UB territory. I explain this in detail in one of the inline comments.

Thanks, you're right and your suggestion makes sense. Keep in mind that the registry's Init method needs to have access to the SB definitions though.

• I believe that your unit tests (not just in this patch) focus too much on testing the behavior of a single method, even when that method does not have an easily observable results. For this you then often need to expose internals of the tested class to be able to test some effect of that method on the internal state. This not all bad (i've done it myself sometimes), and it's definitely better that not writing any unit tests.

It's funny you mention this, maybe I misremember but I recall you commenting on a patch that the current reproducers test were too high level. Maybe we have different views on what unit tests are, but I strongly believe that it's import to tests small *units* of code. They may seem trivial today until someone to decides to refactor them tomorrow. Indeed, we're already planning to move some of this code around and these tests will give me a lot more confidence. This stuff is relatively tricky, hard to debug and easy to get wrong. I strongly believe it's important to have these kind of unit tests, but I also totally agree that they should not be the only tests. See my next comment for more on that :-)

However, it's generally better to test the public interface of a method/class/entity, and in this case, I believe it should be possible.

It's 100% my fault for not making this clearer, but all that is in the pipeline. I wasn't able to work on this as much as I would've wanted the last week so that might have given the wrong impression. Maybe I should've kept the title as WIP, but I didn't want to discourage people from looking at the framework while I was adding this tests. Maybe I should've done this in a separate patch.

I'm imagining some tests like having a dummy class with a bunch of methods which are annotated with the SB_RECORD macros. Then, in the test you call the methods of this class with some arguments, have the repro framework serialize the calls to a (in-memory?) stream, and verify the contents of that stream. This will test a lot more code -- the (De)Serialize functions are just fancy ways of invoking memcpy, but if you take all of that together with the SB_RECORD, SB_REGISTER macros, it becomes some pretty deep magic that is interesting to test exhaustively (it's fine, but not really interesting to test that Deserialize<T*> and Deserialize<T&> do the right thing for each of the possible fundamental types.) And if you use the deserializer interface to read out the recorded traces (instead of comparing raw bytes), then you can avoid depending on the endianness of the values.

Conversely, you can write a trace file with the serializer interface, and then tell the replayer to invoke the mock class which will verify that it was called with the right arguments.

This is very much what I'm planning to do. Coming back to my original point about testing smaller units, this is all great while it works, but it's going to be far from obvious as soon as this breaks. My hope is that if something fundamental breaks, one of the "obvious" unit test would catch it first.

[labath]

In D56322#1380996, @JDevlieghere wrote:

In D56322#1380442, @labath wrote:

I have a lot of comments. The two major ones are:

• i think the way you link the tests is in the UB territory. I explain this in detail in one of the inline comments.

Thanks, you're right and your suggestion makes sense. Keep in mind that the registry's Init method needs to have access to the SB definitions though.

Yes, in this world, the I guess the version of the registry in the utility folder would be just an abstract class which provides the building blocks to intercept "any" api. And the API folder would contain an instantiation of this class which intercepts the SB methods. (And tests would instantiate/subclass/whatever it to intercept the mock methods.)

• I believe that your unit tests (not just in this patch) focus too much on testing the behavior of a single method, even when that method does not have an easily observable results. For this you then often need to expose internals of the tested class to be able to test some effect of that method on the internal state. This not all bad (i've done it myself sometimes), and it's definitely better that not writing any unit tests.

It's funny you mention this, maybe I misremember but I recall you commenting on a patch that the current reproducers test were too high level. Maybe we have different views on what unit tests are, but I strongly believe that it's import to tests small *units* of code. They may seem trivial today until someone to decides to refactor them tomorrow. Indeed, we're already planning to move some of this code around and these tests will give me a lot more confidence. This stuff is relatively tricky, hard to debug and easy to get wrong. I strongly believe it's important to have these kind of unit tests, but I also totally agree that they should not be the only tests. See my next comment for more on that :-)

Yes, that is funny, and a bit ironic. :) Though I think we're on the same page here. My comment was more about the lack of higher-level (or, shall we call them middle-level?) tests, then the presence of these low-level ones. Lldb has a very big deficiency in low-level tests, so I'd rather have more (even if some end up being "change-detector tests") than less (and miss some opportunities to increase coverage) of them.

Bottom line: if you write some tests like the one I described previously, I am going to be _very_ happy. I have no problem with the existing tests staying, if you find them valuable. Just please make sure they work on big-endian platforms too. :)

[JDevlieghere]

Address Pavel's inline comments, modulo

• Fixing the ODR violation (should be easier to verify inline comments are addressed before moving the code).
• Fixing the endianness in the unit test (I originally considered ifdef'ing this, but that combined with the float-encoding made me reconsider).

source/API/SBReproducerPrivate.h

82–93 ↗

(On Diff #184644)

Mostly preference, but the structs are less code so I've simplified it.

121 ↗

(On Diff #184644)

We don't go back, but the buffer is the backing memory for deserialized c-strings.

182 ↗

(On Diff #184644)

Pretty sure, but please elaborate on why you think that.

453 ↗

(On Diff #184644)

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.

tools/driver/Driver.cpp

913–917 ↗

(On Diff #184644)

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.

[JDevlieghere]

In D56322#1381254, @labath wrote:

In D56322#1380996, @JDevlieghere wrote:

In D56322#1380442, @labath wrote:

I have a lot of comments. The two major ones are:

• i think the way you link the tests is in the UB territory. I explain this in detail in one of the inline comments.

Thanks, you're right and your suggestion makes sense. Keep in mind that the registry's Init method needs to have access to the SB definitions though.

Yes, in this world, the I guess the version of the registry in the utility folder would be just an abstract class which provides the building blocks to intercept "any" api. And the API folder would contain an instantiation of this class which intercepts the SB methods. (And tests would instantiate/subclass/whatever it to intercept the mock methods.)

• I believe that your unit tests (not just in this patch) focus too much on testing the behavior of a single method, even when that method does not have an easily observable results. For this you then often need to expose internals of the tested class to be able to test some effect of that method on the internal state. This not all bad (i've done it myself sometimes), and it's definitely better that not writing any unit tests.

It's funny you mention this, maybe I misremember but I recall you commenting on a patch that the current reproducers test were too high level. Maybe we have different views on what unit tests are, but I strongly believe that it's import to tests small *units* of code. They may seem trivial today until someone to decides to refactor them tomorrow. Indeed, we're already planning to move some of this code around and these tests will give me a lot more confidence. This stuff is relatively tricky, hard to debug and easy to get wrong. I strongly believe it's important to have these kind of unit tests, but I also totally agree that they should not be the only tests. See my next comment for more on that :-)

Yes, that is funny, and a bit ironic. :) Though I think we're on the same page here. My comment was more about the lack of higher-level (or, shall we call them middle-level?) tests, then the presence of these low-level ones. Lldb has a very big deficiency in low-level tests, so I'd rather have more (even if some end up being "change-detector tests") than less (and miss some opportunities to increase coverage) of them.

Great :-)

Bottom line: if you write some tests like the one I described previously, I am going to be _very_ happy. I have no problem with the existing tests staying, if you find them valuable. Just please make sure they work on big-endian platforms too. :)

Awesome, yeah I didn't consider the float encoding, I'll just do a roundtrip test.

[JDevlieghere]

• Move framework in Utility
• Update tests

[labath]

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?

include/lldb/API/SBReproducer.h

19 ↗

(On Diff #184874)

Should this be static?

include/lldb/Utility/ReproducerInstrumentation.h

290–291 ↗

(On Diff #184874)

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

294 ↗

(On Diff #184874)

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.

298 ↗

(On Diff #184874)

s/m_sbreplayers/m_replayers`

393 ↗

(On Diff #184874)

Since you don't support void *, maybe this should be llvm_unreachable, or =delete?

432 ↗

(On Diff #184874)

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

source/API/SBReproducer.cpp

42–59 ↗

(On Diff #184874)

Unused functions.

source/API/SBReproducerPrivate.h

525–533 ↗

(On Diff #184825)

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.

121 ↗

(On Diff #184644)

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.

182 ↗

(On Diff #184644)

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?

453 ↗

(On Diff #184644)

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?

source/Utility/ReproducerInstrumentation.cpp

47 ↗

(On Diff #184874)

(LLDB_LOG already checks for null-ness of the log argument)

tools/driver/Driver.cpp

913–917 ↗

(On Diff #184644)

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;
}

unittests/Utility/ReproducerInstrumentationTest.cpp

37–41 ↗

(On Diff #184874)

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.

[JDevlieghere]

In D56322#1382971, @labath wrote:

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?

Sure, makes sense to me, please see D57714

[JDevlieghere]

Add context

[JDevlieghere]

(I plan to add another test that deals with returning objects)

[labath]

Thank you. I like the new test, but see my comment about avoiding duplicating macros in the test. Besides returning objects, I can think of a couple of more interesting scenarios to cover:

• recursive calls (to exercise the g_global_boundary thingy)
• having two objects with identical this pointers (you can use placement new to guarantee that)
• calling the "custom" function during replay

unittests/Utility/ReproducerInstrumentationTest.cpp

60–62 ↗

(On Diff #185208)

googletest already has apis for this: EXPECT_FLOAT/DOUBLE_EQ or EXPECT_NEAR.

67–122 ↗

(On Diff #185208)

Since these macros are the actual "public" interface of the instrumentation framework, and they do contain non-trivial logic, I think it would be nice to test the real macros, instead of redefining them here. (It would also be nice to simply avoid duplicating code.)

Would something like this work:

• move the macros to ReproducerInstrumentation.h (maybe also rename to LLDB_RECORD_blah_blah).
• parameterize them on a macro for getting the instrumentation classes. So, something like:

#define LLDB_RECORD_CONSTRUCTOR(Class, Signature, ...)                           \
  if (InstrumentationData data = LLDB_GET_INSTRUMENTATION_DATA()) {  \
    lldb_private::repro::Recorder recorder(                                 \
        data.GetSerializer(),                   \
        data.GetRegistry(),                     \
        LLVM_PRETTY_FUNCTION);                                                 \
    sb_recorder.Record(&lldb_private::repro::construct<Class Signature>::doit, \
                       __VA_ARGS__);                                           \
    sb_recorder.RecordResult(this);                                            \
  }

where InstrumentationData is essentially std::pair<Serializer *, Registry *> with a suitable operator bool. Then the tests could define LLDB_GET_INSTRUMENTATION_DATA simply to InstrumentationData(&g_serializer, &g_registry) and the "real" code could do something like

#define LLDB_GET_INSTRUMENTATION_DATA lldb_private::repro::GetInstrumentationData()

InstrumentationData GetInstrumentationData() {
  auto *g = lldb_private::repro::Reproducer::Instance().GetGenerator();
  if (!g) return {};
  auto &p = g->GetOrCreate<SBProvider>();
  return {p.GetSerializer(), p.GetRegistry();
}

143–148 ↗

(On Diff #185208)

This seems rather pointless, as you've just set the variables to those values. What are you testing here? The compiler?

384 ↗

(On Diff #185208)

Make g_serializer llvm:Optional, and then you can avoid needing it be copyable (I think making the Serializer class copyable is a bad idea for the same reason than making raw_ostream copyable would be a bad idea)

399–404 ↗

(On Diff #185208)

access variables via the class (InstrumentedFoo::g_a) to make it clear that they are static.

[labath]

unittests/Utility/ReproducerInstrumentationTest.cpp

399–404 ↗

(On Diff #185208)

Actually, what do you think about this pattern:

• have the variables be member variables
• add a Validate method to the InstrumentedFoo class. This method would also be instrumented, and it would check that the members are set to the correct value
• call the method during recording and it will be automatically called during replay. Then all you need to do in the test is check that Validate was called.

I am hoping that this will make things more manageable once you have multiple instrumented objects floating around in the test. Also, this would implicitly test that the replay framework calls the member functions on the right object (e.g., right now this would succeed if the framework just created a fresh new object for each call).

[labath]

I think we're very close. This batch of comments is just about small improvements to the tests. It looks like the Validate thingy wasn't such a clever idea, since the fact that the framework creates extra copies means that there are some unvalidated copies floating around. Still, I think it's worth it overall, because it tests the bahavior we want more closely than if we just went through global variables (however, do see my comments on the global variables to make sure that validation did indeed take place).

include/lldb/Utility/ReproducerInstrumentation.h

594–595 ↗

(On Diff #185431)

I guess this comment is no longer true. We should only ever create this object if we are recording.

source/API/SBReproducerPrivate.h

23 ↗

(On Diff #185431)

If it's all the same to you, i'd prefer to have this be a function-like macro (so just add () after the macro name here, and also add () to all macro "invocations").

61 ↗

(On Diff #185431)

inline (or move to .cpp)

unittests/Utility/ReproducerInstrumentationTest.cpp

50–52 ↗

(On Diff #185431)

I guess this is no longer used?

62 ↗

(On Diff #185431)

I think it's still important to independently check that Validate was called, IIUC, the framework never deletes the objects it creates, so this wouldn't actually check anything during the replay. Even if it did, this opens up the possibility that the Replay method decides to do absolutely nothing for some reason, in which case the test would still succeed. The idea for stashing the this pointer from the other comment could be reused for this purpose too.

443 ↗

(On Diff #185431)

destroy the old object before creating a new one in its place (foo->~InstrumentedFoo()).

455 ↗

(On Diff #185431)

A good thing to check here would be to make sure that the framework created actually created two instances of this object. As it stands now, if the framework just decided to keep calling methods on the original object, the test would probably still succeed. One way to achieve that would be to enhance the Validate method to stash the this pointer into an array somewhere. Then you could check that the array contains two elements (and that they are different).

479–480 ↗

(On Diff #185431)

Add a check that these two methods are called with the same object during replay. The methods could store the object pointer instead of just a plain flag, and then Validate could verify their equality.

[JDevlieghere]

Feedback Pavel.

[labath]

This looks good to me. Thank you for your patience.

lldb/source/Utility/ReproducerInstrumentation.cpp

91 ↗

(On Diff #185585)

old macro name in assert message

[JDevlieghere]

In D56322#1387357, @labath wrote:

This looks good to me. Thank you for your patience.

My pleasure, thank you for the thorough review!

[labath]

Btw, I've just noticed that the files you've added here still have the old license header.

[lebedev.ri]

In D56322#1389013, @labath wrote:

Btw, I've just noticed that the files you've added here still have the old license header.

Would be good to get at least an automatic Herald rule for this
I suspect there might be more occurrences of patches/diffs/commits with old license in the time to come.

lldb/trunk/include/lldb/API/SBReproducer.h

+//===-- SBReproducer.h ------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_API_SBREPRODUCER_H
+#define LLDB_API_SBREPRODUCER_H
+
+#include "lldb/lldb-defines.h"
+
+namespace lldb {
+
+class LLDB_API SBReproducer {
+public:
+  static bool Replay();
+};
+
+} // namespace lldb
+
+#endif

lldb/trunk/include/lldb/Utility/ReproducerInstrumentation.h

 //===-- ReproducerInstrumentation.h -----------------------------*- C++ -*-===//
-//
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_REPRODUCER_INSTRUMENTATION_H
 #define LLDB_UTILITY_REPRODUCER_INSTRUMENTATION_H
 
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Logging.h"
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ErrorHandling.h"
 
 #include <map>
-#include <mutex>
+
+#define LLDB_REGISTER_CONSTRUCTOR(Class, Signature)                            \
+  Register<Class * Signature>(&construct<Class Signature>::doit)
+#define LLDB_REGISTER_METHOD(Result, Class, Method, Signature)                 \
+  Register(&invoke<Result(Class::*) Signature>::method<&Class::Method>::doit)
+#define LLDB_REGISTER_METHOD_CONST(Result, Class, Method, Signature)           \
+  Register(&invoke<Result(Class::*)                                            \
+                       Signature const>::method_const<&Class::Method>::doit)
+#define LLDB_REGISTER_STATIC_METHOD(Result, Class, Method, Signature)          \
+  Register<Result Signature>(static_cast<Result(*) Signature>(&Class::Method))
+
+#define LLDB_RECORD_CONSTRUCTOR(Class, Signature, ...)                         \
+  if (lldb_private::repro::InstrumentationData data =                          \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    lldb_private::repro::Recorder sb_recorder(                                 \
+        data.GetSerializer(), data.GetRegistry(), LLVM_PRETTY_FUNCTION);       \
+    sb_recorder.Record(&lldb_private::repro::construct<Class Signature>::doit, \
+                       __VA_ARGS__);                                           \
+    sb_recorder.RecordResult(this);                                            \
+  }
+
+#define LLDB_RECORD_CONSTRUCTOR_NO_ARGS(Class)                                 \
+  if (lldb_private::repro::InstrumentationData data =                          \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    lldb_private::repro::Recorder sb_recorder(                                 \
+        data.GetSerializer(), data.GetRegistry(), LLVM_PRETTY_FUNCTION);       \
+    sb_recorder.Record(&lldb_private::repro::construct<Class()>::doit);        \
+    sb_recorder.RecordResult(this);                                            \
+  }
+
+#define LLDB_RECORD_METHOD(Result, Class, Method, Signature, ...)              \
+  llvm::Optional<lldb_private::repro::Recorder> sb_recorder;                   \
+  if (lldb_private::repro::InstrumentationData data =                          \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    sb_recorder.emplace(data.GetSerializer(), data.GetRegistry(),              \
+                        LLVM_PRETTY_FUNCTION);                                 \
+    sb_recorder->Record(                                                       \
+        &lldb_private::repro::invoke<Result(                                   \
+            Class::*) Signature>::method<&Class::Method>::doit,                \
+        this, __VA_ARGS__);                                                    \
+  }
+
+#define LLDB_RECORD_METHOD_CONST(Result, Class, Method, Signature, ...)        \
+  llvm::Optional<lldb_private::repro::Recorder> sb_recorder;                   \
+  if (lldb_private::repro::InstrumentationData data =                          \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    sb_recorder.emplace(data.GetSerializer(), data.GetRegistry(),              \
+                        LLVM_PRETTY_FUNCTION);                                 \
+    sb_recorder->Record(                                                       \
+        &lldb_private::repro::invoke<Result(                                   \
+            Class::*) Signature const>::method_const<&Class::Method>::doit,    \
+        this, __VA_ARGS__);                                                    \
+  }
+
+#define LLDB_RECORD_METHOD_NO_ARGS(Result, Class, Method)                      \
+  llvm::Optional<lldb_private::repro::Recorder> sb_recorder;                   \
+  if (lldb_private::repro::InstrumentationData data =                          \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    sb_recorder.emplace(data.GetSerializer(), data.GetRegistry(),              \
+                        LLVM_PRETTY_FUNCTION);                                 \
+    sb_recorder->Record(&lldb_private::repro::invoke<Result (                  \
+                            Class::*)()>::method<&Class::Method>::doit,        \
+                        this);                                                 \
+  }
+
+#define LLDB_RECORD_METHOD_CONST_NO_ARGS(Result, Class, Method)                \
+  llvm::Optional<lldb_private::repro::Recorder> sb_recorder;                   \
+  if (lldb_private::repro::InstrumentationData data =                          \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    sb_recorder.emplace(data.GetSerializer(), data.GetRegistry(),              \
+                        LLVM_PRETTY_FUNCTION);                                 \
+    sb_recorder->Record(                                                       \
+        &lldb_private::repro::invoke<Result (                                  \
+            Class::*)() const>::method_const<&Class::Method>::doit,            \
+        this);                                                                 \
+  }
+
+#define LLDB_RECORD_STATIC_METHOD(Result, Class, Method, Signature, ...)       \
+  llvm::Optional<lldb_private::repro::Recorder> sb_recorder;                   \
+  if (lldb_private::repro::InstrumentationData data =                          \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    sb_recorder.emplace(data.GetSerializer(), data.GetRegistry(),              \
+                        LLVM_PRETTY_FUNCTION);                                 \
+    sb_recorder->Record(static_cast<Result(*) Signature>(&Class::Method),      \
+                        __VA_ARGS__);                                          \
+  }
+
+#define LLDB_RECORD_STATIC_METHOD_NO_ARGS(Result, Class, Method)               \
+  llvm::Optional<lldb_private::repro::Recorder> sb_recorder;                   \
+  if (lldb_private::repro::InstrumentationData data =                          \
+          LLDB_GET_INSTRUMENTATION_DATA()) {                                   \
+    sb_recorder.emplace(data.GetSerializer(), data.GetRegistry(),              \
+                        LLVM_PRETTY_FUNCTION);                                 \
+    sb_recorder->Record(static_cast<Result (*)()>(&Class::Method));            \
+  }
+
+#define LLDB_RECORD_RESULT(Result)                                             \
+  sb_recorder ? sb_recorder->RecordResult(Result) : Result;
 
 namespace lldb_private {
 namespace repro {
 
 /// Mapping between serialized indices and their corresponding objects.
 ///
 /// This class is used during replay to map indices back to in-memory objects.
 ///
public:
 
   /// All returned types are recorded, even when the function returns a void.
   /// The latter requires special handling.
   void HandleReplayResultVoid() {
     unsigned result = Deserialize<unsigned>();
     assert(result == 0);
   }
 
-protected:
-  IndexToObject &GetIndexToObject() { return m_index_to_object; }
-
 private:
   template <typename T> T Read(ValueTag) {
     assert(HasData(sizeof(T)));
     T t;
     std::memcpy(reinterpret_cast<char *>(&t), m_buffer.data(), sizeof(T));
     m_buffer = m_buffer.drop_front(sizeof(T));
     return t;
   }
template <> struct DeserializationHelper<> {
   template <typename Result, typename... Deserialized> struct deserialized {
     static Result doit(Deserializer &deserializer, Result (*f)(Deserialized...),
                        Deserialized... d) {
       return f(d...);
     }
   };
 };
 
+/// The replayer interface.
+struct Replayer {
+  virtual ~Replayer() {}
+  virtual void operator()(Deserializer &deserializer) const = 0;
+};
+
+/// The default replayer deserializes the arguments and calls the function.
+template <typename Signature> struct DefaultReplayer;
+template <typename Result, typename... Args>
+struct DefaultReplayer<Result(Args...)> : public Replayer {
+  DefaultReplayer(Result (*f)(Args...)) : Replayer(), f(f) {}
+
+  void operator()(Deserializer &deserializer) const override {
+    deserializer.HandleReplayResult(
+        DeserializationHelper<Args...>::template deserialized<Result>::doit(
+            deserializer, f));
+  }
+
+  Result (*f)(Args...);
+};
+
+/// Partial specialization for function returning a void type. It ignores the
+/// (absent) return value.
+template <typename... Args>
+struct DefaultReplayer<void(Args...)> : public Replayer {
+  DefaultReplayer(void (*f)(Args...)) : Replayer(), f(f) {}
+
+  void operator()(Deserializer &deserializer) const override {
+    DeserializationHelper<Args...>::template deserialized<void>::doit(
+        deserializer, f);
+    deserializer.HandleReplayResultVoid();
+  }
+
+  void (*f)(Args...);
+};
+
+/// The registry contains a unique mapping between functions and their ID. The
+/// IDs can be serialized and deserialized to replay a function. Functions need
+/// to be registered with the registry for this to work.
+class Registry {
+public:
+  Registry() = default;
+  virtual ~Registry() = default;
+
+  /// Register a default replayer for a function.
+  template <typename Signature> void Register(Signature *f) {
+    DoRegister(uintptr_t(f), llvm::make_unique<DefaultReplayer<Signature>>(f));
+  }
+
+  /// Register a replayer that invokes a custom function with the same
+  /// signature as the replayed function.
+  template <typename Signature> void Register(Signature *f, Signature *g) {
+    DoRegister(uintptr_t(f), llvm::make_unique<DefaultReplayer<Signature>>(g));
+  }
+
+  /// Replay functions from a file.
+  bool Replay(const FileSpec &file);
+
+  /// Replay functions from a buffer.
+  bool Replay(llvm::StringRef buffer);
+
+  /// Returns the ID for a given function address.
+  unsigned GetID(uintptr_t addr);
+
+protected:
+  /// Register the given replayer for a function (and the ID mapping).
+  void DoRegister(uintptr_t RunID, std::unique_ptr<Replayer> replayer);
+
+private:
+  /// Mapping of function addresses to replayers and their ID.
+  std::map<uintptr_t, std::pair<std::unique_ptr<Replayer>, unsigned>>
+      m_replayers;
+
+  /// Mapping of IDs to replayer instances.
+  std::map<unsigned, Replayer *> m_ids;
+};
+
+/// To be used as the "Runtime ID" of a constructor. It also invokes the
+/// constructor when called.
+template <typename Signature> struct construct;
+template <typename Class, typename... Args> struct construct<Class(Args...)> {
+  static Class *doit(Args... args) { return new Class(args...); }
+};
+
+/// To be used as the "Runtime ID" of a member function. It also invokes the
+/// member function when called.
+template <typename Signature> struct invoke;
+template <typename Result, typename Class, typename... Args>
+struct invoke<Result (Class::*)(Args...)> {
+  template <Result (Class::*m)(Args...)> struct method {
+    static Result doit(Class *c, Args... args) { return (c->*m)(args...); }
+  };
+};
+
+template <typename Result, typename Class, typename... Args>
+struct invoke<Result (Class::*)(Args...) const> {
+  template <Result (Class::*m)(Args...) const> struct method_const {
+    static Result doit(Class *c, Args... args) { return (c->*m)(args...); }
+  };
+};
+
+template <typename Class, typename... Args>
+struct invoke<void (Class::*)(Args...)> {
+  template <void (Class::*m)(Args...)> struct method {
+    static void doit(Class *c, Args... args) { (c->*m)(args...); }
+  };
+};
+
 /// Maps an object to an index for serialization. Indices are unique and
 /// incremented for every new object.
 ///
 /// Indices start at 1 in order to differentiate with an invalid index (0) in
 /// the serialized buffer.
 class ObjectToIndex {
 public:
   template <typename T> unsigned GetIndexForObject(T *t) {
     return GetIndexForObjectImpl((void *)t);
   }
 
 private:
   unsigned GetIndexForObjectImpl(void *object);
 
-  std::mutex m_mutex;
   llvm::DenseMap<void *, unsigned> m_mapping;
 };
 
 /// Serializes functions, their arguments and their return type to a stream.
 class Serializer {
 public:
   Serializer(llvm::raw_ostream &stream = llvm::outs()) : m_stream(stream) {}
 
private:
 
   /// Serialization stream.
   llvm::raw_ostream &m_stream;
 
   /// Mapping of objects to indices.
   ObjectToIndex m_tracker;
 };
 
+class InstrumentationData {
+public:
+  InstrumentationData() : m_serializer(nullptr), m_registry(nullptr){};
+  InstrumentationData(Serializer &serializer, Registry &registry)
+      : m_serializer(&serializer), m_registry(&registry){};
+
+  Serializer &GetSerializer() { return *m_serializer; }
+  Registry &GetRegistry() { return *m_registry; }
+
+  operator bool() { return m_serializer != nullptr && m_registry != nullptr; }
+
+private:
+  Serializer *m_serializer;
+  Registry *m_registry;
+};
+
+/// RAII object that tracks the function invocations and their return value.
+///
+/// API calls are only captured when the API boundary is crossed. Once we're in
+/// the API layer, and another API function is called, it doesn't need to be
+/// recorded.
+///
+/// When a call is recored, its result is always recorded as well, even if the
+/// function returns a void. For functions that return by value, RecordResult
+/// should be used. Otherwise a sentinel value (0) will be serialized.
+class Recorder {
+public:
+  Recorder(Serializer &serializer, Registry &registry,
+           llvm::StringRef pretty_func = {});
+  ~Recorder();
+
+  /// Records a single function call.
+  template <typename Result, typename... FArgs, typename... RArgs>
+  void Record(Result (*f)(FArgs...), const RArgs &... args) {
+    if (!ShouldCapture())
+      return;
+
+    unsigned id = m_registry.GetID(uintptr_t(f));
+
+    LLDB_LOG(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API), "#{0} '{1}'", id,
+             m_pretty_func);
+
+    m_serializer.SerializeAll(id);
+    m_serializer.SerializeAll(args...);
+
+    if (std::is_class<typename std::remove_pointer<
+            typename std::remove_reference<Result>::type>::type>::value) {
+      m_result_recorded = false;
+    } else {
+      m_serializer.SerializeAll(0);
+      m_result_recorded = true;
+    }
+  }
+
+  /// Records a single function call.
+  template <typename... Args>
+  void Record(void (*f)(Args...), const Args &... args) {
+    if (!ShouldCapture())
+      return;
+
+    unsigned id = m_registry.GetID(uintptr_t(f));
+
+    LLDB_LOG(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API), "#{0} '{1}'", id,
+             m_pretty_func);
+
+    m_serializer.SerializeAll(id);
+    m_serializer.SerializeAll(args...);
+
+    // Record result.
+    m_serializer.SerializeAll(0);
+    m_result_recorded = true;
+  }
+
+  /// Record the result of a function call.
+  template <typename Result> Result RecordResult(const Result &r) {
+    UpdateBoundary();
+    if (ShouldCapture()) {
+      assert(!m_result_recorded);
+      m_serializer.SerializeAll(r);
+      m_result_recorded = true;
+    }
+    return r;
+  }
+
+private:
+  void UpdateBoundary() {
+    if (m_local_boundary)
+      g_global_boundary = false;
+  }
+
+  bool ShouldCapture() { return m_local_boundary; }
+
+  Serializer &m_serializer;
+  Registry &m_registry;
+
+  /// Pretty function for logging.
+  llvm::StringRef m_pretty_func;
+
+  /// Whether this function call was the one crossing the API boundary.
+  bool m_local_boundary;
+
+  /// Whether the return value was recorded explicitly.
+  bool m_result_recorded;
+
+  /// Whether we're currently across the API boundary.
+  static bool g_global_boundary;
+};
+
 } // namespace repro
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_REPRODUCER_INSTRUMENTATION_H

lldb/trunk/source/API/CMakeLists.txt

add_lldb_library(liblldb SHARED
   SBMemoryRegionInfoList.cpp
   SBModule.cpp
   SBModuleSpec.cpp
   SBPlatform.cpp
   SBProcess.cpp
   SBProcessInfo.cpp
   SBQueue.cpp
   SBQueueItem.cpp
+  SBReproducer.cpp
   SBSection.cpp
   SBSourceManager.cpp
   SBStream.cpp
   SBStringList.cpp
   SBStructuredData.cpp
   SBSymbol.cpp
   SBSymbolContext.cpp
   SBSymbolContextList.cpp

lldb/trunk/source/API/SBReproducer.cpp

+//===-- SBReproducer.cpp ----------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "SBReproducerPrivate.h"
+
+#include "lldb/API/LLDB.h"
+#include "lldb/API/SBAddress.h"
+#include "lldb/API/SBAttachInfo.h"
+#include "lldb/API/SBBlock.h"
+#include "lldb/API/SBBreakpoint.h"
+#include "lldb/API/SBCommandInterpreter.h"
+#include "lldb/API/SBData.h"
+#include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBDeclaration.h"
+#include "lldb/API/SBError.h"
+#include "lldb/API/SBFileSpec.h"
+#include "lldb/API/SBHostOS.h"
+#include "lldb/API/SBReproducer.h"
+
+#include "lldb/Host/FileSystem.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::repro;
+
+SBRegistry::SBRegistry() {}
+
+bool SBReproducer::Replay() {
+  repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
+  if (!loader)
+    return false;
+
+  FileSpec file = loader->GetFile<SBInfo>();
+  if (!file)
+    return false;
+
+  SBRegistry registry;
+  registry.Replay(file);
+
+  return true;
+}
+
+char lldb_private::repro::SBProvider::ID = 0;
+const char *SBInfo::name = "sbapi";
+const char *SBInfo::file = "sbapi.bin";

lldb/trunk/source/API/SBReproducerPrivate.h

+//===-- SBReproducerPrivate.h -----------------------------------*- C++ -*-===//
+//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_API_SBREPRODUCER_PRIVATE_H
+#define LLDB_API_SBREPRODUCER_PRIVATE_H
+
+#include "lldb/API/SBReproducer.h"
+
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/Reproducer.h"
+#include "lldb/Utility/ReproducerInstrumentation.h"
+
+#include "llvm/ADT/DenseMap.h"
+
+#define LLDB_GET_INSTRUMENTATION_DATA()                                        \
+  lldb_private::repro::GetInstrumentationData()
+
+namespace lldb_private {
+namespace repro {
+
+class SBRegistry : public Registry {
+public:
+  SBRegistry();
+};
+
+struct SBInfo {
+  static const char *name;
+  static const char *file;
+};
+
+class SBProvider : public Provider<SBProvider> {
+public:
+  typedef SBInfo info;
+
+  SBProvider(const FileSpec &directory)
+      : Provider(directory),
+        m_stream(directory.CopyByAppendingPathComponent("sbapi.bin").GetPath(),
+                 m_ec, llvm::sys::fs::OpenFlags::F_None),
+        m_serializer(m_stream) {}
+
+  Serializer &GetSerializer() { return m_serializer; }
+  Registry &GetRegistry() { return m_registry; }
+
+  static char ID;
+
+private:
+  std::error_code m_ec;
+  llvm::raw_fd_ostream m_stream;
+  Serializer m_serializer;
+  SBRegistry m_registry;
+};
+
+inline InstrumentationData GetInstrumentationData() {
+  if (auto *g = lldb_private::repro::Reproducer::Instance().GetGenerator()) {
+    auto &p = g->GetOrCreate<SBProvider>();
+    return {p.GetSerializer(), p.GetRegistry()};
+  }
+  return {};
+}
+
+} // namespace repro
+} // namespace lldb_private
+
+#endif

lldb/trunk/source/Utility/ReproducerInstrumentation.cpp

template <> const char *Deserializer::Deserialize<const char *>() {
   auto pos = m_buffer.find('\0');
   if (pos == llvm::StringRef::npos)
     return nullptr;
   const char *str = m_buffer.data();
   m_buffer = m_buffer.drop_front(pos + 1);
   return str;
 }
 
+bool Registry::Replay(const FileSpec &file) {
+  auto error_or_file = llvm::MemoryBuffer::getFile(file.GetPath());
+  if (auto err = error_or_file.getError())
+    return false;
+
+  return Replay((*error_or_file)->getBuffer());
+}
+
+bool Registry::Replay(llvm::StringRef buffer) {
+  Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_API);
+
+  Deserializer deserializer(buffer);
+  while (deserializer.HasData(1)) {
+    unsigned id = deserializer.Deserialize<unsigned>();
+    LLDB_LOG(log, "Replaying function #{0}", id);
+    m_ids[id]->operator()(deserializer);
+  }
+
+  return true;
+}
+
+void Registry::DoRegister(uintptr_t RunID, std::unique_ptr<Replayer> replayer) {
+  const unsigned id = m_replayers.size() + 1;
+  assert(m_replayers.find(RunID) == m_replayers.end());
+  m_replayers[RunID] = std::make_pair(std::move(replayer), id);
+  m_ids[id] = m_replayers[RunID].first.get();
+}
+
+unsigned Registry::GetID(uintptr_t addr) {
+  unsigned id = m_replayers[addr].second;
+  assert(id != 0 && "Forgot to add function to registry?");
+  return id;
+}
+
 unsigned ObjectToIndex::GetIndexForObjectImpl(void *object) {
-  std::lock_guard<std::mutex> guard(m_mutex);
   unsigned index = m_mapping.size() + 1;
   auto it = m_mapping.find(object);
   if (it == m_mapping.end())
     m_mapping[object] = index;
   return m_mapping[object];
 }
+
+Recorder::Recorder(Serializer &serializer, Registry &registry,
+                   llvm::StringRef pretty_func)
+    : m_serializer(serializer), m_registry(registry),
+      m_pretty_func(pretty_func), m_local_boundary(false),
+      m_result_recorded(true) {
+  if (!g_global_boundary) {
+    g_global_boundary = true;
+    m_local_boundary = true;
+  }
+}
+
+Recorder::~Recorder() {
+  assert(m_result_recorded && "Did you forget LLDB_RECORD_RESULT?");
+  UpdateBoundary();
+}
+
+bool lldb_private::repro::Recorder::g_global_boundary;

lldb/trunk/tools/driver/Driver.cpp

 //===-- Driver.cpp ----------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Driver.h"
 
 #include "lldb/API/SBCommandInterpreter.h"
 #include "lldb/API/SBCommandReturnObject.h"
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBHostOS.h"
 #include "lldb/API/SBLanguageRuntime.h"
+#include "lldb/API/SBReproducer.h"
 #include "lldb/API/SBStream.h"
 #include "lldb/API/SBStringList.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
if (input_args.hasArg(OPT_help)) {
     return 0;
   }
 
   for (auto *arg : input_args.filtered(OPT_UNKNOWN)) {
     WithColor::warning() << "ignoring unknown option: " << arg->getSpelling()
                          << '\n';
   }
 
-  SBInitializerOptions options;
+  // Remember if we're in replay mode for later.
+  bool replay = false;
 
+  SBInitializerOptions options;
   if (auto *arg = input_args.getLastArg(OPT_capture)) {
     auto arg_value = arg->getValue();
     options.SetReproducerPath(arg_value);
     options.SetCaptureReproducer(true);
   }
 
   if (auto *arg = input_args.getLastArg(OPT_replay)) {
     auto arg_value = arg->getValue();
     options.SetReplayReproducer(true);
     options.SetReproducerPath(arg_value);
+    replay = true;
   }
 
   SBError error = SBDebugger::Initialize(options);
   if (error.Fail()) {
     WithColor::error() << "initialization failed: " << error.GetCString()
                        << '\n';
     return 1;
   }
 
+  if (replay) {
+    SBReproducer reproducer;
+    if (!reproducer.Replay()) {
+      WithColor::error() << "something went wrong running the reporducer.\n";
+    }
+    return 0;
+  }
+
   SBHostOS::ThreadCreated("<lldb.driver.main-thread>");
 
   signal(SIGINT, sigint_handler);
 #if !defined(_MSC_VER)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
   signal(SIGTSTP, sigtstp_handler);
   signal(SIGCONT, sigcont_handler);

lldb/trunk/unittests/Utility/ReproducerInstrumentationTest.cpp

 //===-- ReproducerInstrumentationTest.cpp -----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+#include <cmath>
+#include <limits>
+
 #include "lldb/Utility/ReproducerInstrumentation.h"
 
 using namespace lldb_private;
 using namespace lldb_private::repro;
 
-namespace {
 struct Foo {
   int m = 1;
 };
 struct Bar {
   double m = 2;
 };
 
 bool operator==(const Foo &LHS, const Foo &RHS) { return LHS.m == RHS.m; }
struct Pod {
   long g = 4;
   short h = 5;
   unsigned char i = 'b';
   unsigned int j = 6;
   unsigned long long k = 7;
   unsigned long l = 8;
   unsigned short m = 9;
 };
-} // namespace
+
+class TestingRegistry : public Registry {
+public:
+  TestingRegistry();
+};
+
+static llvm::Optional<Serializer> g_serializer;
+static llvm::Optional<TestingRegistry> g_registry;
+
+#define LLDB_GET_INSTRUMENTATION_DATA()                                        \
+  InstrumentationData(*g_serializer, *g_registry)
+
+class InstrumentedFoo {
+public:
+  InstrumentedFoo() = default;
+  /// Instrumented methods.
+  /// {
+  InstrumentedFoo(int i);
+  InstrumentedFoo(const InstrumentedFoo &foo);
+  InstrumentedFoo &operator=(const InstrumentedFoo &foo);
+  void A(int a);
+  void B(int &b) const;
+  int C(float *c);
+  int D(const char *d) const;
+  static void E(double e);
+  static int F();
+  void Validate();
+  //// }
+
+private:
+  int m_a = 0;
+  mutable int m_b = 0;
+  float m_c = 0;
+  mutable std::string m_d = {};
+  static double g_e;
+  static bool g_f;
+  mutable int m_called = 0;
+};
+
+class InstrumentedBar {
+public:
+  /// Instrumented methods.
+  /// {
+  InstrumentedBar();
+  InstrumentedFoo GetInstrumentedFoo();
+  void SetInstrumentedFoo(InstrumentedFoo *foo);
+  void SetInstrumentedFoo(InstrumentedFoo &foo);
+  void Validate();
+  /// }
+
+private:
+  bool m_get_instrumend_foo_called = false;
+  InstrumentedFoo *m_foo_set_by_ptr = nullptr;
+  InstrumentedFoo *m_foo_set_by_ref = nullptr;
+};
+
+double InstrumentedFoo::g_e = 0;
+bool InstrumentedFoo::g_f = false;
+
+static std::vector<InstrumentedFoo *> g_foos;
+static std::vector<InstrumentedBar *> g_bars;
+
+void ClearObjects() {
+  g_foos.clear();
+  g_bars.clear();
+}
+
+void ValidateObjects(size_t expected_foos, size_t expected_bars) {
+  EXPECT_EQ(expected_foos, g_foos.size());
+  EXPECT_EQ(expected_bars, g_bars.size());
+
+  for (auto *foo : g_foos) {
+    foo->Validate();
+  }
+
+  for (auto *bar : g_bars) {
+    bar->Validate();
+  }
+}
+
+InstrumentedFoo::InstrumentedFoo(int i) {
+  LLDB_RECORD_CONSTRUCTOR(InstrumentedFoo, (int), i);
+  g_foos.push_back(this);
+}
+
+InstrumentedFoo::InstrumentedFoo(const InstrumentedFoo &foo) {
+  LLDB_RECORD_CONSTRUCTOR(InstrumentedFoo, (const InstrumentedFoo &), foo);
+  g_foos.erase(std::remove(g_foos.begin(), g_foos.end(), &foo));
+  g_foos.push_back(this);
+}
+
+InstrumentedFoo &InstrumentedFoo::operator=(const InstrumentedFoo &foo) {
+  LLDB_RECORD_METHOD(InstrumentedFoo &,
+                     InstrumentedFoo, operator=,(const InstrumentedFoo &), foo);
+  g_foos.erase(std::remove(g_foos.begin(), g_foos.end(), &foo));
+  g_foos.push_back(this);
+  return *this;
+}
+
+void InstrumentedFoo::A(int a) {
+  LLDB_RECORD_METHOD(void, InstrumentedFoo, A, (int), a);
+  B(a);
+  m_a = a;
+}
+
+void InstrumentedFoo::B(int &b) const {
+  LLDB_RECORD_METHOD_CONST(void, InstrumentedFoo, B, (int &), b);
+  m_called++;
+  m_b = b;
+}
+
+int InstrumentedFoo::C(float *c) {
+  LLDB_RECORD_METHOD(int, InstrumentedFoo, C, (float *), c);
+  m_c = *c;
+  return 1;
+}
+
+int InstrumentedFoo::D(const char *d) const {
+  LLDB_RECORD_METHOD_CONST(int, InstrumentedFoo, D, (const char *), d);
+  m_d = std::string(d);
+  return 2;
+}
+
+void InstrumentedFoo::E(double e) {
+  LLDB_RECORD_STATIC_METHOD(void, InstrumentedFoo, E, (double), e);
+  g_e = e;
+}
+
+int InstrumentedFoo::F() {
+  LLDB_RECORD_STATIC_METHOD_NO_ARGS(int, InstrumentedFoo, F);
+  g_f = true;
+  return 3;
+}
+
+void InstrumentedFoo::Validate() {
+  LLDB_RECORD_METHOD_NO_ARGS(void, InstrumentedFoo, Validate);
+  EXPECT_EQ(m_a, 100);
+  EXPECT_EQ(m_b, 200);
+  EXPECT_NEAR(m_c, 300.3, 0.01);
+  EXPECT_EQ(m_d, "bar");
+  EXPECT_NEAR(g_e, 400.4, 0.01);
+  EXPECT_EQ(g_f, true);
+  EXPECT_EQ(2, m_called);
+}
+
+InstrumentedBar::InstrumentedBar() {
+  LLDB_RECORD_CONSTRUCTOR_NO_ARGS(InstrumentedBar);
+  g_bars.push_back(this);
+}
+
+InstrumentedFoo InstrumentedBar::GetInstrumentedFoo() {
+  LLDB_RECORD_METHOD_NO_ARGS(InstrumentedFoo, InstrumentedBar,
+                             GetInstrumentedFoo);
+  m_get_instrumend_foo_called = true;
+  return LLDB_RECORD_RESULT(InstrumentedFoo(0));
+}
+
+void InstrumentedBar::SetInstrumentedFoo(InstrumentedFoo *foo) {
+  LLDB_RECORD_METHOD(void, InstrumentedBar, SetInstrumentedFoo,
+                     (InstrumentedFoo *), foo);
+  m_foo_set_by_ptr = foo;
+}
+
+void InstrumentedBar::SetInstrumentedFoo(InstrumentedFoo &foo) {
+  LLDB_RECORD_METHOD(void, InstrumentedBar, SetInstrumentedFoo,
+                     (InstrumentedFoo &), foo);
+  m_foo_set_by_ref = &foo;
+}
+
+void InstrumentedBar::Validate() {
+  LLDB_RECORD_METHOD_NO_ARGS(void, InstrumentedBar, Validate);
+
+  EXPECT_TRUE(m_get_instrumend_foo_called);
+  EXPECT_NE(m_foo_set_by_ptr, nullptr);
+  EXPECT_EQ(m_foo_set_by_ptr, m_foo_set_by_ref);
+}
+
+TestingRegistry::TestingRegistry() {
+  LLDB_REGISTER_CONSTRUCTOR(InstrumentedFoo, (int i));
+  LLDB_REGISTER_CONSTRUCTOR(InstrumentedFoo, (const InstrumentedFoo &));
+  LLDB_REGISTER_METHOD(InstrumentedFoo &,
+                       InstrumentedFoo, operator=,(const InstrumentedFoo &));
+  LLDB_REGISTER_METHOD(void, InstrumentedFoo, A, (int));
+  LLDB_REGISTER_METHOD_CONST(void, InstrumentedFoo, B, (int &));
+  LLDB_REGISTER_METHOD(int, InstrumentedFoo, C, (float *));
+  LLDB_REGISTER_METHOD_CONST(int, InstrumentedFoo, D, (const char *));
+  LLDB_REGISTER_STATIC_METHOD(void, InstrumentedFoo, E, (double));
+  LLDB_REGISTER_STATIC_METHOD(int, InstrumentedFoo, F, ());
+  LLDB_REGISTER_METHOD(void, InstrumentedFoo, Validate, ());
+
+  LLDB_REGISTER_CONSTRUCTOR(InstrumentedBar, ());
+  LLDB_REGISTER_METHOD(InstrumentedFoo, InstrumentedBar, GetInstrumentedFoo,
+                       ());
+  LLDB_REGISTER_METHOD(void, InstrumentedBar, SetInstrumentedFoo,
+                       (InstrumentedFoo *));
+  LLDB_REGISTER_METHOD(void, InstrumentedBar, SetInstrumentedFoo,
+                       (InstrumentedFoo &));
+  LLDB_REGISTER_METHOD(void, InstrumentedBar, Validate, ());
+}
 
 static const Pod p;
 
 TEST(IndexToObjectTest, ObjectForIndex) {
   IndexToObject index_to_object;
   Foo foo;
   Bar bar;
 
TEST(SerializationRountripTest, SerializeDeserializeObjectReference) {
   Deserializer deserializer(buffer);
 
   deserializer.HandleReplayResult(&foo);
   deserializer.HandleReplayResult(&bar);
 
   EXPECT_EQ(foo, deserializer.Deserialize<Foo &>());
   EXPECT_EQ(bar, deserializer.Deserialize<Bar &>());
 }
+
+TEST(RecordReplayTest, InstrumentedFoo) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+  g_registry.emplace();
+  g_serializer.emplace(os);
+
+  {
+    int b = 200;
+    float c = 300.3;
+    double e = 400.4;
+
+    InstrumentedFoo foo(0);
+    foo.A(100);
+    foo.B(b);
+    foo.C(&c);
+    foo.D("bar");
+    InstrumentedFoo::E(e);
+    InstrumentedFoo::F();
+    foo.Validate();
+  }
+
+  ClearObjects();
+
+  TestingRegistry registry;
+  registry.Replay(os.str());
+
+  ValidateObjects(1, 0);
+}
+
+TEST(RecordReplayTest, InstrumentedFooSameThis) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+  g_registry.emplace();
+  g_serializer.emplace(os);
+
+  int b = 200;
+  float c = 300.3;
+  double e = 400.4;
+
+  InstrumentedFoo *foo = new InstrumentedFoo(0);
+  foo->A(100);
+  foo->B(b);
+  foo->C(&c);
+  foo->D("bar");
+  InstrumentedFoo::E(e);
+  InstrumentedFoo::F();
+  foo->Validate();
+  foo->~InstrumentedFoo();
+
+  InstrumentedFoo *foo2 = new (foo) InstrumentedFoo(0);
+  foo2->A(100);
+  foo2->B(b);
+  foo2->C(&c);
+  foo2->D("bar");
+  InstrumentedFoo::E(e);
+  InstrumentedFoo::F();
+  foo2->Validate();
+  delete foo2;
+
+  ClearObjects();
+
+  TestingRegistry registry;
+  registry.Replay(os.str());
+
+  ValidateObjects(2, 0);
+}
+
+TEST(RecordReplayTest, InstrumentedBar) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+  g_registry.emplace();
+  g_serializer.emplace(os);
+
+  {
+    InstrumentedBar bar;
+    InstrumentedFoo foo = bar.GetInstrumentedFoo();
+
+    int b = 200;
+    float c = 300.3;
+    double e = 400.4;
+
+    foo.A(100);
+    foo.B(b);
+    foo.C(&c);
+    foo.D("bar");
+    InstrumentedFoo::E(e);
+    InstrumentedFoo::F();
+    foo.Validate();
+
+    bar.SetInstrumentedFoo(foo);
+    bar.SetInstrumentedFoo(&foo);
+    bar.Validate();
+  }
+
+  ClearObjects();
+
+  TestingRegistry registry;
+  registry.Replay(os.str());
+
+  ValidateObjects(1, 1);
+}