[PATCH 0/2] ArmPkg: DebugPeCoffExtraActionLib: debugger commands are not errors

[PATCH 0/2] ArmPkg: DebugPeCoffExtraActionLib: debugger commands are not errors

[Philippe Mathieu-Daudé]

Hi,

This series completes the cleanup from commit 1fce963d89f3 ("ArmPkg:
DebugPeCoffExtraActionLib: debugger commands are not errors", from
2015-03-02), on PeCoffLoaderUnloadImageExtraAction().

This solves an issue when building in silent mode with
-D DEBUG_PRINT_ERROR_LEVEL=0x80000000. With the QemuRamFbDxe platform
DXE driver, when we get a driver dispatch failure it is reported as
an error level:

qemu-system-aarch64 -machine virt,gic-version=3,accel=kvm -cpu host \
  -display none -serial stdio -bios /usr/share/AAVMF/AAVMF_CODE.fd
remove-symbol-file /builddir/build/BUILD/edk2-89910a39dcfd/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe/DEBUG/QemuRamfbDxe.dll 0x3F5F6000

The second patch is an obvious typo fix.

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  ArmPkg: DebugPeCoffExtraActionLib: debugger commands are not errors
  ArmPkg: DebugPeCoffExtraActionLib: fix trivial typos

 .../DebugPeCoffExtraActionLib.c                      | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH 1/2] ArmPkg: DebugPeCoffExtraActionLib: debugger commands are not errors

[Philippe Mathieu-Daudé]

In commit 1fce963d89f3e we reduced the level of information printed
by PeCoffLoaderRelocateImageExtraAction() but we did not update the
similar PeCoffLoaderUnloadImageExtraAction() function.

PeCoffLoaderUnloadImageExtraAction() prints helpful debugger commands
for source level debugging. These messages should not be printed on the
EFI_D_ERROR level; they don't report errors. Change the debug level
(bitmask, actually) to DEBUG_LOAD | DEBUG_INFO, because the messages are
printed in relation to image loading, and they are informative.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: Andrew Jones <drjones@redhat.com>
Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 .../DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index a1cb99677fe6..3379744aa185 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -122,14 +122,14 @@ PeCoffLoaderUnloadImageExtraAction (
   if (ImageContext->PdbPointer) {
 #ifdef __CC_ARM
     // Print out the command for the RVD debugger to load symbols for this image
-    DEBUG ((EFI_D_ERROR, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
 #elif __GNUC__
     // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
-    DEBUG ((EFI_D_ERROR, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
 #else
-    DEBUG ((EFI_D_ERROR, "Unloading %a\n", ImageContext->PdbPointer));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Unloading %a\n", ImageContext->PdbPointer));
 #endif
   } else {
-    DEBUG ((EFI_D_ERROR, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress));
+    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress));
   }
 }
-- 
2.20.1

[PATCH 2/2] ArmPkg: DebugPeCoffExtraActionLib: fix trivial typos

[Philippe Mathieu-Daudé]

Fix a pair of trivial typos by inserting a space.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 .../DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c     | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index 3379744aa185..3f88e84372ee 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -87,7 +87,7 @@ PeCoffLoaderRelocateImageExtraAction (
     DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
 #endif
 #elif __GNUC__
-    // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
+    // This may not work correctly if you generate PE/COFF directly as then the Offset would not be required
     DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
 #else
     DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
@@ -124,7 +124,7 @@ PeCoffLoaderUnloadImageExtraAction (
     // Print out the command for the RVD debugger to load symbols for this image
     DEBUG ((DEBUG_LOAD | DEBUG_INFO, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
 #elif __GNUC__
-    // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
+    // This may not work correctly if you generate PE/COFF directly as then the Offset would not be required
     DEBUG ((DEBUG_LOAD | DEBUG_INFO, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
 #else
     DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Unloading %a\n", ImageContext->PdbPointer));
-- 
2.20.1

Re: [PATCH 1/2] ArmPkg: DebugPeCoffExtraActionLib: debugger commands are not errors

[Laszlo Ersek]

On 07/29/19 20:03, Philippe Mathieu-Daude wrote:
> In commit 1fce963d89f3e we reduced the level of information printed
> by PeCoffLoaderRelocateImageExtraAction() but we did not update the
> similar PeCoffLoaderUnloadImageExtraAction() function.
> 
> PeCoffLoaderUnloadImageExtraAction() prints helpful debugger commands
> for source level debugging. These messages should not be printed on the
> EFI_D_ERROR level; they don't report errors. Change the debug level
> (bitmask, actually) to DEBUG_LOAD | DEBUG_INFO, because the messages are
> printed in relation to image loading, and they are informative.
> 
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reported-by: Andrew Jones <drjones@redhat.com>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
> ---
>  .../DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> index a1cb99677fe6..3379744aa185 100644
> --- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> @@ -122,14 +122,14 @@ PeCoffLoaderUnloadImageExtraAction (
>    if (ImageContext->PdbPointer) {
>  #ifdef __CC_ARM
>      // Print out the command for the RVD debugger to load symbols for this image
> -    DEBUG ((EFI_D_ERROR, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
>  #elif __GNUC__
>      // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
> -    DEBUG ((EFI_D_ERROR, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
>  #else
> -    DEBUG ((EFI_D_ERROR, "Unloading %a\n", ImageContext->PdbPointer));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Unloading %a\n", ImageContext->PdbPointer));
>  #endif
>    } else {
> -    DEBUG ((EFI_D_ERROR, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress));
>    }
>  }
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 2/2] ArmPkg: DebugPeCoffExtraActionLib: fix trivial typos

[Laszlo Ersek]

On 07/29/19 20:03, Philippe Mathieu-Daude wrote:
> Fix a pair of trivial typos by inserting a space.
> 
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
> ---
>  .../DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c     | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> index 3379744aa185..3f88e84372ee 100644
> --- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> @@ -87,7 +87,7 @@ PeCoffLoaderRelocateImageExtraAction (
>      DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
>  #endif
>  #elif __GNUC__
> -    // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
> +    // This may not work correctly if you generate PE/COFF directly as then the Offset would not be required
>      DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
>  #else
>      DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
> @@ -124,7 +124,7 @@ PeCoffLoaderUnloadImageExtraAction (
>      // Print out the command for the RVD debugger to load symbols for this image
>      DEBUG ((DEBUG_LOAD | DEBUG_INFO, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
>  #elif __GNUC__
> -    // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
> +    // This may not work correctly if you generate PE/COFF directly as then the Offset would not be required
>      DEBUG ((DEBUG_LOAD | DEBUG_INFO, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
>  #else
>      DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Unloading %a\n", ImageContext->PdbPointer));
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 0/2] ArmPkg: DebugPeCoffExtraActionLib: debugger commands are not errors

[Leif Lindholm]

On Mon, Jul 29, 2019 at 08:03:19PM +0200, Philippe Mathieu-Daude wrote:
> Hi,
> 
> This series completes the cleanup from commit 1fce963d89f3 ("ArmPkg:
> DebugPeCoffExtraActionLib: debugger commands are not errors", from
> 2015-03-02), on PeCoffLoaderUnloadImageExtraAction().
> 
> This solves an issue when building in silent mode with
> -D DEBUG_PRINT_ERROR_LEVEL=0x80000000. With the QemuRamFbDxe platform
> DXE driver, when we get a driver dispatch failure it is reported as
> an error level:
> 
> qemu-system-aarch64 -machine virt,gic-version=3,accel=kvm -cpu host \
>   -display none -serial stdio -bios /usr/share/AAVMF/AAVMF_CODE.fd
> remove-symbol-file /builddir/build/BUILD/edk2-89910a39dcfd/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe/DEBUG/QemuRamfbDxe.dll 0x3F5F6000
> 
> The second patch is an obvious typo fix.

I added the word "comment" in subject/message of 2/2, to make
it explicit that the typo does not affect functionality.

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Pushed as 8fed4e47d9a6..3d34b5f32692.

Thanks!

> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (2):
>   ArmPkg: DebugPeCoffExtraActionLib: debugger commands are not errors
>   ArmPkg: DebugPeCoffExtraActionLib: fix trivial typos
> 
>  .../DebugPeCoffExtraActionLib.c                      | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> -- 
> 2.20.1
> 

Re: [edk2-devel] [PATCH 0/2] ArmPkg: DebugPeCoffExtraActionLib: debugger commands are not errors

[Philippe Mathieu-Daudé]

On 7/30/19 4:01 PM, Leif Lindholm wrote:
> On Mon, Jul 29, 2019 at 08:03:19PM +0200, Philippe Mathieu-Daude wrote:
>> Hi,
>>
>> This series completes the cleanup from commit 1fce963d89f3 ("ArmPkg:
>> DebugPeCoffExtraActionLib: debugger commands are not errors", from
>> 2015-03-02), on PeCoffLoaderUnloadImageExtraAction().
>>
>> This solves an issue when building in silent mode with
>> -D DEBUG_PRINT_ERROR_LEVEL=0x80000000. With the QemuRamFbDxe platform
>> DXE driver, when we get a driver dispatch failure it is reported as
>> an error level:
>>
>> qemu-system-aarch64 -machine virt,gic-version=3,accel=kvm -cpu host \
>>   -display none -serial stdio -bios /usr/share/AAVMF/AAVMF_CODE.fd
>> remove-symbol-file /builddir/build/BUILD/edk2-89910a39dcfd/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/OvmfPkg/QemuRamfbDxe/QemuRamfbDxe/DEBUG/QemuRamfbDxe.dll 0x3F5F6000
>>
>> The second patch is an obvious typo fix.
> 
> I added the word "comment" in subject/message of 2/2, to make
> it explicit that the typo does not affect functionality.

Good idea, thanks.

> 
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> Pushed as 8fed4e47d9a6..3d34b5f32692.

Thanks Laszlo and Leif!

> 
> Thanks!
> 
>> Regards,
>>
>> Phil.
>>
>> Philippe Mathieu-Daudé (2):
>>   ArmPkg: DebugPeCoffExtraActionLib: debugger commands are not errors
>>   ArmPkg: DebugPeCoffExtraActionLib: fix trivial typos
>>
>>  .../DebugPeCoffExtraActionLib.c                      | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> -- 
>> 2.20.1
>>
> 
> 
>