Page MenuHomePhabricator

[Dexter] Add DexLimitSteps command and ConditionalController implementation
ClosedPublic

Authored by TWeaver on May 12 2020, 7:19 AM.

Details

Summary
  • Adds DexLimitSteps Command.
  • Add ConditionalController, a new DebuggerController type.
  • 5 regression tests
  • documentation

THE PROBLEM

The default operating behaviour of dexter would step onto every line and gather step information for each of them. This means that, given a large enough test program, dexter would take an undesirable amount of time to run.

This is because dexter would set a break point on every line of the program, step onto each line, repeatedly if within a loop, and attempt to gather step information. Even if no step information is required for a line, the debugger would halt, context switch to dexter, dexter would then attempt to get step info and then move onto the next line.

This is a problem when we have tight loops for example that run thousands of iterations. With an unoptimized binary dexter, a for loop with 100 iterations could take as long as 20/30 seconds to complete.

The debugging information for that for loop shouldn't change during each iteration, so why gather step info for every iteration?

THE SOLUTION

Allow test writers to specify a range of lines in a test program that dexter will conditionally gather step information for. The condition will be predicated on a variable value at run time. If I have a loop counter variable ix, and ix increments up to 100, then specify a range of lines within the loop that we gather step information only when ix is equal to 50, 65 and 90.

Using the DexLimitSteps command, test writers can now write tests that have this behaviour.

DexLimitSteps('ix', 50, 65, 90, from_line='start_of_for', to_line'end_of_for')

This expresses that the test write wishes to only gather step information on the range of lines from start_of_for TO end_of_for when the loop counter variable 'ix' is equal to 50, 65, 90.

Without a DexLimitSteps command to specify a conditional range of lines to step on, the default behaviour would dumbly gather step information for all 100 iterations of the for loop. This is costly in time.

THE IMPLEMENTATION

Given a DexLimitSteps command within a test suite, we then have to 'control' the debugger to carry out these instructions. The old 'default' behaviour that is now encapsulated in the 'DefaultController' of dexter would require heavy modification to work with both behaviours. Much simpler then to create a new class that can carry out the conditional stepping behaviour we desire.

Drum roll please, presenting the 'ConditionalController'

The conditional controller's job is to 'control/pull the strings' of the debugger in order to carry out conditional stepping behaviour. For every 'DexLimitSteps' command in your test program it creates a conditional break point at the start of the range you defined with "from_line='from', to_line='to'".

There maybe multiple conditional breakpoints on a single line, each one testing a different condition for each variable and value combination specified by the 'DexLimitSteps' commands in the test. The conditional checking is handled by the debugger implementation natively as context switching back to dexter to check them on each iteration is slow.

When a conditional break point is hit, we get a list of all the conditional ranges that start on that line. It's not currently possible to check which break point was last hit for both the VS Debugger and LLDB, so instead we have to recheck that the conditionals were hit. As one or more conditional range can start on the same line, we need to check which conditions are true in Dexter in order to set the correct ranges, this is handled my _conditional_met(), probably the hackiest part of this implementation, recommendations on how to spruce this part up are welcomed.

Remember that we only set a conditional break point on the first line on a range. If a conditional bp fires, we then set a range of unconditional break points through out the rest of the range of 'from_line='from' TO to_line='to'. We gather step information for the line the conditional was hit and then continue execution up to the next break point.

Any non-conditional break points on a line we breaked on get deleted immediately before we continue execution.

All this boils down to is a set of lines in a program that the conditional controller gathers step information for IFF the value of a variable is equal to an expected value at runtime.

Diff Detail

Event Timeline

TWeaver created this revision.May 12 2020, 7:19 AM

missing note, dbgeng does not support the new DexLimitSteps commands as of right now.

We do have Visual Studio and LLDB support though.

rnk removed a reviewer: rnk.May 13 2020, 1:56 PM

This all seems like a good idea, but I don't know dexter so.. :)

@echristo thanks for your interest. do you need any further clarification on what dexter does or how this patch fits in with the existing behaviour?

One nit inline; on the broader topic of review, we've (Sony) been reviewing things like this internally first. I guess we should move all of that to phab seeing how the code lives in the monorepo now.

debuginfo-tests/dexter/dex/debugger/lldb/LLDB.py
116

AFAIUI "LoadDebugger.." is more about config and loading time errors, do we have a better exception class for "stuff that broke while we were running?"

TWeaver marked an inline comment as done.May 15 2020, 8:53 AM
TWeaver added inline comments.
debuginfo-tests/dexter/dex/debugger/lldb/LLDB.py
116

good point, this is copy pasta from add_breakpoint() tbh.

We have DebuggerException('your message here').

Should be able to drop in and replace LoadDebuggerException with no issues.

This also raises questions around the validity of the LoadDebuggerException raised in add_breakpoint too. If my memory serves then the original default debugger behaviour would set breakpoints during the initialisation phase, hence why this Load error is here... An issue of the behaviour of debugger control seeping into the debugger features/functionality.

Given that add_breakpoint could potentially be called from many different controllers it'd make sense to move the LoadDebuggerException out to the DefaultController and a raise a DebuggerException instead.

jmorse accepted this revision.May 19 2020, 6:48 AM

(Inline):

Given that add_breakpoint could potentially be called from many different controllers it'd make sense to move the LoadDebuggerException out to the DefaultController and a raise a DebuggerException instead.

Cool -- LGTM with that twiddled. Best to leave it a day or so in case there's more interest from elsewhere; and we'll move to fully phab reviews to include more people in the process.

This revision is now accepted and ready to land.May 19 2020, 6:48 AM
TWeaver updated this revision to Diff 267892.Jun 2 2020, 8:02 AM

removed and replaced LoadDebuggerException with DebuggerException in LLDB.

Moved LoadDebuggerException out to DefaultController.

There's a bunch of dexter test failures that popped up on the bots today: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/20274/

Could they be related to this change?

@aprantl I'll take a look, thanks for pointing them out.

seems I've made a more mundane error of committing a diff file as part of my patch. this has been reverted for now.

I'll continue looking into the test failures.

Hi Adrian (@aprantl)

I've been unable to reproduce the errors seen in job 20274.

This and another Dexter commit was built and tested in job 20264. 20264 had tests failures too, but not the same failures seen in 20274.

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/20264/

by job 20269 the CI pipeline went green with no test failures. No intermediate jobs between 20269 and 20264 showed the dexter-test failures.

In fact, the only job I can find that has the failures is the job you originally linked, 20274. 20275, the job afterwards is also green and doesn't contain any commits in a relevant area, AFAIK. This is quite perplexing as I would expect, given no other changes to dexter, we'd still be seeing the test failures introduced in 20274.

I'm also unable to reproduce the failures on my local machine, even with a build at the point of introducing my patch, a build at the point the CI encountered the failures and a build afterwards but before I reverted.

As such, I'm not sure what to chalk this up to other than some transient error in the CI perhaps.

I did go down one rabbit hole to try and find some answers but came back with nothing concrete; job 20274 contains a patch that adds a new diagnostic warning to DiagnosticsSemaKind.td, you can see the diff at the following link:

https://reviews.llvm.org/D79895

Given that the tests are failing on an assertion in clang/include/clang/Basic/Diagnostics.h, it could be that some 'strange' and 'unexpected' behavior during tablegenning of the aforementioned .td file may have caused the failures we've seen, and that it's a coincidence that this aligned with my patches being introduced.

I've checked the logs for the 20274 build and taken a look at the table gen lines and found nothing suspicious, but this is the only explanation that has any kind of link to the failing job that I can find, so that's all I can offer I'm afraid.

Not a particularly satisfying answer, so you have my apologies on that, but, as said, I can't reproduce and neither does the CI atm, so it seems the failure was temporary.

Hope this helps,
Tom W

It looks like the bots recovered by now, which makes it likely to have been an unrelated clang regression. Thanks for checking!

MaskRay added inline comments.
debuginfo-tests/dexter/Commands.md
178

There is an extra , (in ],[)

debuginfo-tests/dexter/dex/debugger/lldb/LLDB.py
121

range(bp_count) is more common

debuginfo-tests/dexter/feature_tests/commands/perfect/limit_steps/limit_steps_check_json_step_count.cpp
1

In most other places a file level comment does not start with Purpose:. I think you may consider deleting Purpose:

Comments probably should start with /// to make them stand out from RUN/CHECK lines and avoid future enhanced FileCheck diagnostics (If a check prefix appears in the comment without a punctuation (:), a future FileCheck might warn/error. /// can suppress the diagnostic)

19

DexLimitSteps appears to affect DexExpectWatchValue above it, but the interaction is not very clear...

debuginfo-tests/dexter/feature_tests/commands/perfect/limit_steps/limit_steps_expect_loop.cpp
20

DexLimitSteps seems to take effect on nearby DexExpectWatchValue, right?

But how they interact isn't very clear from the documentation...

MaskRay added inline comments.Jul 9 2020, 11:15 AM
debuginfo-tests/dexter/feature_tests/commands/perfect/limit_steps/limit_steps_check_json_step_count.cpp
9

I think it would be clearer if you expand the output:

[0, "main", "{{.*}}limit_steps_check_json_step_count.cpp", [[#@LINE+5]], 15, "StopReason.BREAKPOINT", "StepKind.FUNC", []]
[1, "main", "{{.*}}limit_steps_check_json_step_count.cpp", [[#@LINE+4]], 15, "StopReason.BREAKPOINT", "StepKind.FUNC", []]
[2, "main", "{{.*}}limit_steps_check_json_step_count.cpp", [[#@LINE+3]], 15, "StopReason.BREAKPOINT", "StepKind.FUNC", []]