Hi Nate, Reviewed-by: Michael D Kinney <michael.d.kin...@intel.com>
That makes complete sense now on the use case when building drivers or apps outside EmulatorPkg DSC file and wanting to support source level debug. In the past, I have had to add components to EmulatorPkg DSC file. Thank you for adding feature to support source level debug of components built outside EmulatorPkg. Thanks, Mike > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com> > Sent: Tuesday, September 26, 2023 2:55 PM > To: Kinney, Michael D <michael.d.kin...@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <af...@apple.com>; Ni, Ray <ray...@intel.com>; Chiu, > Chasel <chasel.c...@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Source Level Debug on Windows > > Hi Mike, > > Yes, we can delete those lines now 😊. I have those lines commented > out in my copy of EmulatorPkg. I found this because I have been trying > to use EmulatorPkg to source level debug drivers that I built > separately via one of the edk2-platform builds. > > Thanks, > Nate > > -----Original Message----- > From: Kinney, Michael D <michael.d.kin...@intel.com> > Sent: Tuesday, September 26, 2023 2:29 PM > To: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; > devel@edk2.groups.io > Cc: Andrew Fish <af...@apple.com>; Ni, Ray <ray...@intel.com>; Chiu, > Chasel <chasel.c...@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com> > Subject: RE: [PATCH v1] EmulatorPkg: Fix Source Level Debug on Windows > > I have VS Code on Windows working today using the PDB based symbol > file. > > The following lines in the EmulatorPkg DSC file force 4KB alignment > and the DLL export of the InitializeDriver symbol > > https://github.com/tianocore/edk2/blob/bf0bdacdd6f6cdd2e9ac5db14b6daf1 > 9a5a5bd57/EmulatorPkg/EmulatorPkg.dsc#L497 > > MSFT:*_*_*_DLINK_FLAGS = /ALIGN:4096 /FILEALIGN:4096 > /SUBSYSTEM:CONSOLE > MSFT:DEBUG_*_*_DLINK_FLAGS = > /EXPORT:InitializeDriver=$(IMAGE_ENTRY_POINT) /BASE:0x10000 > MSFT:NOOPT_*_*_DLINK_FLAGS = > /EXPORT:InitializeDriver=$(IMAGE_ENTRY_POINT) /BASE:0x10000 > > > I think this is why it is working for me without this change. > > Are you suggesting that with this change these can be removed? > > Mike > > > -----Original Message----- > > From: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com> > > Sent: Tuesday, September 26, 2023 1:32 PM > > To: Kinney, Michael D <michael.d.kin...@intel.com>; > > devel@edk2.groups.io > > Cc: Andrew Fish <af...@apple.com>; Ni, Ray <ray...@intel.com>; Chiu, > > Chasel <chasel.c...@intel.com> > > Subject: RE: [PATCH v1] EmulatorPkg: Fix Source Level Debug on > Windows > > > > Hi Mike, > > > > Source level debug with VS Code does indeed work today with gdb or > > lldb. This change makes the Visual Studio Windows debugger work as > > well. > > > > You are correct that if the same DLL is loaded more than once that > > this method cannot perform source level debug on the second > instance; > > but this change won't break that scenario either. If the same DLL > > occurs twice, then we will use the PE/COFF image loaded by either > the > > PEI core or DXE core instead of the one loaded by Windows. This > means > > that the second instance of the DLL will not be source level debug- > > able by Visual Studio; but PI-spec compliance is maintained. This > > behavior is unchanged from the original code. > > > > Yes, this code enables PE/COFF images that do not have sections that > > are 4KB aligned. It will setup page protection for user mode. > Without > > this change you must turn off the NX bit when using the Visual > Studio > > Windows debugger. > > > > Thanks, > > Nate > > > > -----Original Message----- > > From: Kinney, Michael D <michael.d.kin...@intel.com> > > Sent: Tuesday, September 26, 2023 1:08 PM > > To: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; > > devel@edk2.groups.io > > Cc: Andrew Fish <af...@apple.com>; Ni, Ray <ray...@intel.com>; Chiu, > > Chasel <chasel.c...@intel.com>; Kinney, Michael D > > <michael.d.kin...@intel.com> > > Subject: RE: [PATCH v1] EmulatorPkg: Fix Source Level Debug on > Windows > > > > Hi Nate, > > > > I am able to do source level debug of EmulatorPkg using VS Code > today. > > > > What scenarios are broken? > > > > I do know that the DLL based approach would only allow a single > > instance of the module to be loaded and debugged. If, for example, > a > > driver is loaded more than once from the UEFI Shell in the > EmulatorPkg > > env, the 2nd driver would use the first DLL which does not match the > > PI spec behavior. > > > > It also appears that this change can support PE/COFF images that do > > not have sections that are 4KB aligned and handles the page > protection > > settings for the user mode application env. Is that correct? > > > > Mike > > > > > -----Original Message----- > > > From: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com> > > > Sent: Friday, September 22, 2023 3:49 PM > > > To: devel@edk2.groups.io > > > Cc: Andrew Fish <af...@apple.com>; Ni, Ray <ray...@intel.com>; > > Kinney, > > > Michael D <michael.d.kin...@intel.com>; Chiu, Chasel > > > <chasel.c...@intel.com> > > > Subject: [PATCH v1] EmulatorPkg: Fix Source Level Debug on Windows > > > > > > The Visual Studio Windows debugger will only load symbols for > > PE/COFF > > > images that Windows is aware of. Therefore, to enable source level > > > debugging, all PEI/DXE modules must be loaded via LoadLibrary() or > > > LoadLibraryEx() and the the instance in memory created by > > > LoadLibrary() must be the one that is actually executed. > > > > > > The current source level debug implementation in EmulatorPkg for > > > Windows is inherited from the old Nt32Pkg. This implementation > makes > > > the assumption that all PEI/DXE modules have a DLL export tables > > with > > > a symbol named InitializeDriver. Therefore, this source level > debug > > > implementation requires all modules to be linked in a non-PI spec > > > defined manner. Support for adding the InitializeDriver symbol was > > > removed in EmulatorPkg, which broke source level debugging. > > > > > > To fix this, the source level debugging implementation has been > > > modified to use the PE/COFF entry point directly. This brings the > > > implementation into compliance with the PI spec and should work > with > > > any PEIM/DXE driver. > > > Implementing this requires parsing the in-memory instance of the > > > PE/COFF image created by Windows to find the entrypoint and since > > > PEIMs/DXE drivers are not garunteed to have 4KB aligned sections, > it > > > also requires explicit configuration of the page table using > > > VirtualProtect(). > > > > > > With this fix, the debugging experience is now so good it is > > > unprecedented! > > > In Visual Studio Code, add the following to launch.json: > > > > > > { > > > "version": "0.2.0", > > > "configurations": [ > > > { > > > "name": "EmulatorPkg Launch", > > > "type": "cppvsdbg", > > > "request": "launch", > > > "program": > > > > > > "${workspaceFolder}/<path_to_build>/Build/EmulatorX64/DEBUG_<tool_chai > > > n>/X64/WinHost", > > > "args": [], > > > "stopAtEntry": false, > > > "cwd": > > > > > > "${workspaceFolder}/<path_to_build>/Build/EmulatorX64/DEBUG_<tool_chai > > > n>/X64/", > > > "environment": [], > > > "console": false, > > > } > > > ] > > > } > > > > > > Make modifications to the above template as nessesary and build > > > EmulatorPkg. > > > Now, just add breakpoints directly in Visual Studio Code the way > you > > > would with any other software project. When you start the > debugger, > > it > > > will halt at the breakpoint automatically without any extra > > > configuration required. > > > > > > Cc: Andrew Fish <af...@apple.com> > > > Cc: Ray Ni <ray...@intel.com> > > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > > Cc: Chasel Chiu <chasel.c...@intel.com> > > > Signed-off-by: Nate DeSimone <nathaniel.l.desim...@intel.com> > > > --- > > > EmulatorPkg/Win/Host/WinHost.c | 206 > +++++++++++++++++++++++++++++- > > -- > > > - > > > 1 file changed, 182 insertions(+), 24 deletions(-) > > > > > > diff --git a/EmulatorPkg/Win/Host/WinHost.c > > > b/EmulatorPkg/Win/Host/WinHost.c index 193a947fbd..e414da6c55 > 100644 > > > --- a/EmulatorPkg/Win/Host/WinHost.c > > > +++ b/EmulatorPkg/Win/Host/WinHost.c > > > @@ -8,7 +8,7 @@ > > > This code produces 128 K of temporary memory for the SEC stack > by > > > directly > > > allocate memory space with ReadWrite and Execute attribute. > > > > > > -Copyright (c) 2006 - 2022, Intel Corporation. All rights > > > reserved.<BR> > > > +Copyright (c) 2006 - 2023, Intel Corporation. All rights > > > reserved.<BR> > > > (C) Copyright 2016-2020 Hewlett Packard Enterprise Development > > LP<BR> > > > SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -977,7 > +977,7 > > @@ > > > AddModHandle ( > > > for (Index = 0; Index < mPdbNameModHandleArraySize; Index++, > > > Array++) { > > > if (Array->PdbPointer == NULL) { > > > // > > > - // Make a copy of the stirng and store the ModHandle > > > + // Make a copy of the string and store the ModHandle > > > // > > > Handle = GetProcessHeap (); > > > Size = AsciiStrLen (ImageContext->PdbPointer) > + > > 1; > > > @@ -1056,26 +1056,45 @@ RemoveModHandle ( > > > return NULL; > > > } > > > > > > +typedef struct { > > > + UINTN Base; > > > + UINT32 Size; > > > + UINT32 Flags; > > > +} IMAGE_SECTION_DATA; > > > + > > > VOID > > > EFIAPI > > > PeCoffLoaderRelocateImageExtraAction ( > > > IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext > > > ) > > > { > > > - EFI_STATUS Status; > > > - VOID *DllEntryPoint; > > > - CHAR16 *DllFileName; > > > - HMODULE Library; > > > - UINTN Index; > > > + EFI_STATUS Status; > > > + VOID *DllEntryPoint; > > > + CHAR16 *DllFileName; > > > + HMODULE Library; > > > + UINTN Index; > > > + PE_COFF_LOADER_IMAGE_CONTEXT PeCoffImageContext; > > > + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr; > > > + EFI_IMAGE_SECTION_HEADER *FirstSection; > > > + EFI_IMAGE_SECTION_HEADER *Section; > > > + IMAGE_SECTION_DATA *SectionData; > > > + UINTN NumberOfSections; > > > + UINTN Base; > > > + UINTN End; > > > + UINTN RegionBase; > > > + UINTN RegionSize; > > > + UINT32 Flags; > > > + DWORD NewProtection; > > > + DWORD OldProtection; > > > > > > ASSERT (ImageContext != NULL); > > > // > > > - // If we load our own PE COFF images the Windows debugger can > not > > > source > > > - // level debug our code. If a valid PDB pointer exists use it > to > > > load > > > - // the *.dll file as a library using Windows* APIs. This > allows > > > - // source level debug. The image is still loaded and relocated > > > - // in the Framework memory space like on a real system (by the > > > code above), > > > - // but the entry point points into the DLL loaded by the code > > > below. > > > + // If we load our own PE/COFF images the Windows debugger can > not > > > source > > > + // level debug our code. If a valid PDB pointer exists use it > to > > > load > > > + // the *.dll file as a library using Windows* APIs. This allows > > // > > > + source level debug. The image is still loaded and relocated // > in > > > + the Framework memory space like on a real system (by the code > > > above), > > > + // but the entry point points into the DLL loaded by the code > > > below. > > > // > > > > > > DllEntryPoint = NULL; > > > @@ -1106,27 +1125,166 @@ PeCoffLoaderRelocateImageExtraAction ( > > > } > > > > > > // > > > - // Replace .PDB with .DLL on the filename > > > + // Replace .PDB with .DLL in the filename > > > // > > > DllFileName[Index - 3] = 'D'; > > > DllFileName[Index - 2] = 'L'; > > > DllFileName[Index - 1] = 'L'; > > > > > > // > > > - // Load the .DLL file into the user process's address space > for > > > source > > > - // level debug > > > + // Load the .DLL file into the process's address space for > > source > > > level > > > + // debug. > > > + // > > > + // EFI modules use the PE32 entry point for a different > purpose > > > than > > > + // Windows. For Windows DLLs, the PE entry point is used for > > the > > > DllMain() > > > + // function. DllMain() has a very specific purpose; it > > > initializes runtime > > > + // libraries, instance data, and thread local storage. > > > LoadLibrary()/ > > > + // LoadLibraryEx() will run the PE32 entry point and assume > it > > to > > > be a > > > + // DllMain() implementation by default. By passing the > > > + // DONT_RESOLVE_DLL_REFERENCES argument to LoadLibraryEx(), > the > > > execution > > > + // of the entry point as a DllMain() function will be > > suppressed. > > > This > > > + // also prevents other modules that are referenced by the DLL > > > from being > > > + // loaded. We use LoadLibraryEx() to create a copy of the > PE32 > > > + // image that the OS (and therefore the debugger) is aware > of. > > > + // Source level debugging is the only reason to do this. > > > // > > > Library = LoadLibraryEx (DllFileName, NULL, > > > DONT_RESOLVE_DLL_REFERENCES); > > > if (Library != NULL) { > > > // > > > - // InitializeDriver is the entry point we put in all our > EFI > > > DLL's. The > > > - // DONT_RESOLVE_DLL_REFERENCES argument to LoadLIbraryEx() > > > suppresses the > > > - // normal DLL entry point of DllMain, and prevents other > > > modules that are > > > - // referenced in side the DllFileName from being loaded. > > There > > > is no error > > > - // checking as the we can point to the PE32 image loaded by > > > Tiano. This > > > - // step is only needed for source level debugging > > > + // Parse the PE32 image loaded by the OS and find the entry > > > point > > > // > > > - DllEntryPoint = (VOID *)(UINTN)GetProcAddress (Library, > > > "InitializeDriver"); > > > + ZeroMem (&PeCoffImageContext, sizeof (PeCoffImageContext)); > > > + PeCoffImageContext.Handle = Library; > > > + PeCoffImageContext.ImageRead = > > PeCoffLoaderImageReadFromMemory; > > > + Status = PeCoffLoaderGetImageInfo (&PeCoffImageContext); > > > + if (EFI_ERROR (Status) || (PeCoffImageContext.ImageError != > > > IMAGE_ERROR_SUCCESS)) { > > > + SecPrint ("DLL is not a valid PE/COFF image.\n\r"); > > > + FreeLibrary (Library); > > > + Library = NULL; > > > + } else { > > > + Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)Library + > > > (UINTN)PeCoffImageContext.PeCoffHeaderOffset); > > > + if (Hdr.Pe32->OptionalHeader.Magic == > > > EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { > > > + // > > > + // Use PE32 offset > > > + // > > > + DllEntryPoint = (VOID *) ((UINTN)Library + > > (UINTN)Hdr.Pe32- > > > >OptionalHeader.AddressOfEntryPoint); > > > + } else { > > > + // > > > + // Use PE32+ offset > > > + // > > > + DllEntryPoint = (VOID *) ((UINTN)Library + > > > (UINTN)Hdr.Pe32Plus->OptionalHeader.AddressOfEntryPoint); > > > + } > > > + // > > > + // Now we need to configure memory access for the copy of > > the > > > PE32 image > > > + // loaded by the OS. > > > + // > > > + // Most Windows DLLs are linked with sections 4KB aligned > > but > > > EFI > > > + // modules are not to reduce size. Because of this we > need > > to > > > compute > > > + // the union of memory access attributes and explicitly > > > configure > > > + // each page. > > > + // > > > + FirstSection = (EFI_IMAGE_SECTION_HEADER *)( > > > + (UINTN)Library + > > > + > > > PeCoffImageContext.PeCoffHeaderOffset + > > > + sizeof (UINT32) + > > > + sizeof > > > (EFI_IMAGE_FILE_HEADER) + > > > + Hdr.Pe32- > > > >FileHeader.SizeOfOptionalHeader > > > + ); > > > + NumberOfSections = (UINTN)(Hdr.Pe32- > > > >FileHeader.NumberOfSections); > > > + Section = FirstSection; > > > + SectionData = malloc (NumberOfSections * sizeof > > > (IMAGE_SECTION_DATA)); > > > + if (SectionData == NULL) { > > > + FreeLibrary (Library); > > > + Library = NULL; > > > + DllEntryPoint = NULL; > > > + } > > > + ZeroMem (SectionData, NumberOfSections * sizeof > > > (IMAGE_SECTION_DATA)); > > > + // > > > + // Extract the section data from the PE32 image > > > + // > > > + for (Index = 0; Index < NumberOfSections; Index++) { > > > + SectionData[Index].Base = (UINTN)Library + Section- > > > >VirtualAddress; > > > + SectionData[Index].Size = Section->Misc.VirtualSize; > > > + if (SectionData[Index].Size == 0) { > > > + SectionData[Index].Size = Section->SizeOfRawData; > > > + } > > > + SectionData[Index].Flags = (Section->Characteristics & > > > + (EFI_IMAGE_SCN_MEM_EXECUTE > | > > > EFI_IMAGE_SCN_MEM_WRITE)); > > > + Section += 1; > > > + } > > > + // > > > + // Loop over every DWORD in memory and compute the union > of > > > the memory > > > + // access bits. > > > + // > > > + End = (UINTN)Library + > (UINTN)PeCoffImageContext.ImageSize; > > > + RegionBase = (UINTN)Library; > > > + RegionSize = 0; > > > + Flags = 0; > > > + for (Base = (UINTN)Library + sizeof (UINT32); Base < End; > > > Base += sizeof (UINT32)) { > > > + for (Index = 0; Index < NumberOfSections; Index++) { > > > + if (SectionData[Index].Base <= Base && > > > + (SectionData[Index].Base + > SectionData[Index].Size) > > > > > > Base) { > > > + Flags |= SectionData[Index].Flags; > > > + } > > > + } > > > + // > > > + // When a new page is reached configure the memory > access > > > for the > > > + // previous page. > > > + // > > > + if (Base % SIZE_4KB == 0) { > > > + RegionSize += SIZE_4KB; > > > + if ((Flags & EFI_IMAGE_SCN_MEM_WRITE) == > > > EFI_IMAGE_SCN_MEM_WRITE) { > > > + if ((Flags & EFI_IMAGE_SCN_MEM_EXECUTE) == > > > EFI_IMAGE_SCN_MEM_EXECUTE) { > > > + NewProtection = PAGE_EXECUTE_READWRITE; > > > + } else { > > > + NewProtection = PAGE_READWRITE; > > > + } > > > + } else { > > > + if ((Flags & EFI_IMAGE_SCN_MEM_EXECUTE) == > > > EFI_IMAGE_SCN_MEM_EXECUTE) { > > > + NewProtection = PAGE_EXECUTE_READ; > > > + } else { > > > + NewProtection = PAGE_READONLY; > > > + } > > > + } > > > + if (!VirtualProtect ((LPVOID)RegionBase, (SIZE_T) > > > RegionSize, NewProtection, &OldProtection)) { > > > + SecPrint ("Setting PE32 Section Access > Failed\n\r"); > > > + FreeLibrary (Library); > > > + free (SectionData); > > > + Library = NULL; > > > + DllEntryPoint = NULL; > > > + break; > > > + } > > > + Flags = 0; > > > + RegionBase = Base; > > > + RegionSize = 0; > > > + } > > > + } > > > + free (SectionData); > > > + // > > > + // Configure the last partial page > > > + // > > > + if (Library != NULL && (End - RegionBase) > 0) { > > > + if ((Flags & EFI_IMAGE_SCN_MEM_WRITE) == > > > EFI_IMAGE_SCN_MEM_WRITE) { > > > + if ((Flags & EFI_IMAGE_SCN_MEM_EXECUTE) == > > > EFI_IMAGE_SCN_MEM_EXECUTE) { > > > + NewProtection = PAGE_EXECUTE_READWRITE; > > > + } else { > > > + NewProtection = PAGE_READWRITE; > > > + } > > > + } else { > > > + if ((Flags & EFI_IMAGE_SCN_MEM_EXECUTE) == > > > EFI_IMAGE_SCN_MEM_EXECUTE) { > > > + NewProtection = PAGE_EXECUTE_READ; > > > + } else { > > > + NewProtection = PAGE_READONLY; > > > + } > > > + } > > > + if (!VirtualProtect ((LPVOID)RegionBase, (SIZE_T) (End > - > > > RegionBase), NewProtection, &OldProtection)) { > > > + SecPrint ("Setting PE32 Section Access Failed\n\r"); > > > + FreeLibrary (Library); > > > + Library = NULL; > > > + DllEntryPoint = NULL; > > > + } > > > + } > > > + } > > > } > > > > > > if ((Library != NULL) && (DllEntryPoint != NULL)) { @@ - > 1142,7 > > > +1300,7 @@ PeCoffLoaderRelocateImageExtraAction ( > > > // This DLL is not already loaded, so source level > > debugging > > > is supported. > > > // > > > ImageContext->EntryPoint = > > > (EFI_PHYSICAL_ADDRESS)(UINTN)DllEntryPoint; > > > - SecPrint ("LoadLibraryEx (\n\r %S,\n\r NULL, > > > DONT_RESOLVE_DLL_REFERENCES)\n\r", DllFileName); > > > + SecPrint ("LoadLibraryEx (\n\r %S,\n\r NULL, > > > DONT_RESOLVE_DLL_REFERENCES) @ 0x%X\n\r", DllFileName, (int) > (UINTN) > > > Library); > > > } > > > } else { > > > SecPrint ("WARNING: No source level debug %S. \n\r", > > > DllFileName); > > > -- > > > 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109096): https://edk2.groups.io/g/devel/message/109096 Mute This Topic: https://groups.io/mt/101531560/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
