Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

debug: make allThreadStop reason generic #6627

Merged
merged 1 commit into from
Nov 27, 2019
Merged

debug: make allThreadStop reason generic #6627

merged 1 commit into from
Nov 27, 2019

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Nov 25, 2019

What it does

When we receive a stopped event from a debug adapter, a
allThreadStopped flag can be set. When doing so, it feels weird to
display the reason of the thread that caused it on other threads.

This commit reports "PAUSED" for all the paused threads, but keeps
displaying the reason for the thread that caused the event.

How to test

Run a multi-threaded debug session that pauses every thread when one hit a breakpoint hit. The latter should be stopped with reason "PAUSED ON BREAKPOINT", while all the others threads should just display "PAUSED".

I used the following vscode extension and workspace (linux/mac? only)

Run the task "Build C++", then you should be able to debug using the "Debug C++" configuration. Place a breakpoint on line 6 to break inside a function that runs in a separate thread.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added debug issues that related to debug functionality enhancement issues that are enhancements to current functionality - nice to haves labels Nov 25, 2019
@paul-marechal
Copy link
Member Author

This PR aligns Theia's UI with VS Code.

@akosyakov
Copy link
Member

Run a multi-threaded debug session that pauses every thread when one hit a breakpoint hit. The latter should be stopped with reason "PAUSED ON BREAKPOINT", while all the others threads should just display "PAUSED".

Please elaborate how with which extension which setup.

@paul-marechal
Copy link
Member Author

@akosyakov updated

@paul-marechal paul-marechal force-pushed the mp/da-pause branch 2 times, most recently from fca964a to f3fddd9 Compare November 25, 2019 18:30
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code-wise it makes sense to me

@marcdumais-work
Copy link
Contributor

Hi @marechal-p ,

I noticed something that seems wrong. The thread that hits the breakppoint is ok, but the other ones, though shown as "Paused" do not allow getting stack and variable info: "Not paused".

Peek 2019-11-26 07-40

@marcdumais-work
Copy link
Contributor

Here's the same scenario, on master:

Peek 2019-11-26 07-44

Ideally we would not have to click "load more frames", but otherwise it works well.

@marcdumais-work
Copy link
Contributor

I tested debugging Java with a small multi-threaded program and did not notice any problems vs master.

@paul-marechal
Copy link
Member Author

I will investigate this call stack issue a bit more before thinking about merging.

@marcdumais-work
Copy link
Contributor

@marechal-p As you mentioned offline, when debugging C/C++ on theia master, the stack frames fetched are bogus - they are a copy of the stack frames of the thread that stopped on the breakpoint, not of the selected thread.

So the issue I found above may not be as bad as I initially thought.

When we receive a stopped event from a debug adapter, a
`allThreadStopped` flag can be set. When doing so, it feels weird to
display the reason of the thread that caused it on other threads.

This commit reports "PAUSED" for all the paused threads, but keeps
displaying the reason for the thread that caused the event.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@paul-marechal
Copy link
Member Author

paul-marechal commented Nov 26, 2019

@marcdumais-work the C++ debug adapter I have been using is not really good at fetching stack frames for threads paused with the allThreadStopped flag. If you can check again with your Java setup that we don't get regressions, then feel free to merge :)

@marcdumais-work
Copy link
Contributor

@marcdumais-work the C++ debug adapter I have been using is not really good at fetching stack frames for threads paused with the allThreadStopped flag. If you can check again with your Java setup that we don't get regressions, then feel free to merge :)

Ok, let me check with the latest version of this PR.

@marcdumais-work
Copy link
Contributor

Ok, let me check with the latest version of this PR.

@marechal-p Java debugging is still good with latest version of this PR 👍

@paul-marechal paul-marechal merged commit 87df21b into master Nov 27, 2019
@paul-marechal paul-marechal deleted the mp/da-pause branch November 27, 2019 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality enhancement issues that are enhancements to current functionality - nice to haves
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants