Page MenuHomePhabricator

[Reproducers] Improve reproducer API and add unit tests.
ClosedPublic

Authored by JDevlieghere on Nov 15 2018, 10:05 PM.

Details

Summary

When I landed the initial reproducer framework I knew there were some things that needed improvement. Rather than bundling it with a patch that adds more functionality I split it off into this patch. I also think the API is stable enough to add unit testing, which is included in this patch as well.

Other improvements include:

  • Refactor how we initialize the loader and generator.
  • Improve naming consistency: capture and replay seems the least ambiguous.
  • Index providers by name and make sure there's only one of each.
  • Add convenience methods for creating and accessing providers.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Nov 15 2018, 10:05 PM

Looks pretty good to me, but added my 2¢ on minor things.

include/lldb/Utility/Reproducer.h
169 ↗(On Diff #174330)

Are the default-value-cases necessary?

source/API/SBDebugger.cpp
1064 ↗(On Diff #174330)

Will the error be passed up the stack soon? Otherwise maybe add FIXME comment?

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
303 ↗(On Diff #174330)

In which case would GetOrCreate() fail?

3440 ↗(On Diff #174330)

Brackets

unittests/Utility/ReproducerTest.cpp
37 ↗(On Diff #174330)

It may be good to know what went wrong if this fails. And: are the other EXPECTs still relevant in the error-case?

If SetCapture() can fail in this particular situation, you may want to do something like:

if (error)
  FAIL() << llvm::toString(std::move(error));

Otherwise, if this was only to prepare the test and you wanted to indicate that it's not supposed to fail, then you could do:

llvm::cantFail(reproducer.SetCapture(true, FileSpec("/bogus/path"));
JDevlieghere marked 4 inline comments as done.

Address Stefan's review feedback.

JDevlieghere marked an inline comment as done.

Missed a comment; fixed now.

Simplify code a little, as per Stefan's (offline) suggestion.

labath added a subscriber: labath.Nov 19 2018, 6:01 AM
labath added inline comments.
include/lldb/Utility/Reproducer.h
59–64 ↗(On Diff #174416)

Could this be replaced by additional arguments to the constructor (avoiding two-stage initialization)?

99 ↗(On Diff #174416)

Is the value of the NAME used anywhere? If not, you could just have each class define a char variable (like llvm::ErrorInfo subclasses do) and then use its address as a key, making everything faster.

(Even if name is useful (for printing to the user or whatever), it might be worth to switch to using the char-address for lookup and then just have getName method for other things.)

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
304 ↗(On Diff #174416)

could GetOrCreate keep returning a reference (to avoid spurious guaranteed to be not null comments)?

source/Utility/Reproducer.cpp
131–133 ↗(On Diff #174416)

Given these limitations, would it be possible to set the root once in the constructor and keep it immutable since then?

unittests/Utility/ReproducerTest.cpp
44–46 ↗(On Diff #174416)

Shorter and with better error messages:
EXPECT_THAT_ERROR(reproducer.SetReplay(FileSpec("/bogus/path")), llvm::Succeeded());

79–81 ↗(On Diff #174416)

EXPECT_THAT_ERROR(..., llvm::Failed());

JDevlieghere marked 6 inline comments as done.

Thanks Pavel, really useful feedback, I learned a few new things :-)

Does anybody want to have another look or can I land this?

I haven't been following the reproducer work in detail, but this seems reasonable to me. Thanks for incorporating my drive-by suggestions.

include/lldb/Utility/Reproducer.h
104 ↗(On Diff #175293)

You should still be able to use make_unique here.

134 ↗(On Diff #175293)

Out of date comment.

davide accepted this revision.Nov 27 2018, 12:31 PM

LGTM

This revision is now accepted and ready to land.Nov 27 2018, 12:31 PM
This revision was automatically updated to reflect the committed changes.