Re: [edk2-devel] [[PATCH 1/1]] EmulatorPkg using CpuBreakPoint

["Kilian Kegel" <KILIAN_KEGEL@OUTLOOK.COM>]

[-- Attachment #1.1: Type: text/plain, Size: 12321 bytes --]

Ray,

thank you for your patience.

Kilian

diff --git a/EmulatorPkg/Win/Host/WinHost.c b/EmulatorPkg/Win/Host/WinHost.c
index 62a89f7617..b47a1fa8dd 100644
--- a/EmulatorPkg/Win/Host/WinHost.c
+++ b/EmulatorPkg/Win/Host/WinHost.c
@@ -394,6 +394,7 @@ Returns:
   UINTN                 ProcessAffinityMask;
   UINTN                 SystemAffinityMask;
   INT32                 LowBit;
+  INT32                 index;
   //
   // Enable the privilege so that RTC driver can successfully run SetTime()
@@ -410,6 +411,24 @@ Returns:
   SecPrint ("\n\rEDK II WIN Host Emulation Environment from http://www.tianocore.org/edk2/\n\r");
+  SecPrint ("######################################################################################################\n");
+  SecPrint ("add \"/debug\" command line switch, to connect to the debugger at the very beginning of POST emulation\n");
+  SecPrint ("######################################################################################################\n");
+  for (index = 1; index < Argc; index++) {
+    if (0 == strcmp("/debug", Argv[index])) {
+        SecPrint("\t1. start your debug engine\n");
+        SecPrint("\t2. attach to process WinHost.exe\n");
+        SecPrint("\t4. set software breakpoints as needed in your debug session\n");
+        SecPrint("\t5. and press ENTER in the WinHost command box\n");
+        SecPrint("\t6. go back to the debug engine and RUN/SINGLE STEP the application\n");
+        SecPrint("\t7. otherwise press ENTER to continue...\n");
+
+        getchar();      //wait for keyboard input
+
+        SecPrint("");   //now you can single step the entire boot/emulation process, good luck...
+    }
+  }
+
   //
   // Determine the first thread available to this process.
   //
From: Ni, Ray<mailto:ray.ni@intel.com>
Sent: Thursday, November 14, 2019 07:57 AM
To: Kilian Kegel<mailto:kilian_kegel@outlook.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Liming<mailto:liming.gao@intel.com>
Cc: Richardson, Brian<mailto:brian.richardson@intel.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [[PATCH 1/1]] EmulatorPkg using CpuBreakPoint

Kilian,
Thank you very much. Comments below.


From: Kilian Kegel <kilian_kegel@outlook.com>
Sent: Thursday, November 14, 2019 1:24 PM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
Cc: Richardson, Brian <brian.richardson@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [[PATCH 1/1]] EmulatorPkg using CpuBreakPoint

I have no preference at all here. We can also consider to use a stricmp() to be case-in-sensitive…
[Ray] I am ok to either case sensitive or insensitive.

Thanks,
Kilian

diff --git a/EmulatorPkg/Win/Host/WinHost.c b/EmulatorPkg/Win/Host/WinHost.c
index 62a89f7617..0eeb5979ff 100644
--- a/EmulatorPkg/Win/Host/WinHost.c
+++ b/EmulatorPkg/Win/Host/WinHost.c
@@ -394,6 +394,7 @@ Returns:
   UINTN                 ProcessAffinityMask;
   UINTN                 SystemAffinityMask;
   INT32                 LowBit;
+  INT32                 i;
[Ray] 1. Can you please rename it to “Index” to align the coding style?

   //
   // Enable the privilege so that RTC driver can successfully run SetTime()
@@ -410,6 +411,26 @@ Returns:
   SecPrint ("\n\rEDK II WIN Host Emulation Environment from http://www.tianocore.org/edk2/\n\r<http://www.tianocore.org/edk2/n/r>");
+  SecPrint ("######################################################################################################\n");
+  SecPrint ("add \"/debug\" command line switch, to connect to the debugger at the very beginning of POST emulation\n");
+  SecPrint ("######################################################################################################\n");
+  if (Argc > 1) {
[Ray] 2. Can you remove the if check? It’s unnecessary because of below for-loop.

+    for (i = 1; i < Argc; i++) {
+      if (0 == strcmp("/debug", Argv[i])) {
+          SecPrint("\t1. start your debug engine\n");
+          SecPrint("\t2. attach to process WinHost.exe\n");

[Ray] 3. I don’t think it’s proper to hardcode line number in code because that will cause this debug message is changed in future.
Can you remove the line number?

+          SecPrint("\t4. SET A SOFTWARE BREAKPOINT (F9) in line 431\n");
+          SecPrint("\t5. and press ENTER in the WinHost command box\n");
+          SecPrint("\t6. go back to the debug engine and RUN/SINGLE STEP the application\n");
+          SecPrint("\t7. otherwise press enter to continue...\n");
+
+          getchar();    //wait for keyboard input
+
+          SecPrint("");//now you can single step the entire boot/emulation process, good luck...
+      }
+    }
+  }
+
   //
   // Determine the first thread available to this process.
   //
From: Ni, Ray<mailto:ray.ni@intel.com>
Sent: Wednesday, November 13, 2019 03:27 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Liming<mailto:liming.gao@intel.com>; KILIAN_KEGEL@OUTLOOK.COM<mailto:KILIAN_KEGEL@OUTLOOK.COM>
Cc: Richardson, Brian<mailto:brian.richardson@intel.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [[PATCH 1/1]] EmulatorPkg using CpuBreakPoint

Kilian,
Thank you very much for enabling the better VS debugging experience.

Minor comments below.

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Liming Gao
Sent: Wednesday, November 13, 2019 9:07 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; KILIAN_KEGEL@OUTLOOK.COM<mailto:KILIAN_KEGEL@OUTLOOK.COM>
Cc: Richardson, Brian <brian.richardson@intel.com<mailto:brian.richardson@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
Subject: Re: [edk2-devel] [[PATCH 1/1]] EmulatorPkg using CpuBreakPoint

Kilian:
  Can you create the patch with the commit message?

  For the patch, I add my comments.


From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Kilian Kegel
Sent: Wednesday, November 06, 2019 1:36 AM
To: Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Richardson, Brian <brian.richardson@intel.com<mailto:brian.richardson@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Subject: [edk2-devel] [[PATCH 1/1]] EmulatorPkg using CpuBreakPoint

Hi Liming,
>Kilian: can you send the patch to edk2 mail list for code review?
Regards,
Kilian

https://bugzilla.tianocore.org/show_bug.cgi?id=2319

diff --git a/EmulatorPkg/Win/Host/WinHost.c b/EmulatorPkg/Win/Host/WinHost.c
index 62a89f7617..53cda86f9a 100644
--- a/EmulatorPkg/Win/Host/WinHost.c
+++ b/EmulatorPkg/Win/Host/WinHost.c
@@ -409,7 +409,30 @@ Returns:
   FirmwareVolumesStr = (CHAR16 *) PcdGetPtr (PcdEmuFirmwareVolume);
   SecPrint ("\n\rEDK II WIN Host Emulation Environment from http://www.tianocore.org/edk2/\n\r<http://www.tianocore.org/edk2/n/r>");
-
+
+  if (1) {
Please use TRUE instead of 1
[Ray] 1. Can you remove “if(1)”?

+    int i;
Please define it in the begin of the function, and use edk2 data type and coding style, such as UINTN Index.

+    SecPrint ("######################################################################################################\n");
+    SecPrint ("add \"/debug\" command line switch, to connect to the debugger at the very beginning of POST emulation\n");
+    SecPrint ("######################################################################################################\n");
+    if (Argc > 1) {
+      for (i = 1; i < Argc; i++) {
+        if (0 == strcmp("/debug", Argv[i])) {
How about use strncmp?
[Ray] 2. strncmp assumes user may input “/debugxxxxx”. Is that a valid assumption? I thought we only accept “/debug” the whole word as input parameter to trigger debugging.

Thanks
Liming
+          //SecPrint("IF YOU WANT TO DEBUG from the very beginning of the EMULATION:\n\t1. start the TASKMGR\n\t2. connect WinHost.exe to the debugger\n\t3. and press ENTER in this command box\nOR\n");
[Ray] 3. I prefer to break the long string to multiple lines of SecPrint() call as what the code does in below.

+          SecPrint("\t1. start your debug engine\n");
+          SecPrint("\t2. attache to process WinHost.exe\n");
+          SecPrint("\t4. SET A SOFTWARE BREAKPOINT (F9) in line 431\n");
+          SecPrint("\t5. and press ENTER in the WinHost command box\n");
+          SecPrint("\t6. go back to the debug engine and RUN/SINGLE STEP the application\n");
+          SecPrint("\t7. otherwise press enter to continue...\n");
+
+          getchar();    //wait for keyboard input
+
+          SecPrint("");//now you can single step the entire boot/emulation process, good luck...
+        }
+      }
+    }
+  }
   //
   // Determine the first thread available to this process.
   //


From: Kilian Kegel <kilian_kegel@outlook.com<mailto:kilian_kegel@outlook.com>>
Sent: Monday, October 28, 2019 2:01:23 PM
To: liming.gao@intel.com<mailto:liming.gao@intel.com> <liming.gao@intel.com<mailto:liming.gao@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Richardson, Brian <brian.richardson@intel.com<mailto:brian.richardson@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Subject: [edk2-devel] [edk2] [EmulatorPkg] using __debugbreak()


Hi Liming,



If have observed in newer Windows 10 versions, when using __debugbreak()in any

application that Windows just terminates the app, instead offering to debug it.

So in WinHost.exe too.



That’s why I usually insert the code snippet below to run into getchar()

when the program was started with the /debug command line switch.

As long as the App waits for the next keystroke, I can start the debugger (VS2019)

and connect to WinHost.exe process for debugging.



Do you have a better solution without modifying the source code?



Thanks,

Kilian

    if(1){

        int i;



        SecPrint ("######################################################################################################\n");

        SecPrint ("add \"/debug\" command line switch, to connect to the debugger at the very beginning of POST emulation\n");

        SecPrint ("######################################################################################################\n");



        if(Argc > 1){

            for(i = 1 ; i < Argc ; i++){

                if(0 == strcmp("/debug", Argv[i])){

                        //SecPrint("IF YOU WANT TO DEBUG from the very beginning of the EMULATION:\n\t1. start the TASKMGR\n\t2. connect WinHost.exe to the debugger\n\t3. and press ENTER in this command box\nOR\n");

                        SecPrint("\t1. start Visual Studio\n");

                        SecPrint("\t2. DEBUG->ATTACH TO PROCESS (CTRL + ALT + P) --> WinHost.exe\n");

                        SecPrint("\t3. Break All  (CTRL + ALT + Break) --> WinHost.exe\n");

                        SecPrint("\t4. SET A SOFTWARE BREAKPOINT (F9) in line 445\n");

                        SecPrint("\t5. and press ENTER in this command box\n");

                        SecPrint("\t6. go back to Visual Studio an RUN/SINGLE STEP the application\n");

                        SecPrint("\t7. otherwise press enter to continue...\n");

                        getchar();

                        //

                        // 1. SET A SOFTWARE BREAKPOINT TO THE NEXT LINE of code below -->> SecPrint("");

                        // 2. switch to the command box and press ENTER

                        // 3. start single stepping the entire boot/emulation process, good luck...

                        //

                        SecPrint("");//now you can single step the entire boot/emulation process, good luck...

                   }

            }

        }

    }




[-- Attachment #1.2: Type: text/html, Size: 29865 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 144 bytes --]

[-- Attachment #3: image002.png --]
[-- Type: image/png, Size: 144 bytes --]