Page MenuHomePhabricator

[dexter] Fix DexLimitSteps when breakpoint can't be set at requested location
ClosedPublic

Authored by Orlando on Mar 16 2021, 5:19 AM.

Details

Summary

Using a DexLimitSteps command forces dexter to use the ConditionalController
debugger controller. At each breakpoint the ConditionalController needs to
understand which one has been hit. Prior to this patch, upon hitting a
breakpoint, dexter used the current source location to look up which requested
breakpoint had been hit.

A breakpoint may not get set at the exact location that the user (dexter)
requests. For example, if the requested breakpoint location doesn't exist
in the line table then then debuggers will (usually, AFAICT) set the breakpoint
at the next available valid breakpoint location.

This meant that, occasionally in unoptimised programs and frequently in
optimised programs, the ConditionalController was failing to determine which
breakpoint had been hit.

This is the fix:

Change the DebuggerBase breakpoint interface to use opaque breakpoint ids
instead of using source location to identify breakpoints, and update the
ConditionalController to track breakpoints instead of locations.

These now return a breakpoint id:

add_breakpoint(self, file_, line)
_add_breakpoint(self, file_, line)
add_conditional_breakpoint(self, file_, line, condition)
_add_conditional_breakpoint(self, file_, line, condition)

Replace:

delete_conditional_breakpoint(self, file_, line, condition)
_delete_conditional_breakpoint(self, file_, line, condition)

with:

delete_breakpoint(self, id)

Add:

get_triggered_breakpoint_ids(self)

A breakpoint id is guaranteed to be unique for each requested breakpoint, even
for duplicate breakpoint requests. Identifying breakpoints like this, instead
of by location, removes the possibility of mixing up requested and bound
breakpoints.

This closely matches the LLDB debugger interface so little work was required in
LLDB.py, but some extra bookkeeping is required in VisualStudio.py to maintain
the new breakpoint id semantics. No implementation work has been done in
dbgeng.py as DexLimitSteps doesn't seem to support dbgeng at the moment.

Testing
Added:
dexter/feature_tests/commands/perfect/limit_steps/limit_steps_line_mismatch.cpp

There were no unexpected failures running the full debuginfo-tests suite.

The regression tests use dbgeng on windows by default, and as mentioned above
dbgeng isn't supported yet, so I have also manually tested (i.e. without lit)
that this specific test works as expected with clang and Visual Studio 2017 on
Windows.

Diff Detail

Event Timeline

Orlando requested review of this revision.Mar 16 2021, 5:19 AM
Orlando created this revision.

To my mind, the two attribute names, _conditional_bp_ranges and _conditional_bps are quite similar -- but AFAICT the former is a on-initialization record of where conditional bps should be placed, while the latter is a "live" record of conditional bp handles, is that right? IMO, better to have an indication of this distinction in the name, may I suggest _conditional_bp_handles?

Otherwise LGTM

Orlando updated this revision to Diff 331549.Mar 18 2021, 6:49 AM

To my mind, the two attribute names, _conditional_bp_ranges and _conditional_bps are quite similar -- but AFAICT the former is a on-initialization record of where conditional bps should be placed, while the latter is a "live" record of conditional bp handles, is that right? IMO, better to have an indication of this distinction in the name, may I suggest _conditional_bp_handles?

Yeah that's right, and I agree. Done! Thanks for taking a look.

One small suggestion, otherwise LGTM.

debuginfo-tests/dexter/dex/debugger/DebuggerControllers/ConditionalController.py
99

_pause_between_steps seems odd. Maybe 'time_between_steps_ms' is more decriptive. I realise that variable wasn't introduced in this patch

Orlando added inline comments.Mar 19 2021, 9:16 AM
debuginfo-tests/dexter/dex/debugger/DebuggerControllers/ConditionalController.py
99

I agree that the name could be better. It looks like the name is used elsewhere too though. In ConditionalController.__init__ we have
self._pause_between_steps = context.options.pause_between_steps. I think it makes sense to keep the name the same as other uses, and I'm not sure updating the name elsewhere should be in the scope of this patch. So I don't think it's worth changing here. Wdyt?

TWeaver accepted this revision.Mar 23 2021, 4:01 AM

Hey Orlando! thanks for this. A really nice, solid reimplementation and improvement of the original work. Great code comments too.

thanks again, LGTM.

This revision is now accepted and ready to land.Mar 23 2021, 4:01 AM

Hey Orlando! thanks for this. A really nice, solid reimplementation and improvement of the original work. Great code comments too.

thanks again, LGTM.

Thanks @TWeaver! And thank you everyone for the reviews.

debuginfo-tests/dexter/dex/debugger/DebuggerControllers/ConditionalController.py
99

I landed this just now, and in coming back to comment realise that we didn't resolve our conversation @chrisjackson, sorry! Happy to follow up with a quick patch to address this if you still think it should be changed?