This is an archive of the discontinued LLVM Phabricator instance.

Change Target::FindBreakpointsByName to return Expected<vector>
ClosedPublic

Authored by JosephTremoulet on Dec 2 2019, 8:05 AM.

Details

Summary

Using a BreakpointList corrupts the breakpoints' IDs because
BreakpointList::Add sets the ID, so use a vector instead, and
update the signature to return the vector wrapped in an
llvm::Expected which can propagate any error from the inner
call to StringIsBreakpointName.

Note that, despite the similar name, SBTarget::FindBreakpointsByName
doesn't suffer the same problem, because it uses a SBBreakpointList,
which is more like a BreakpointIDList than a BreakpointList under the
covers.

Add a check to TestBreakpointNames that, without this fix, notices the
ID getting mutated and fails.

Event Timeline

JosephTremoulet created this revision.Dec 2 2019, 8:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2019, 8:05 AM
JDevlieghere added inline comments.Dec 2 2019, 10:05 AM
lldb/include/lldb/Breakpoint/BreakpointList.h
70–73

I think the API would look nicer if we returned an llvm::Optional<std::vector>> where None means an invalid breakpoint name and an empty list no matches. What do you think?

JosephTremoulet marked an inline comment as done.Dec 2 2019, 10:26 AM
JosephTremoulet added inline comments.
lldb/include/lldb/Breakpoint/BreakpointList.h
70–73

I think I'd go with Expected<> over Optional<>, since the false return indicates invalid input.

I actually originally considered different signatures for this change. My first inclination was to switch the populated list from a BreakpointList to a BreakpointIDList, but it seemed that would be inefficient for the call from Target::ApplyNameToBreakpoints that needs the actual breakpoints. So then I went down the route of Expected<iterator_range<breakpoint iterator>>, but it was quickly becoming more code (and more convoluted) than seemed warranted. So I looked around, saw std::vectors being populated by e.g. Breakpoint::GetNames and SourceManager::FindLinesMatchingRegex, and decided to follow suit.

Which is a long way to say: populating a std::vector seems to "fit in" with surrounding code better, but aside from that, yes I think returning Expected<std::vector> would be a more natural fit. I don't know which of those concerns to prefer in this code; LMK and I'm happy to switch it if that seems best.

JDevlieghere added inline comments.Dec 2 2019, 11:13 AM
lldb/include/lldb/Breakpoint/BreakpointList.h
70–73

I think an Expected is perfect. I proposed Optional because the way things are, the call sites would have to discard the error anyway. However, given that FindBreakpointsByName takes a Status, I think it would be good to propagate that up.

In other places, I've used "XList" to mean the container that manages the things contained, and "XCollection" to be a random possibly unrelated collection of items. It doesn't make any sense to have a collection of breakpoints from more than one target, so the collection class could ensure that, whereas a std::vector is just a random assortment. But I'm not sure that adds enough value to warrant adding a collection container here. std::vector is fine.

  • Change signature to return Expected<vector<BreakpointSP>>
JosephTremoulet retitled this revision from Change Target::FindBreakpointsByName to use a vector to Change Target::FindBreakpointsByName to return Expected<vector>.Dec 3 2019, 8:05 AM
JosephTremoulet edited the summary of this revision. (Show Details)
JosephTremoulet marked 2 inline comments as done.
JDevlieghere accepted this revision.Dec 3 2019, 8:53 AM

LGTM

lldb/source/Breakpoint/BreakpointList.cpp
138

I think llvm::errc::invalid_argument would fit.

This revision is now accepted and ready to land.Dec 3 2019, 8:53 AM
  • Use invalid_argument error code
JosephTremoulet marked 2 inline comments as done.Dec 3 2019, 9:34 AM
JosephTremoulet added inline comments.
lldb/source/Breakpoint/BreakpointList.cpp
138

SGTM; updated.

This revision was automatically updated to reflect the committed changes.
JosephTremoulet marked an inline comment as done.