Implement create process snapshot and attach by AviAvni · Pull Request #91 · microsoft/clrmd

AviAvni · 2017-12-07T09:46:25Z

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

goldshtn · 2017-12-07T10:05:07Z

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 · 2017-12-12T13:15:45Z

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

leculver · 2017-12-13T05:36:24Z

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 · 2017-12-18T06:40:08Z

@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 · 2017-12-18T10:00:47Z

@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 · 2017-12-18T11:34:00Z

@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 · 2017-12-18T17:34:30Z

        private const int PROCESS_VM_READ = 0x10;
        private const int PROCESS_QUERY_INFORMATION = 0x0400;
-        public LiveDataReader(int pid)
+        public LiveDataReader(int pid, bool createSnapshot = false)


Can you make this a non-default parameter? I'd rather it be clear there's a choice here at the callsite.

leculver · 2017-12-18T17:34:55Z

+        /// <returns>A DataTarget instance.</returns>
+        public static DataTarget CreateSnapshotAndAttach(int pid)
+        {
+            IDataReader reader = new LiveDataReader(pid, true);


Can you make this:

IDataReader reader = new LiveDataReader(pid, createSnapshot:true);

leculver · 2017-12-18T17:37:11Z

+            if (createSnapshot)
+            {
+                _originalPid = pid;
+                var process = Process.GetProcessById(pid);


Can you change these 'var' statements to use the actual types? (Throughout this change.) It's not completely consistent throughout the code, but the only time var should be used is generally in statements where it reduces a ton of redundancy in a new statement. Having 'Process process', 'int hr' makes it clear what we are working with.

leculver · 2017-12-18T17:41:33Z

        #region Variables
+        private int _originalPid;
+        private IntPtr _snapshotHandle;
+        private IntPtr _vaCloneHandle;


Can this be renamed "_cloneHandle" for consistency? ClrMD doesn't really use Hungarian notation for variable names.

leculver · 2017-12-18T17:47:11Z

@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 · 2017-12-18T19:51:58Z

@leculver I fixed the style issues thanks for the review

leculver · 2017-12-18T19:58:13Z

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 · 2018-03-13T18:49:42Z

@leculver did you publish it to nuget?

leculver · 2018-03-14T17:40:06Z

@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

add create snapshot and attach feature

c3eb6fb

Alois-xx reviewed Dec 17, 2017

View reviewed changes

leculver requested changes Dec 18, 2017

View reviewed changes

fix review issues

375cf91

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

AviAvni mentioned this pull request Mar 19, 2018

Add Dump command to VS dotnet/fsharp#4580

Closed

Conversation

AviAvni commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goldshtn commented Dec 7, 2017

Uh oh!

goldshtn commented Dec 12, 2017

Uh oh!

leculver commented Dec 13, 2017

Uh oh!

Alois-xx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goldshtn commented Dec 18, 2017

Uh oh!

Alois-xx commented Dec 18, 2017

Uh oh!

goldshtn commented Dec 18, 2017

Uh oh!

leculver left a comment

Choose a reason for hiding this comment

Uh oh!

leculver Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

leculver Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

leculver Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

leculver Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

leculver commented Dec 18, 2017

Uh oh!

AviAvni commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leculver commented Dec 18, 2017

Uh oh!

AviAvni commented Mar 13, 2018

Uh oh!

leculver commented Mar 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AviAvni commented Dec 7, 2017 •

edited

Loading

Alois-xx left a comment •

edited

Loading

AviAvni commented Dec 18, 2017 •

edited

Loading