This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Symbol] Fix column breakpoint `move_to_nearest_code` match
ClosedPublic

Authored by mib on Apr 23 2021, 7:38 PM.

Details

Summary

This patch fixes the column symbol resolution when creating a breakpoint
with the move_to_nearest_code flag set.

In order to achieve this, the patch adds column information handling in
the LineTable's LineEntry finder. After experimenting a little, it
turns out the most natural approach in case of an inaccurate column match,
is to move backward and match the previous LineEntry rather than going
forward like we do with simple line breakpoints.

The patch also reflows the function to reduce code duplication.

Finally, it updates the BreakpointResolver heuristic to align it with
the LineTable method.

rdar://73218201

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib requested review of this revision.Apr 23 2021, 7:38 PM
mib created this revision.
mib created this object with visibility "Custom Policy".
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2021, 7:38 PM
mib updated this revision to Diff 341813.Apr 30 2021, 2:34 AM
mib retitled this revision from [lldb/Symbol] Use column information for symbol resolution to [lldb/Symbol] Fix column breakpoint `move_to_nearest_code` match.
mib edited the summary of this revision. (Show Details)
mib added reviewers: aprantl, jingham.
mib changed the visibility from "Custom Policy" to "Public (No Login Required)".
mib added a subscriber: Restricted Project.
JDevlieghere added inline comments.Apr 30 2021, 3:12 PM
lldb/include/lldb/Symbol/LineTable.h
414

It seems like this method is only using m_entries, so I would turn this into a static (non-member) function in the cpp file and pass m_entries as an argument.

440–442

nit: reflow

447

I know you're just moving the code but maybe a good opportunity to simplify the else-after-continue and else-after-return below.

lldb/source/Breakpoint/BreakpointResolver.cpp
230–234 ↗(On Diff #341813)

Do these comments need to be updated?

mib marked 2 inline comments as done.May 4 2021, 9:24 PM
mib added inline comments.
lldb/include/lldb/Symbol/LineTable.h
414

Besides m_entries, the function also calls LineTable::ConvertEntryAtIndexToLineEntry which is declared protected. Making the function "static" makes it more convoluted, IMO. I'd rather leave it in the header file.

447

As discussed, I'll simplify these in a follow-up patch.

mib updated this revision to Diff 342940.May 4 2021, 9:27 PM
mib edited the summary of this revision. (Show Details)

Address @JDevlieghere comments.

This revision is now accepted and ready to land.May 4 2021, 10:04 PM
This revision was landed with ongoing or failed builds.May 4 2021, 10:08 PM
This revision was automatically updated to reflect the committed changes.