Page MenuHomePhabricator

[Dexter] Avoid potentially infinite loop in dbgeng driver
ClosedPublic

Authored by jmorse on Nov 18 2020, 12:52 PM.

Details

Summary

The "go" method of Debugger classes in Dexter used to be called once, to launch the process. However with the new DebuggerController classes it's now used to set the target process free running until we hit a relevant source line. Alas, I'd baked the former assumption into the DbgEng driver and had the "go" method do nothing. This can lead to an infinite loop where the DebuggerController repeatedly calls "go".

Quickly experimenting with setting the target process free running under DbgEng shows some weird behaviour which I'm yet to get to the bottom of; in the meantime, have the "go" method single step instead. This is slow, but makes progress.

Tests: this is stimulated by the tests in D91648

Diff Detail

Event Timeline

jmorse created this revision.Nov 18 2020, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2020, 12:52 PM
jmorse requested review of this revision.Nov 18 2020, 12:52 PM
TWeaver added a comment.EditedJan 5 2021, 9:32 AM

The "go" method of Debugger classes in Dexter used to be called once, to launch the process. However with the new DebuggerController classes it's now used to set the target process free running until we hit a relevant source line. Alas, I'd baked the former assumption into the DbgEng driver and had the "go" method do nothing. This can lead to an infinite loop where the DebuggerController repeatedly calls "go".

This assertion is false. The launch method is still called once at the start of a 'debug' session [1] [4] [5] (once all other initialisation has finished) followed by subsequent calls to the 'go' method when 'breaked' in a source file that isn't part of the test (i.e. library headers etc). If the debugger has paused within a test file the step method is called instead. [2]

This is only the case for the DefaultController and not the ConditionalController, which calls 'go' no matter what. But, and it's a big but, dbgeng doesn't have the necessary breakpoint setting methods for use with ConditionalController, so this should never happen.

The confusion here may have arose from a previous change that switched the VisualStudio debugger implementation's launch method from the step function to the go function. But again, this change shouldn't affect dbgeng's operation. [3]

It could be that the infinite looping/strange behaviour we're seeing is a result of being 'stuck' in library files and dexter repeatedly calling the empty 'go' method of dbgeng?

[1] https://github.com/llvm/llvm-project/blob/main/debuginfo-tests/dexter/dex/debugger/DebuggerControllers/DefaultController.py#L38

[2] https://github.com/llvm/llvm-project/blob/main/debuginfo-tests/dexter/dex/debugger/DebuggerControllers/DefaultController.py#L61

[3] https://github.com/llvm/llvm-project/commit/c6aa829644f30d5590451b892918298f8117c985#diff-1cda01223f50d4b2b748ef665d422cd9d4972b1843da218ce79e9c21c5e048ddR130

[4] https://github.com/llvm/llvm-project/commit/9cf9710bb0d61cb5c27c6e780af6a182cb162bfb#diff-7987664c4de69a80b4115bff1f9279d4a1707091aad95be450c5efaf75446160L123

[5] https://github.com/llvm/llvm-project/commit/f78c236efda85af1e526ac35ed535ef4786450e3#diff-7987664c4de69a80b4115bff1f9279d4a1707091aad95be450c5efaf75446160R123

(/me finally works his way through his backlog...)

Tom wrote:

The launch method is still called once at the start of a 'debug' session

Cool, I guess "go" more or less matches gdb's "continue" then,

It could be that the infinite looping/strange behaviour we're seeing is a result of being 'stuck' in library files and dexter repeatedly calling the empty 'go' method of dbgeng?

I think that's it, hence getting it to at least step to make progress in this patch. Dexter is still vulnerable to stepping through all of printf, but at least it'll make progress and not hang forever this way.

(It's been a while since I did battle with COM, fixing dbgengs free-running is making its way slowly up my todo list).

TWeaver accepted this revision.Jan 28 2021, 9:11 AM

Looks good to me.

This revision is now accepted and ready to land.Jan 28 2021, 9:11 AM
This revision was automatically updated to reflect the committed changes.