Page MenuHomePhabricator

[Reproducers] Initialize reproducers before initializing the debugger.
ClosedPublic

Authored by JDevlieghere on Feb 19 2019, 3:03 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Feb 19 2019, 3:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 3:03 PM
Herald added subscribers: abidh, mgorny. · View Herald Transcript
JDevlieghere marked an inline comment as done.Feb 19 2019, 3:04 PM
JDevlieghere added inline comments.
lldb/include/lldb/API/SBDebugger.h
48 ↗(On Diff #187450)

This should be safe, I don't know anyone relying on this and apparently the python .i file contained an incorrect return type (void).

JDevlieghere marked an inline comment as done.Feb 19 2019, 10:56 PM
JDevlieghere added inline comments.
lldb/source/API/SBReproducer.cpp
49 ↗(On Diff #187450)

The SBError is a problem. We can create it after the initialization, but then we’d need a bogus repro::Record to make sure the api boundary is correctly registered. Let me know if you can think of an alternative.

I think this looks mostly fine. See my comment about not using SB classes in the reproducer api. I still kind of like the idea of naming the reproducer class in some special way, to make it more obvious that it is not "just another" SB class, but I'm not sure if that would be just more confusing.

lldb/include/lldb/API/SBReproducer.h
18–20 ↗(On Diff #187450)

So, in this new world, how are we expected to perform the replay? InitializeReplay(), followed by Replay() ? Could we fold those two calls into one? I don't see what meaningful work could the user do between those two calls, as all sb calls (including SBDebugger::Initialize) will now be recorded (right?).

lldb/source/API/SBDebugger.cpp
127 ↗(On Diff #187450)

I think the usual way to do that is to just cast the result to void.

lldb/source/API/SBReproducer.cpp
49 ↗(On Diff #187450)

Maybe this function should not return an SBError at all (just like it does not accept an SBFileSpec due to bootstrapping problems). You could just return a std::string, and have an empty string denote success. This would be consistent with the idea that the repro API is not a part of SB API, but rather something sitting slightly above (or at least, besides) it.

  • Return a string instead of an SBError
  • Make InitializeReplay perform the actual replay.

I think this looks mostly fine. See my comment about not using SB classes in the reproducer api. I still kind of like the idea of naming the reproducer class in some special way, to make it more obvious that it is not "just another" SB class, but I'm not sure if that would be just more confusing.

I'm afraid it's going to be confusing. I'll add a comment to the SBReproducer class to point out that we cannot rely on any other SB* objects because of bootstrapping.

  • Add comment to SBReproducer.h explaining we cannot use any SB objects in the interface or implementation.
labath accepted this revision.Feb 20 2019, 11:49 PM

LGTM. I'd maybe rename InitializeReplay into Replay (i.e., do the merging the other way around), since the Initialize part makes me think the replay will not actually commence there.

I forgot that we don't return STL objects through the API. It's a shame we have to resort to the static std::string hack. Another option would be to ConstStringify the string, but as these are functions that are to be called just once, this should do too.

This revision is now accepted and ready to land.Feb 20 2019, 11:49 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 2:25 PM