Procdump Arguments Changed #1700

vagisha-nidhi · 2018-07-26T12:14:37Z

Description

Procdump options enhanced to handle crash scenorios in test host. STACK_OVERFLOW and ACCESS_VIOLATION filter added to get dumps on these crashes.
Procdump is launched according to the process. The version of the procdump chosen is now depending on the bitness of the test host.
Validated the scenarios on 32 bit OS as well as 64 bit OS.

Using procdump options as
For mini dump : -accepteula -e 1 -g -t -f STACK_OVERFLOW -f ACCESS_VIOLATION
For full dump : -accepteula -e 1 -g -t -ma -f STACK_OVERFLOW -f ACCESS_VIOLATION

-accepteula: Auto accept end-user license agreement
-e: Write a dump when the process encounters an unhandled exception. Include the 1 to create dump on first chance exceptions.
-g: Run as a native debugger in a managed process (no interop).
-t: Write a dump when the process terminates.
-ma: Full dump argument.
-f: Filter the exceptions.

abhishkk · 2018-07-27T07:01:46Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Constants.cs

@@ -42,5 +42,10 @@ internal static class Constants
        /// Configuration key name for dump mode
        /// </summary>
        public const string DumpModeKey = "CollectDump";
+
+        /// <summary>
+        /// X86 Test Host Process Name


Nit: small casing X86 test host process name

abhishkk · 2018-07-27T07:05:33Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

@@ -100,27 +99,38 @@ private static string BuildProcDumpArgs(int processId, string filename)
            // -accepteula: Auto accept end-user license agreement
            // -t: Write a dump when the process terminates.
            // This will create a minidump of the process with specified filename
-            return "-accepteula -t " + processId + " " + filename + ".dmp";
+            return "-accepteula -e 1 -g -t -f STACK_OVERFLOW -f ACCESS_VIOLATION " + processId + " " + filename + ".dmp";


Can you write in detail in description what validations you did for these parameters check?

Updated the description.

abhishkk · 2018-07-27T07:07:43Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

@@ -100,27 +99,38 @@ private static string BuildProcDumpArgs(int processId, string filename)
            // -accepteula: Auto accept end-user license agreement
            // -t: Write a dump when the process terminates.
            // This will create a minidump of the process with specified filename
-            return "-accepteula -t " + processId + " " + filename + ".dmp";
+            return "-accepteula -e 1 -g -t -f STACK_OVERFLOW -f ACCESS_VIOLATION " + processId + " " + filename + ".dmp";


STACK_OVERFLOW and ACCESS_VIOLATION.. pick these values from a list. This list will contain only these 2 values for now but can be extended in future.

use a string builder to build the params.

Done. Picking values from a list of exceptionFilters now.

abhishkk · 2018-07-27T07:10:07Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

            if (!string.IsNullOrWhiteSpace(procdumpPath))
            {
                string filename = string.Empty;

-                if (this.environment.Architecture == PlatformArchitecture.X64)
+                // Launch procdump according to process architecture
+                if (this.environment.Architecture == PlatformArchitecture.X86 || processName == Constants.X86TestHostProcessName)


I think we dont need this (this.environment.Architecture == PlatformArchitecture.X86) change as in 32 bit OS, we can't run 64 bit process. So only second condition (processName == Constants.X86TestHostProcessName) is sufficient.

Changed the logic to determine if a process is 32 bit or not using ISWow64 Native method. We don't need to use processName == Constants.X86TestHostProcessName now

abhishkk · 2018-07-27T07:14:11Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

            if (!string.IsNullOrWhiteSpace(procdumpPath))
            {
                string filename = string.Empty;

-                if (this.environment.Architecture == PlatformArchitecture.X64)
+                // Launch procdump according to process architecture
+                if (this.environment.Architecture == PlatformArchitecture.X86 || processName == Constants.X86TestHostProcessName)


ProcDumpUtility should not be aware of what process it is taking dump. So dont use X86TestHostProcessName name directly here. Instead pass it via blameCollector.

Using IsWow64.

abhishkk · 2018-07-27T07:16:05Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

                {
-                    filename = "procdump64.exe";
+                    filename = "procdump.exe";


Define these strings in constants

Yes okay. Defined in Constants.

abhishkk · 2018-07-27T07:16:46Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

@@ -100,27 +99,38 @@ private static string BuildProcDumpArgs(int processId, string filename)
            // -accepteula: Auto accept end-user license agreement
            // -t: Write a dump when the process terminates.
            // This will create a minidump of the process with specified filename
-            return "-accepteula -t " + processId + " " + filename + ".dmp";
+            return "-accepteula -e 1 -g -t -f STACK_OVERFLOW -f ACCESS_VIOLATION " + processId + " " + filename + ".dmp";


Output in verbose the command which we are passing to procdump.

abhishkk · 2018-07-27T07:19:05Z

test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/ProcessDumpUtilityTests.cs

+        /// Start process dump will start exe according to Test host Process
+        /// </summary>
+        [TestMethod]
+        public void StartProcessDumpWillStartExeCorrespondingToTestHostProcessIn64BitOS()


test the command which we are passing to procdump as well whether if its correct.

kaadhina · 2018-07-30T14:04:35Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

@@ -100,27 +99,38 @@ private static string BuildProcDumpArgs(int processId, string filename)
            // -accepteula: Auto accept end-user license agreement
            // -t: Write a dump when the process terminates.
            // This will create a minidump of the process with specified filename
-            return "-accepteula -t " + processId + " " + filename + ".dmp";
+            return "-accepteula -e 1 -g -t -f STACK_OVERFLOW -f ACCESS_VIOLATION " + processId + " " + filename + ".dmp";


Are we okay with -g option @abhishkk

@kaadhina :
@vagisha-nidhi did extensive validations with known crash scenarios in different possible scenarios. Also she validated if the dump created is usable.

kaadhina · 2018-07-30T14:08:10Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

            if (!string.IsNullOrWhiteSpace(procdumpPath))
            {
                string filename = string.Empty;

-                if (this.environment.Architecture == PlatformArchitecture.X64)
+                // Launch procdump according to process architecture
+                if (this.environment.Architecture == PlatformArchitecture.X86 || processName == Constants.X86TestHostProcessName)


is it possible to find the bitness from process id instead of relying on process name?

Yes. On 64 bit OS, we are now determinig if a process is 32 bit by using just the process Id .

kaadhina · 2018-08-01T04:18:17Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/NativeMethodsHelper.cs

+        /// <param name="processHandle">Process Handle</param>
+        /// <returns>Bool for Is64Bit</returns>
+        public bool Is64Bit(IntPtr processHandle)
+        {


Does this work for 32 bit os as well

We use this condition only for 64 bit

abhishkk · 2018-08-01T05:27:35Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Constants.cs

@@ -44,6 +44,16 @@ internal static class Constants
        public const string DumpModeKey = "CollectDump";

        /// <summary>
+        /// Procdump 32 bit version
+        /// </summary>
+        public const string ProcdumpProcessName = "procdump.exe";


Nit: can remove Name from both variables.

Updated the names

abhishkk · 2018-08-01T05:31:35Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/INativeMethodsHelper.cs

+{
+    using System;
+
+    public interface INativeMethodsHelper


We can remove this interface and make NativeMethodsHelper a static class as it has methods which are not using any class level variables. Also class seems more like helper class this making it static makes sense.

Made it via interface to use it for unit tests.

abhishkk · 2018-08-01T05:31:44Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/NativeMethodsHelper.cs

+    using System.Runtime.InteropServices;
+    using Microsoft.TestPlatform.Extensions.BlameDataCollector.Interfaces;
+
+    public class NativeMethodsHelper : INativeMethodsHelper


abhishkk · 2018-08-01T05:34:21Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/NativeMethodsHelper.cs

+            bool isWow64 = false;
+            if (!IsWow64Process(processHandle, out isWow64))
+            {
+                throw new Win32Exception();


Why we are throwing Win32Exception? We are not catching it anywhere and there are no UTs for it. Can't we simply return the flag?

Removed the exception thrown.

abhishkk · 2018-08-01T05:35:04Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/NativeMethodsHelper.cs

+            return !isWow64;
+        }
+
+        [DllImport("kernel32.dll", SetLastError = true, CallingConvention = CallingConvention.Winapi)]


Can you add a comment for this method?

Added comments.

abhishkk · 2018-08-01T05:37:08Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

            // -t: Write a dump when the process terminates.
+            // -ma: Full dump argument.
+            // -f: Filter the exceptions.
+            StringBuilder procDumpArgument = new StringBuilder("-accepteula -e 1 -g -t ");


abhishkk · 2018-08-01T05:40:15Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

                {
-                    filename = "procdump.exe";
+                    if (this.nativeMethodsHelper.Is64Bit(this.processHelper.GetProcessHandleById(processId)))


Nit: Can use ternary operator for reducing lines.

filename = this.nativeMethodsHelper.Is64Bit(this.processHelper.GetProcessHandleById(processId)) ? Constants.Procdump64ProcessName : Constants.ProcdumpProcessName;

abhishkk · 2018-08-01T05:41:32Z

src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs

+        /// </summary>
+        /// <param name="processId">process id</param>
+        /// <returns>Process Handle</returns>
+        IntPtr GetProcessHandleById(int processId);


Nit: Method name can be simply GetProcessHandle

Updated the name to GetProcessHandle

abhishkk · 2018-08-01T05:42:22Z

src/Microsoft.TestPlatform.PlatformAbstractions/netcore/System/ProcessHelper.cs

+
+        public IntPtr GetProcessHandleById(int processId)
+        {
+            return Process.GetProcessById(processId).SafeHandle.DangerousGetHandle();


What is this DangerousGetHandle? Is it dangerous? :P
Can you write a comment as well?

Added comment for description.

abhishkk · 2018-08-01T05:46:42Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

            }
+
+            procDumpArgument.Append($"{processId} {filename}.dmp");
+            return procDumpArgument.ToString();


Can we output this argument in logs as well (with info level may be)?

Output in logs done

abhishkk

Approved with suggestions.

kaadhina

ProcDump Args Changed

3f228ab

abhishkk reviewed Jul 27, 2018

View reviewed changes

kaadhina reviewed Jul 30, 2018

View reviewed changes

vagisha-nidhi added 2 commits July 31, 2018 10:17

Iswow64 used for 32bit process

3ced475

Conflicts resolved for procdump argument changes

a236fd3

kaadhina reviewed Aug 1, 2018

View reviewed changes

abhishkk reviewed Aug 1, 2018

View reviewed changes

abhishkk approved these changes Aug 1, 2018

View reviewed changes

Minor fixes

65a3263

vagisha-nidhi force-pushed the ChangeInProcdumpOptions branch from e41957f to 65a3263 Compare August 1, 2018 08:48

kaadhina approved these changes Aug 1, 2018

View reviewed changes

vagisha-nidhi added 2 commits August 1, 2018 17:00

Merge branch 'master' into ChangeInProcdumpOptions

5da69bf

Merge branch 'master' into ChangeInProcdumpOptions

6c70874

vagisha-nidhi merged commit 3db5c0d into microsoft:master Aug 2, 2018

vagisha-nidhi deleted the ChangeInProcdumpOptions branch August 10, 2018 06:22

Procdump Arguments Changed #1700

Procdump Arguments Changed #1700

Conversation

vagisha-nidhi commented Jul 26, 2018 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishkk left a comment

Choose a reason for hiding this comment

kaadhina left a comment

Choose a reason for hiding this comment

vagisha-nidhi commented Jul 26, 2018 •

edited

Loading