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

Implement create process snapshot and attach #91

Merged
merged 2 commits into from Dec 18, 2017

Conversation

Projects
None yet
4 participants
@AviAvni
Contributor

AviAvni commented Dec 7, 2017

This pr is create at SELA hackathon with Sasha Goldshtein @goldshtn

@goldshtn

This comment has been minimized.

Show comment
Hide comment
@goldshtn

goldshtn Dec 7, 2017

To clarify this PR a bit: the Windows Process Snapshotting API provides a facility for creating a virtual address space clone of the target process. By creating a clone and then attaching the CLRMD live data reader to the clone we benefit from the best of both worlds:

  • The target process is not suspended at all (just like AttachMode.Passive), nor do we generate a dump file
  • We get a consistent snapshot of the target process (like AttachMode.NonInvasive or a dump file)

Creating the clone is a relatively cheap operation, however the cloned pages are copy-on-write. If the original process modifies a page, it is duplicated in RAM for the clone.

One final caveat is that Process Snapshotting is supported on Windows 8.1/Windows Server 2012 R2 and later.

goldshtn commented Dec 7, 2017

To clarify this PR a bit: the Windows Process Snapshotting API provides a facility for creating a virtual address space clone of the target process. By creating a clone and then attaching the CLRMD live data reader to the clone we benefit from the best of both worlds:

  • The target process is not suspended at all (just like AttachMode.Passive), nor do we generate a dump file
  • We get a consistent snapshot of the target process (like AttachMode.NonInvasive or a dump file)

Creating the clone is a relatively cheap operation, however the cloned pages are copy-on-write. If the original process modifies a page, it is duplicated in RAM for the clone.

One final caveat is that Process Snapshotting is supported on Windows 8.1/Windows Server 2012 R2 and later.

@goldshtn

This comment has been minimized.

Show comment
Hide comment
@goldshtn

goldshtn Dec 12, 2017

Hi @leculver, I was wondering if we could get some feedback on this PR, or ideally get it merged. Thanks!

goldshtn commented Dec 12, 2017

Hi @leculver, I was wondering if we could get some feedback on this PR, or ideally get it merged. Thanks!

@leculver

This comment has been minimized.

Show comment
Hide comment
@leculver

leculver Dec 13, 2017

Contributor

Overall this looks good. I'd like to double check a few things but I plan to merge this later in the week.

Contributor

leculver commented Dec 13, 2017

Overall this looks good. I'd like to double check a few things but I plan to merge this later in the week.

@Alois-xx

Can CreateSnapshotAndAttach not be merged with AttachToProcess with a flag which tries at first to create a snapshot and the falls back to Invasive mode if this is tried e.g. on an earlier Windows version?

@goldshtn

This comment has been minimized.

Show comment
Hide comment
@goldshtn

goldshtn Dec 18, 2017

@Alois-xx I don't think it's a good idea to change the default behavior of AttachToProcess to use a snapshot. Attaching in passive mode consumes virtually no resources on the target -- there is no clone, there is no extra memory usage. OTOH creating a process snapshot has the potential for using more resources. Also, using a snapshot would not support the Windows debugging interfaces, unlike AttachFlag.Invasive. All in all, I think process snapshotting warrants a separate mode -- it would be confusing otherwise.

goldshtn commented Dec 18, 2017

@Alois-xx I don't think it's a good idea to change the default behavior of AttachToProcess to use a snapshot. Attaching in passive mode consumes virtually no resources on the target -- there is no clone, there is no extra memory usage. OTOH creating a process snapshot has the potential for using more resources. Also, using a snapshot would not support the Windows debugging interfaces, unlike AttachFlag.Invasive. All in all, I think process snapshotting warrants a separate mode -- it would be confusing otherwise.

@Alois-xx

This comment has been minimized.

Show comment
Hide comment
@Alois-xx

Alois-xx Dec 18, 2017

@goldshtn: When I call ClrMD from a command line tool like MemAnalyzer I would always need to think if I can use PSS or not and pass a different command line flag to it. Procdump follows this route but what are the reasons that I would not want to use the faster approach always?
I have tried PSS years ago on Server 2012 machines which resulted in dumps which were not usable at all. Since I want to reliably get full memory dumps I have never tried the PSS feature again. Is this now working reliably?

Yes it would be confusing if not supported functionality will not work on a newer OS because a PSS is now used. But that should be a decision of the caller of the API if he wants to use an AttachFlag like InvasiveOrSnapshot if he can live without dbgeng support.

Alois-xx commented Dec 18, 2017

@goldshtn: When I call ClrMD from a command line tool like MemAnalyzer I would always need to think if I can use PSS or not and pass a different command line flag to it. Procdump follows this route but what are the reasons that I would not want to use the faster approach always?
I have tried PSS years ago on Server 2012 machines which resulted in dumps which were not usable at all. Since I want to reliably get full memory dumps I have never tried the PSS feature again. Is this now working reliably?

Yes it would be confusing if not supported functionality will not work on a newer OS because a PSS is now used. But that should be a decision of the caller of the API if he wants to use an AttachFlag like InvasiveOrSnapshot if he can live without dbgeng support.

@goldshtn

This comment has been minimized.

Show comment
Hide comment
@goldshtn

goldshtn Dec 18, 2017

@Alois-xx Sorry, I disagree. It is already a decision of the API client to use the snapshot API. I don't think we should be making this choice automatically because of its potential cost and complications. And yes, PSS is more stable but the whole thing is not as thoroughly tested as CLRMD's main features, which have been around for years.

goldshtn commented Dec 18, 2017

@Alois-xx Sorry, I disagree. It is already a decision of the API client to use the snapshot API. I don't think we should be making this choice automatically because of its potential cost and complications. And yes, PSS is more stable but the whole thing is not as thoroughly tested as CLRMD's main features, which have been around for years.

@leculver

Overall this looks good. If you can fix a few minor nitpicks I'll merge the PR.

@leculver

This comment has been minimized.

Show comment
Hide comment
@leculver

leculver Dec 18, 2017

Contributor

@AviAvni @goldshtn Really sorry about the delay. I was out sick the second half of last week and wasn't able to get around to this. Thanks for the submission. I have a few modification requests to match the overall style of the code in ClrMD, but functionally it looks good.

@Alois-xx I do not think this should be the default for AttachToProcess with a fallback. The user of the API should make a clear choice between using snapshots or attaching to a live process. Remember, ClrMD supports a bunch of odd scenarios. The LiveDataTarget can be used in places where a 'real' debugger is already attached and has the process paused (such as WinDbg/VS already attached to a process and you want to do extra inspection). Trying to clone the process is just a waste in those scenarios.

Contributor

leculver commented Dec 18, 2017

@AviAvni @goldshtn Really sorry about the delay. I was out sick the second half of last week and wasn't able to get around to this. Thanks for the submission. I have a few modification requests to match the overall style of the code in ClrMD, but functionally it looks good.

@Alois-xx I do not think this should be the default for AttachToProcess with a fallback. The user of the API should make a clear choice between using snapshots or attaching to a live process. Remember, ClrMD supports a bunch of odd scenarios. The LiveDataTarget can be used in places where a 'real' debugger is already attached and has the process paused (such as WinDbg/VS already attached to a process and you want to do extra inspection). Trying to clone the process is just a waste in those scenarios.

@AviAvni

This comment has been minimized.

Show comment
Hide comment
@AviAvni

AviAvni Dec 18, 2017

Contributor

@leculver I fixed the style issues thanks for the review

Contributor

AviAvni commented Dec 18, 2017

@leculver I fixed the style issues thanks for the review

@leculver leculver merged commit 526ce44 into Microsoft:master Dec 18, 2017

1 check passed

license/cla All CLA requirements met.
Details
@leculver

This comment has been minimized.

Show comment
Hide comment
@leculver

leculver Dec 18, 2017

Contributor

Merged. Unfortunately the internal build sever where nuget packages are produced seems to be down, and most of CLR is out until Jan 2. I'm going to try to put together a nuget release as soon as they are back and can take a look at why ClrMD builds aren't getting produced. (Also hopefully adding strong name signing.)

Contributor

leculver commented Dec 18, 2017

Merged. Unfortunately the internal build sever where nuget packages are produced seems to be down, and most of CLR is out until Jan 2. I'm going to try to put together a nuget release as soon as they are back and can take a look at why ClrMD builds aren't getting produced. (Also hopefully adding strong name signing.)

@AviAvni

This comment has been minimized.

Show comment
Hide comment
@AviAvni

AviAvni Mar 13, 2018

Contributor

@leculver did you publish it to nuget?

Contributor

AviAvni commented Mar 13, 2018

@leculver did you publish it to nuget?

@leculver

This comment has been minimized.

Show comment
Hide comment
@leculver

leculver Mar 14, 2018

Contributor

@AviAvni I pushed a new version to NuGet last week. Unfortunately policies changed here and I had to go get re-approved for signing/shipping changes. Sorry for the delay.

This build should contain the change: https://www.nuget.org/packages/Microsoft.Diagnostics.Runtime/0.9.180305.1

Contributor

leculver commented Mar 14, 2018

@AviAvni I pushed a new version to NuGet last week. Unfortunately policies changed here and I had to go get re-approved for signing/shipping changes. Sorry for the delay.

This build should contain the change: https://www.nuget.org/packages/Microsoft.Diagnostics.Runtime/0.9.180305.1

@AviAvni AviAvni referenced this pull request Mar 19, 2018

Open

Add Dump command to VS #4580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment