This is an archive of the discontinued LLVM Phabricator instance.

[Reproducers] SBReproducer framework
ClosedPublic

Authored by JDevlieghere on Jan 4 2019, 10:00 AM.

Details

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

Diff Detail

Event Timeline

JDevlieghere created this revision.Jan 4 2019, 10:00 AM

Prototype 2.0

Rebase on recent reproducer changes.

JDevlieghere retitled this revision from SBReproducer prototype to [Reproducers] SBReproducer framework.
JDevlieghere edited the summary of this revision. (Show Details)
JDevlieghere set the repository for this revision to rLLDB LLDB.
JDevlieghere added a project: Restricted Project.

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.

Add unit testing for supporting classes

labath added a comment.Feb 1 2019, 4:27 AM

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
2

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

70

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

source/API/SBReproducerPrivate.h
45

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.

66–70

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

83–94

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

122

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

156–162

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.

168

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.

170

reinterpret_cast

183

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

304

Shouldn't this call g then?

308–310

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

371

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.

372

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

420–449

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

454

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.

483

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.

485

It looks like GetIndexForObject returns unsigned

563–564

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.

574–575

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.

579

Inconsistent braces around a single statement.

585–586

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

619–627

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?

660

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

tools/driver/Driver.cpp
916–920

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?

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.

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 marked 27 inline comments as done.

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
83–94

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

122

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

183

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

454

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
916–920

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

  • Move framework in Utility
  • Update tests
labath added a comment.Feb 4 2019, 4:38 AM

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
20

Should this be static?

include/lldb/Utility/ReproducerInstrumentation.h
290–291

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

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

s/m_sbreplayers/m_replayers`

393

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

432

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
43–60

Unused functions.

source/API/SBReproducerPrivate.h
122

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.

183

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?

454

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?

526–534

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.

source/Utility/ReproducerInstrumentation.cpp
47

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

tools/driver/Driver.cpp
916–920

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

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 marked 11 inline comments as done.Feb 4 2019, 2:22 PM

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 updated this revision to Diff 185208.

Add context

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

labath added a comment.Feb 5 2019, 5:14 AM

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
50–52

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

57–112

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();
}
133–138

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

402

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)

417–422

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

labath added inline comments.Feb 5 2019, 5:24 AM
unittests/Utility/ReproducerInstrumentationTest.cpp
417–422

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

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

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

source/API/SBReproducerPrivate.h
23

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

inline (or move to .cpp)

unittests/Utility/ReproducerInstrumentationTest.cpp
50–52

I guess this is no longer used?

62

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

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

455

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

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 marked 7 inline comments as done.

Feedback Pavel.

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

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

My pleasure, thank you for the thorough review!

This revision was not accepted when it landed; it landed in state Needs Review.Feb 6 2019, 10:58 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 10:58 AM
labath added a comment.Feb 7 2019, 9:08 AM

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

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.