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

Fix for Issue #844 where pause does not work on Linux and Mac when using gdbservers #855

Merged
merged 9 commits into from
May 14, 2019

Conversation

haneefdm
Copy link
Contributor

@haneefdm haneefdm commented May 7, 2019

There are two ways of having local gdb but running with a gdbserver. You can either use the miDebuggerServerAddress or debugServerPath in launch configuration. Most of us the debugServerPath to start and end the server with the debug session because of additional configuration that is needed to get the server running properly.

When using a gdbserver, the logic was wrong for the case of debugServerPath on linux/Mac and we did kill a process that was not running locally -- ending up potentially killing a random process.

The fix is to treat miDebuggerServerAddress and debugServerPath similarly. I refactored some code and put into a single function IsLocalLaunchUsingServer() as this kind of logic was used in multiple places. I also found that the checks for the use of these properties were inconsistent. Sometimes it was string. IsNullOrWhiteSpace() and sometimes string. IsNullOrEmpty(). Now, they are all IsNullOrWhiteSpace() for these two options.

I renamed two methods IsLocalGdb and IsRemoteGdb to be more meaningful. Add a 'Target' suffix to them

Here is my testing matrix.

Basically, I had to test on all three platforms in multiple configurations. Many fails turned into passes with these changes.

My first PR so, let me know if I missed anything. I could not run the CodeFormatter.exe -- it was crashing due to a missing dll, but I formatted my changes with the VStudio editor.

In the process of this, I found a couple more issues but I want to treat them as separate things. I documented them in my 'testing matrix' link above.

@msftclas
Copy link

msftclas commented May 7, 2019

CLA assistant check
All CLA requirements met.

src/MICore/Debugger.cs Outdated Show resolved Hide resolved
src/MICore/Debugger.cs Outdated Show resolved Hide resolved
@pieandcakes
Copy link
Collaborator

@haneefdm Thank you for submitting this!

@pieandcakes
Copy link
Collaborator

resolves #844

Copy link
Member

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for submitting this!

@gregg-miskelly
Copy link
Member

@pieandcakes Do you have any additional comments? @paulmaybee Do you want to review this since it touches some features that you added?

@gregg-miskelly gregg-miskelly merged commit ecc1b0e into microsoft:master May 14, 2019
@gregg-miskelly
Copy link
Member

@haneefdm Thanks again for submitting this!

@haneefdm
Copy link
Contributor Author

Thank you all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants