[PATCH v2] OvmfPkg: strip build paths in release builds

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH v2] OvmfPkg: strip build paths in release builds
@ 2021-03-24 11:58 Ross Burton
  2021-03-24 22:23 ` [edk2-devel] " Laszlo Ersek
  2021-03-24 23:25 ` Andrew Fish
  0 siblings, 2 replies; 9+ messages in thread
From: Ross Burton @ 2021-03-24 11:58 UTC (permalink / raw)
  To: devel; +Cc: lersek

GenFw will embed a NB10 section which contains the path to the input file,
which means the output files have build paths embedded in them.  To reduce
information leakage and ensure reproducible builds, pass --zero in release
builds to remove this information.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3256
Signed-off-by: Ross Burton <ross.burton@arm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
 OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
 OvmfPkg/OvmfPkgIa32.dsc      | 1 +
 OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
 OvmfPkg/OvmfPkgX64.dsc       | 1 +
 OvmfPkg/OvmfXen.dsc          | 1 +
 6 files changed, 6 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 65c42284d9..69a05feea9 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -78,6 +78,7 @@
   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
 !endif
+  RELEASE_*_*_GENFW_FLAGS = --zero
 
   #
   # Disable deprecated APIs.
diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index 4a1cdf5aca..132f55cf69 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -76,6 +76,7 @@
   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
 !endif
+  RELEASE_*_*_GENFW_FLAGS = --zero
 
   #
   # Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 1eaf3e99c6..93c209950c 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -80,6 +80,7 @@
 !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(TOOL_CHAIN_TAG) != "CLANGPDB"
   GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
 !endif
+  RELEASE_*_*_GENFW_FLAGS = --zero
 
   #
   # Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 4a5a430147..97cc438250 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -84,6 +84,7 @@
   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
 !endif
+  RELEASE_*_*_GENFW_FLAGS = --zero
 
   #
   # Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index d4d601b444..f544fb04bf 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -84,6 +84,7 @@
   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
 !endif
+  RELEASE_*_*_GENFW_FLAGS = --zero
 
   #
   # Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 507029404f..fcaa35acf1 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -74,6 +74,7 @@
   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
 !endif
+  RELEASE_*_*_GENFW_FLAGS = --zero
 
   #
   # Disable deprecated APIs.
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
  2021-03-24 11:58 [PATCH v2] OvmfPkg: strip build paths in release builds Ross Burton
@ 2021-03-24 22:23 ` Laszlo Ersek
  2021-03-24 23:25 ` Andrew Fish
  1 sibling, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2021-03-24 22:23 UTC (permalink / raw)
  To: devel, ross

On 03/24/21 12:58, Ross Burton wrote:
> GenFw will embed a NB10 section which contains the path to the input file,
> which means the output files have build paths embedded in them.  To reduce
> information leakage and ensure reproducible builds, pass --zero in release
> builds to remove this information.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3256
> Signed-off-by: Ross Burton <ross.burton@arm.com>
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
>  OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
>  OvmfPkg/OvmfPkgIa32.dsc      | 1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
>  OvmfPkg/OvmfPkgX64.dsc       | 1 +
>  OvmfPkg/OvmfXen.dsc          | 1 +
>  6 files changed, 6 insertions(+)

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

Merged as commit f037af6ecbc3, via
<https://github.com/tianocore/edk2/pull/1513>.

Thanks,
Laszlo


> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index 65c42284d9..69a05feea9 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -78,6 +78,7 @@
>    GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>    INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>  !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
>  
>    #
>    # Disable deprecated APIs.
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index 4a1cdf5aca..132f55cf69 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -76,6 +76,7 @@
>    GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>    INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>  !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
>  
>    #
>    # Disable deprecated APIs.
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 1eaf3e99c6..93c209950c 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -80,6 +80,7 @@
>  !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(TOOL_CHAIN_TAG) != "CLANGPDB"
>    GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
>  !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
>  
>    #
>    # Disable deprecated APIs.
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 4a5a430147..97cc438250 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -84,6 +84,7 @@
>    GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>    INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>  !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
>  
>    #
>    # Disable deprecated APIs.
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index d4d601b444..f544fb04bf 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -84,6 +84,7 @@
>    GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>    INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>  !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
>  
>    #
>    # Disable deprecated APIs.
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 507029404f..fcaa35acf1 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -74,6 +74,7 @@
>    GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>    INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>  !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
>  
>    #
>    # Disable deprecated APIs.
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
  2021-03-24 11:58 [PATCH v2] OvmfPkg: strip build paths in release builds Ross Burton
  2021-03-24 22:23 ` [edk2-devel] " Laszlo Ersek
@ 2021-03-24 23:25 ` Andrew Fish
  2021-03-25  2:24   ` 回复: " gaoliming
  2021-03-25 17:19   ` Laszlo Ersek
  1 sibling, 2 replies; 9+ messages in thread
From: Andrew Fish @ 2021-03-24 23:25 UTC (permalink / raw)
  To: edk2-devel-groups-io, ross; +Cc: lersek

[-- Attachment #1: Type: text/plain, Size: 6019 bytes --]

This breaks some usage we have have in our fork. We have symbols turned on for Release builds, so this change would break that. 

It looks to me that the root cause of this issue might be that the GenFw is blindly writing the debug directory entry into the PE/COFF? For native PE/COFF I think this is controlled by linker flags? For Xcode/clang it is controlled by the *_XCODE5_*_MTOC_FLAGS. So at this point it is kind of up to each toolchain how they want to deal with symbols on release builds. 


It seems kind of strange to insert a section and then zero it. Almost seems like the intent of —zero was to post process compare images? 

  -z, --zero            Zero the Debug Data Fields in the PE input image file.
                        It also zeros the time stamp fields.
                        This option can be used to compare the binary efi image.
                        It can't be combined with other action options
                        except for -o, -r option. It is a action option.
                        If it is combined with other action options, the later
                        input action option will override the previous one.

And in case you are going to ask our fork uses relative paths from the Build directory and/or a UUID string for the Debug Directory entry file name so it is a constant value and does not impact build reproducibility. 

>From a feature stand point this change will break any hope of source level debugging with RELEASE builds. I also think it changes the exception handler code output in OVMF [1] for ELF toolchains. You are going to get the (No PDB) vs. the file and path you were getting today. I assume if you had tools that natively produce PE/COFF and did not have a Debug Directory entry the same thing would happen prior to this change. 

    Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
    if (EFI_ERROR (Status)) {
      EntryPoint = NULL;
    }
    InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip);
    PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
    if (PdbPointer != NULL) {
      InternalPrintMessage ("%a", PdbPointer);
    } else {
      InternalPrintMessage ("(No PDB) " );
    }
    InternalPrintMessage (
      " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
      (VOID *) Pe32Data,
      EntryPoint
      );

Not saying we have to "stop the presses", but just trying to point out the side effects of this change. It is not so much that this change is bad, but that we have no way to turn off the Debug Directory Entry for ELF conversion, so we seem to be working around that issue with a bigger hammer?

[1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117

Thanks,

Andrew Fish

> On Mar 24, 2021, at 4:58 AM, Ross Burton <ross@burtonini.com> wrote:
> 
> GenFw will embed a NB10 section which contains the path to the input file,
> which means the output files have build paths embedded in them.  To reduce
> information leakage and ensure reproducible builds, pass --zero in release
> builds to remove this information.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3256
> Signed-off-by: Ross Burton <ross.burton@arm.com>
> ---
> OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
> OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
> OvmfPkg/OvmfPkgIa32.dsc      | 1 +
> OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
> OvmfPkg/OvmfPkgX64.dsc       | 1 +
> OvmfPkg/OvmfXen.dsc          | 1 +
> 6 files changed, 6 insertions(+)
> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index 65c42284d9..69a05feea9 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -78,6 +78,7 @@
>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
> 
>   #
>   # Disable deprecated APIs.
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index 4a1cdf5aca..132f55cf69 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -76,6 +76,7 @@
>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
> 
>   #
>   # Disable deprecated APIs.
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 1eaf3e99c6..93c209950c 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -80,6 +80,7 @@
> !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(TOOL_CHAIN_TAG) != "CLANGPDB"
>   GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
> !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
> 
>   #
>   # Disable deprecated APIs.
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 4a5a430147..97cc438250 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -84,6 +84,7 @@
>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
> 
>   #
>   # Disable deprecated APIs.
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index d4d601b444..f544fb04bf 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -84,6 +84,7 @@
>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
> 
>   #
>   # Disable deprecated APIs.
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 507029404f..fcaa35acf1 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -74,6 +74,7 @@
>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> !endif
> +  RELEASE_*_*_GENFW_FLAGS = --zero
> 
>   #
>   # Disable deprecated APIs.
> -- 
> 2.25.1
> 
> 
> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 28663 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* 回复: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
  2021-03-24 23:25 ` Andrew Fish
@ 2021-03-25  2:24   ` gaoliming
  2021-03-25  3:38     ` Andrew Fish
  2021-03-25 17:19   ` Laszlo Ersek
  1 sibling, 1 reply; 9+ messages in thread
From: gaoliming @ 2021-03-25  2:24 UTC (permalink / raw)
  To: devel, afish, ross; +Cc: lersek

[-- Attachment #1: Type: text/plain, Size: 6662 bytes --]

Andrew:

 GenFw generates debug entry, and zero debug entry. Its functionality is same to the image without debug entry. So, new option is not introduced to control whether GenFw generates debug entry when it converts ELF image to EFI image. 

 

Thanks

Liming

发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Andrew Fish via groups.io
发送时间: 2021年3月25日 7:26
收件人: edk2-devel-groups-io <devel@edk2.groups.io>; ross@burtonini.com
抄送: lersek@redhat.com
主题: Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds

 

This breaks some usage we have have in our fork. We have symbols turned on for Release builds, so this change would break that. 

 

It looks to me that the root cause of this issue might be that the GenFw is blindly writing the debug directory entry into the PE/COFF? For native PE/COFF I think this is controlled by linker flags? For Xcode/clang it is controlled by the *_XCODE5_*_MTOC_FLAGS. So at this point it is kind of up to each toolchain how they want to deal with symbols on release builds. 

 

 

It seems kind of strange to insert a section and then zero it. Almost seems like the intent of —zero was to post process compare images? 





  -z, --zero            Zero the Debug Data Fields in the PE input image file.

                        It also zeros the time stamp fields.

                        This option can be used to compare the binary efi image.

                        It can't be combined with other action options

                        except for -o, -r option. It is a action option.

                        If it is combined with other action options, the later

                        input action option will override the previous one.





And in case you are going to ask our fork uses relative paths from the Build directory and/or a UUID string for the Debug Directory entry file name so it is a constant value and does not impact build reproducibility. 





>From a feature stand point this change will break any hope of source level debugging with RELEASE builds. I also think it changes the exception handler code output in OVMF [1] for ELF toolchains. You are going to get the (No PDB) vs. the file and path you were getting today. I assume if you had tools that natively produce PE/COFF and did not have a Debug Directory entry the same thing would happen prior to this change. 





	
    Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);

	
    if (EFI_ERROR (Status)) {

	
      EntryPoint = NULL;

	
    }

	
    InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip);

	
    PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);

	
    if (PdbPointer != NULL) {

	
      InternalPrintMessage ("%a", PdbPointer);

	
    } else {

	
      InternalPrintMessage ("(No PDB) " );

	
    }

	
    InternalPrintMessage (

	
      " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",

	
      (VOID *) Pe32Data,

	
      EntryPoint

	
      );

		





Not saying we have to "stop the presses", but just trying to point out the side effects of this change. It is not so much that this change is bad, but that we have no way to turn off the Debug Directory Entry for ELF conversion, so we seem to be working around that issue with a bigger hammer?

 

[1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117

 

Thanks,





Andrew Fish





On Mar 24, 2021, at 4:58 AM, Ross Burton <ross@burtonini.com <mailto:ross@burtonini.com> > wrote:

 

GenFw will embed a NB10 section which contains the path to the input file,
which means the output files have build paths embedded in them.  To reduce
information leakage and ensure reproducible builds, pass --zero in release
builds to remove this information.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3256
Signed-off-by: Ross Burton <ross.burton@arm.com <mailto:ross.burton@arm.com> >
---
OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
OvmfPkg/OvmfPkgIa32.dsc      | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
OvmfPkg/OvmfPkgX64.dsc       | 1 +
OvmfPkg/OvmfXen.dsc          | 1 +
6 files changed, 6 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 65c42284d9..69a05feea9 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -78,6 +78,7 @@
  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS = --zero

  #
  # Disable deprecated APIs.
diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index 4a1cdf5aca..132f55cf69 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -76,6 +76,7 @@
  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS = --zero

  #
  # Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 1eaf3e99c6..93c209950c 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -80,6 +80,7 @@
!if $(TOOL_CHAIN_TAG) != "XCODE5" && $(TOOL_CHAIN_TAG) != "CLANGPDB"
  GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
!endif
+  RELEASE_*_*_GENFW_FLAGS = --zero

  #
  # Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 4a5a430147..97cc438250 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -84,6 +84,7 @@
  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS = --zero

  #
  # Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index d4d601b444..f544fb04bf 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -84,6 +84,7 @@
  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS = --zero

  #
  # Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 507029404f..fcaa35acf1 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -74,6 +74,7 @@
  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS = --zero

  #
  # Disable deprecated APIs.
-- 
2.25.1







 




[-- Attachment #2: Type: text/html, Size: 24078 bytes --]

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
  2021-03-25  2:24   ` 回复: " gaoliming
@ 2021-03-25  3:38     ` Andrew Fish
  2021-03-25  3:47       ` Andrew Fish
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Fish @ 2021-03-25  3:38 UTC (permalink / raw)
  To: gaoliming; +Cc: devel, ross, lersek

[-- Attachment #1: Type: text/plain, Size: 9411 bytes --]

Liming,

I understand how the tool works, but that is not how it is used. To make it work like other toolchains *_*_*_GENFW_FLAGS for ELF targets should pass —zero for builds that do not contain symbols. This is how it works today [1].

The proposed fix is a global hammer ` RELEASE_*_*_GENFW_FLAGS = --zero` and it prevents some one from overriding the toolchain definition to turn on source level debugging for Release builds. In all the other places this is a tool chain choice. So my complaint is solving this globally vs. on a per toolchain basis. All the other toolchains don’t produce.

If you look at the XCODE5 toolchian that we override the edk2 default   DEBUG_XCODE5_X64_CC_FLAGS has -g and RELEASE_XCODE5_X64_CC_FLAGS does not. So the source level debugging or not is part of the compiler target choice. 
  DEBUG_XCODE5_*_MTOC_FLAGS = -align 0x20 -d $(DEBUG_DIR)/$(MODULE_NAME).dll
RELEASE_XCODE5_*_MTOC_FLAGS = -align 0x20

For our override we pass -g to RELEASE builds CC_FLAGS and do `RELEASE_XCODE5_*_MTOC_FLAGS = -align 0x20 -d $(MODULE_GUID)` so the override to force --zero on every call to GenFw will break this. 

So my point is if the toolchain is generating a Debug Directory entry in a RELEASE target that does not support debugging then that target should setting `RELEASE_<target>_*_GENFW_FLAGS = —zero` to undo the unwanted work. I understand why the debug info gets added as it reduces the complexity of the ELF to PE/COFF conversion, so I’m not complaining about that just that the ELF targets don’t handle this themselves. So to me this fix is working the bug of the ELF targets not passing —zero on RELEASE builds to GenFw.

[1] $ git grep _GENFW_FLAGS -- *.template
BaseTools/Conf/tools_def.template:2872:DEBUG_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
BaseTools/Conf/tools_def.template:2877:RELEASE_CLANGPDB_X64_GENFW_FLAGS    =
BaseTools/Conf/tools_def.template:2882:NOOPT_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
BaseTools/Conf/tools_def.template:3124:*_*_*_GENFW_FLAGS                  =

Thanks,

Andrew Fish

> On Mar 24, 2021, at 7:24 PM, gaoliming <gaoliming@byosoft.com.cn> wrote:
> 
> Andrew:
>  GenFw generates debug entry, and zero debug entry. Its functionality is same to the image without debug entry. So, new option is not introduced to control whether GenFw generates debug entry when it converts ELF image to EFI image. 
>
> Thanks
> Liming
> 发件人: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> 代表 Andrew Fish via groups.io <http://groups.io/>
> 发送时间: 2021年3月25日 7:26
> 收件人: edk2-devel-groups-io <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>; ross@burtonini.com <mailto:ross@burtonini.com>
> 抄送: lersek@redhat.com <mailto:lersek@redhat.com>
> 主题: Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
>
> This breaks some usage we have have in our fork. We have symbols turned on for Release builds, so this change would break that. 
>
> It looks to me that the root cause of this issue might be that the GenFw is blindly writing the debug directory entry into the PE/COFF? For native PE/COFF I think this is controlled by linker flags? For Xcode/clang it is controlled by the *_XCODE5_*_MTOC_FLAGS. So at this point it is kind of up to each toolchain how they want to deal with symbols on release builds. 
>
>
> It seems kind of strange to insert a section and then zero it. Almost seems like the intent of —zero was to post process compare images? 
> 
> 
>   -z, --zero            Zero the Debug Data Fields in the PE input image file.
>                         It also zeros the time stamp fields.
>                         This option can be used to compare the binary efi image.
>                         It can't be combined with other action options
>                         except for -o, -r option. It is a action option.
>                         If it is combined with other action options, the later
>                         input action option will override the previous one.
> 
> 
> And in case you are going to ask our fork uses relative paths from the Build directory and/or a UUID string for the Debug Directory entry file name so it is a constant value and does not impact build reproducibility. 
> 
> 
> From a feature stand point this change will break any hope of source level debugging with RELEASE builds. I also think it changes the exception handler code output in OVMF [1] for ELF toolchains. You are going to get the (No PDB) vs. the file and path you were getting today. I assume if you had tools that natively produce PE/COFF and did not have a Debug Directory entry the same thing would happen prior to this change. 
> 
> 
>     Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
>     if (EFI_ERROR (Status)) {
>       EntryPoint = NULL;
>     }
>     InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip);
>     PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
>     if (PdbPointer != NULL) {
>       InternalPrintMessage ("%a", PdbPointer);
>     } else {
>       InternalPrintMessage ("(No PDB) " );
>     }
>     InternalPrintMessage (
>       " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
>       (VOID *) Pe32Data,
>       EntryPoint
>       );
> 
> 
> Not saying we have to "stop the presses", but just trying to point out the side effects of this change. It is not so much that this change is bad, but that we have no way to turn off the Debug Directory Entry for ELF conversion, so we seem to be working around that issue with a bigger hammer?
>
> [1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117 <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117>
>
> Thanks,
> 
> 
> Andrew Fish
> 
> 
>> On Mar 24, 2021, at 4:58 AM, Ross Burton <ross@burtonini.com <mailto:ross@burtonini.com>> wrote:
>>
>> GenFw will embed a NB10 section which contains the path to the input file,
>> which means the output files have build paths embedded in them.  To reduce
>> information leakage and ensure reproducible builds, pass --zero in release
>> builds to remove this information.
>> 
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3256 <https://bugzilla.tianocore.org/show_bug.cgi?id=3256>
>> Signed-off-by: Ross Burton <ross.burton@arm.com <mailto:ross.burton@arm.com>>
>> ---
>> OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
>> OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
>> OvmfPkg/OvmfPkgIa32.dsc      | 1 +
>> OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
>> OvmfPkg/OvmfPkgX64.dsc       | 1 +
>> OvmfPkg/OvmfXen.dsc          | 1 +
>> 6 files changed, 6 insertions(+)
>> 
>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> index 65c42284d9..69a05feea9 100644
>> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
>> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> @@ -78,6 +78,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>> 
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
>> index 4a1cdf5aca..132f55cf69 100644
>> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
>> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
>> @@ -76,6 +76,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>> 
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 1eaf3e99c6..93c209950c 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -80,6 +80,7 @@
>> !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(TOOL_CHAIN_TAG) != "CLANGPDB"
>>   GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>> 
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 4a5a430147..97cc438250 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -84,6 +84,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>> 
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index d4d601b444..f544fb04bf 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -84,6 +84,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>> 
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
>> index 507029404f..fcaa35acf1 100644
>> --- a/OvmfPkg/OvmfXen.dsc
>> +++ b/OvmfPkg/OvmfXen.dsc
>> @@ -74,6 +74,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>> 
>>   #
>>   # Disable deprecated APIs.
>> -- 
>> 2.25.1
>> 
>> 
>> 
>> 
>> 
>> 
> 
>
> 


[-- Attachment #2: Type: text/html, Size: 41552 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
  2021-03-25  3:38     ` Andrew Fish
@ 2021-03-25  3:47       ` Andrew Fish
  2021-03-25 17:26         ` Laszlo Ersek
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Fish @ 2021-03-25  3:47 UTC (permalink / raw)
  To: gaoliming; +Cc: devel, ross, lersek

[-- Attachment #1: Type: text/plain, Size: 13225 bytes --]

While we are talking about what toolchain targets should do I don’t understand this [1]? Why does OVMF need to turn on —keepexceptiontable? If these toolchains require it why don’t they turn it on? I see Mike fixed it[2]. Is this another case of a global GENFW_FLAG breaking things? Looks like the CLANGPDB toolchain does what I would expect, and the other toolchains do not? Is there some history here I’m missing?

[1]  $ git grep "keepexceptiontable"
BaseTools/Conf/tools_def.template:2872:DEBUG_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
BaseTools/Conf/tools_def.template:2882:NOOPT_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
…
OvmfPkg/OvmfPkgIa32X64.dsc:80:  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
OvmfPkg/OvmfPkgIa32X64.dsc:81:  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
OvmfPkg/OvmfPkgIa32X64.dsc:82:  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
OvmfPkg/OvmfPkgX64.dsc:80:  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
OvmfPkg/OvmfPkgX64.dsc:81:  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
OvmfPkg/OvmfPkgX64.dsc:82:  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
OvmfPkg/OvmfXen.dsc:71:  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
OvmfPkg/OvmfXen.dsc:72:  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
OvmfPkg/OvmfXen.dsc:73:  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable

[2] $ git show bbcafc442b2db
commit bbcafc442b2db91322dd3ba04e166236a41b111d
Author: mdkinney <mdkinney@6f19259b-4bc3-4df7-8a09-765794883524>
Date:   Wed Oct 3 21:00:26 2012 +0000

    The exception table information in X64 PE/COFF images is being stripped by default in the OvmfPkg.

    This patch preserves this information when SOURCE_DEBUG_ENABLE is set.

    Contributed-under: TianoCore Contribution Agreement 1.0
    Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
    Reviewed-by: Laszlo Ersek

    git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@13780 6f19259b-4bc3-4df7-8a09-765794883524

diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 9b2ba8626ab3..40fd5e97b24e 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -41,7 +41,12 @@ [BuildOptions]
   INTEL:RELEASE_*_*_CC_FLAGS           = /D MDEPKG_NDEBUG
   MSFT:RELEASE_*_*_CC_FLAGS            = /D MDEPKG_NDEBUG
   GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
-
+!ifdef $(SOURCE_DEBUG_ENABLE)
+  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
+  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
+  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
+!endif
+
 ################################################################################
 #
 # SKU Identification section - list of all SKU IDs supported by this Platform.
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 9ff4a5de1005..c61d41dccbbe 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -41,6 +41,11 @@ [BuildOptions]
   INTEL:RELEASE_*_*_CC_FLAGS           = /D MDEPKG_NDEBUG
   MSFT:RELEASE_*_*_CC_FLAGS            = /D MDEPKG_NDEBUG
   GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
+!ifdef $(SOURCE_DEBUG_ENABLE)
+  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
+  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
+  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
+!endif
 
 ################################################################################
 #


Thanks,

Andrew Fish

> On Mar 24, 2021, at 8:38 PM, Andrew Fish <afish@apple.com> wrote:
> 
> Liming,
> 
> I understand how the tool works, but that is not how it is used. To make it work like other toolchains *_*_*_GENFW_FLAGS for ELF targets should pass —zero for builds that do not contain symbols. This is how it works today [1].
> 
> The proposed fix is a global hammer ` RELEASE_*_*_GENFW_FLAGS = --zero` and it prevents some one from overriding the toolchain definition to turn on source level debugging for Release builds. In all the other places this is a tool chain choice. So my complaint is solving this globally vs. on a per toolchain basis. All the other toolchains don’t produce.
> 
> If you look at the XCODE5 toolchian that we override the edk2 default   DEBUG_XCODE5_X64_CC_FLAGS has -g and RELEASE_XCODE5_X64_CC_FLAGS does not. So the source level debugging or not is part of the compiler target choice. 
>   DEBUG_XCODE5_*_MTOC_FLAGS = -align 0x20 -d $(DEBUG_DIR)/$(MODULE_NAME).dll
> RELEASE_XCODE5_*_MTOC_FLAGS = -align 0x20
> 
> For our override we pass -g to RELEASE builds CC_FLAGS and do `RELEASE_XCODE5_*_MTOC_FLAGS = -align 0x20 -d $(MODULE_GUID)` so the override to force --zero on every call to GenFw will break this. 
> 
> So my point is if the toolchain is generating a Debug Directory entry in a RELEASE target that does not support debugging then that target should setting `RELEASE_<target>_*_GENFW_FLAGS = —zero` to undo the unwanted work. I understand why the debug info gets added as it reduces the complexity of the ELF to PE/COFF conversion, so I’m not complaining about that just that the ELF targets don’t handle this themselves. So to me this fix is working the bug of the ELF targets not passing —zero on RELEASE builds to GenFw.
> 
> [1] $ git grep _GENFW_FLAGS -- *.template
> BaseTools/Conf/tools_def.template:2872:DEBUG_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
> BaseTools/Conf/tools_def.template:2877:RELEASE_CLANGPDB_X64_GENFW_FLAGS    =
> BaseTools/Conf/tools_def.template:2882:NOOPT_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
> BaseTools/Conf/tools_def.template:3124:*_*_*_GENFW_FLAGS                  =
> 
> Thanks,
> 
> Andrew Fish
> 
>> On Mar 24, 2021, at 7:24 PM, gaoliming <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>> wrote:
>> 
>> Andrew:
>>  GenFw generates debug entry, and zero debug entry. Its functionality is same to the image without debug entry. So, new option is not introduced to control whether GenFw generates debug entry when it converts ELF image to EFI image. 
>>
>> Thanks
>> Liming
>> 发件人: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> 代表 Andrew Fish via groups.io <http://groups.io/>
>> 发送时间: 2021年3月25日 7:26
>> 收件人: edk2-devel-groups-io <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>; ross@burtonini.com <mailto:ross@burtonini.com>
>> 抄送: lersek@redhat.com <mailto:lersek@redhat.com>
>> 主题: Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
>>
>> This breaks some usage we have have in our fork. We have symbols turned on for Release builds, so this change would break that. 
>>
>> It looks to me that the root cause of this issue might be that the GenFw is blindly writing the debug directory entry into the PE/COFF? For native PE/COFF I think this is controlled by linker flags? For Xcode/clang it is controlled by the *_XCODE5_*_MTOC_FLAGS. So at this point it is kind of up to each toolchain how they want to deal with symbols on release builds. 
>>
>>
>> It seems kind of strange to insert a section and then zero it. Almost seems like the intent of —zero was to post process compare images? 
>> 
>> 
>>   -z, --zero            Zero the Debug Data Fields in the PE input image file.
>>                         It also zeros the time stamp fields.
>>                         This option can be used to compare the binary efi image.
>>                         It can't be combined with other action options
>>                         except for -o, -r option. It is a action option.
>>                         If it is combined with other action options, the later
>>                         input action option will override the previous one.
>> 
>> 
>> And in case you are going to ask our fork uses relative paths from the Build directory and/or a UUID string for the Debug Directory entry file name so it is a constant value and does not impact build reproducibility. 
>> 
>> 
>> From a feature stand point this change will break any hope of source level debugging with RELEASE builds. I also think it changes the exception handler code output in OVMF [1] for ELF toolchains. You are going to get the (No PDB) vs. the file and path you were getting today. I assume if you had tools that natively produce PE/COFF and did not have a Debug Directory entry the same thing would happen prior to this change. 
>> 
>> 
>>     Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
>>     if (EFI_ERROR (Status)) {
>>       EntryPoint = NULL;
>>     }
>>     InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip);
>>     PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
>>     if (PdbPointer != NULL) {
>>       InternalPrintMessage ("%a", PdbPointer);
>>     } else {
>>       InternalPrintMessage ("(No PDB) " );
>>     }
>>     InternalPrintMessage (
>>       " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
>>       (VOID *) Pe32Data,
>>       EntryPoint
>>       );
>> 
>> 
>> Not saying we have to "stop the presses", but just trying to point out the side effects of this change. It is not so much that this change is bad, but that we have no way to turn off the Debug Directory Entry for ELF conversion, so we seem to be working around that issue with a bigger hammer?
>>
>> [1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117 <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117>
>>
>> Thanks,
>> 
>> 
>> Andrew Fish
>> 
>> 
>>> On Mar 24, 2021, at 4:58 AM, Ross Burton <ross@burtonini.com <mailto:ross@burtonini.com>> wrote:
>>>
>>> GenFw will embed a NB10 section which contains the path to the input file,
>>> which means the output files have build paths embedded in them.  To reduce
>>> information leakage and ensure reproducible builds, pass --zero in release
>>> builds to remove this information.
>>> 
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3256 <https://bugzilla.tianocore.org/show_bug.cgi?id=3256>
>>> Signed-off-by: Ross Burton <ross.burton@arm.com <mailto:ross.burton@arm.com>>
>>> ---
>>> OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
>>> OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
>>> OvmfPkg/OvmfPkgIa32.dsc      | 1 +
>>> OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
>>> OvmfPkg/OvmfPkgX64.dsc       | 1 +
>>> OvmfPkg/OvmfXen.dsc          | 1 +
>>> 6 files changed, 6 insertions(+)
>>> 
>>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>>> index 65c42284d9..69a05feea9 100644
>>> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
>>> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
>>> @@ -78,6 +78,7 @@
>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>   #
>>>   # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
>>> index 4a1cdf5aca..132f55cf69 100644
>>> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
>>> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
>>> @@ -76,6 +76,7 @@
>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>   #
>>>   # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>> index 1eaf3e99c6..93c209950c 100644
>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>> @@ -80,6 +80,7 @@
>>> !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(TOOL_CHAIN_TAG) != "CLANGPDB"
>>>   GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>   #
>>>   # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> index 4a5a430147..97cc438250 100644
>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> @@ -84,6 +84,7 @@
>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>   #
>>>   # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index d4d601b444..f544fb04bf 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -84,6 +84,7 @@
>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>   #
>>>   # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
>>> index 507029404f..fcaa35acf1 100644
>>> --- a/OvmfPkg/OvmfXen.dsc
>>> +++ b/OvmfPkg/OvmfXen.dsc
>>> @@ -74,6 +74,7 @@
>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>   #
>>>   # Disable deprecated APIs.
>>> -- 
>>> 2.25.1
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>> 
>>
>> 
> 


[-- Attachment #2: Type: text/html, Size: 62167 bytes --]

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
  2021-03-25  3:47       ` Andrew Fish
@ 2021-03-25 17:26         ` Laszlo Ersek
  0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2021-03-25 17:26 UTC (permalink / raw)
  To: Andrew Fish, gaoliming; +Cc: devel, ross, Michael Kinney

On 03/25/21 04:47, Andrew Fish wrote:
> While we are talking about what toolchain targets should do I don’t understand this [1]? Why does OVMF need to turn on —keepexceptiontable? If these toolchains require it why don’t they turn it on? I see Mike fixed it[2]. Is this another case of a global GENFW_FLAG breaking things? Looks like the CLANGPDB toolchain does what I would expect, and the other toolchains do not? Is there some history here I’m missing?

Unfortunately, I can't provide any background on this change. I still
have the original 2012 email thread in my local archives that led to
this commit (message ID of Mike's thread starter:
<E92EE9817A31E24EB0585FDF735412F51CB770F4@ORSMSX107.amr.corp.intel.com>),
but there's really no more info in that than what the commit message
says -- "SOURCE_DEBUG_ENABLE needs it".

I basically never use SOURCE_DEBUG_ENABLE, so I don't know what the
needs and drawbacks are.

Adding Mike to the CC list.

Thanks
Laszlo

> 
> [1]  $ git grep "keepexceptiontable"
> BaseTools/Conf/tools_def.template:2872:DEBUG_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
> BaseTools/Conf/tools_def.template:2882:NOOPT_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
> …
> OvmfPkg/OvmfPkgIa32X64.dsc:80:  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
> OvmfPkg/OvmfPkgIa32X64.dsc:81:  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
> OvmfPkg/OvmfPkgIa32X64.dsc:82:  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> OvmfPkg/OvmfPkgX64.dsc:80:  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
> OvmfPkg/OvmfPkgX64.dsc:81:  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
> OvmfPkg/OvmfPkgX64.dsc:82:  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> OvmfPkg/OvmfXen.dsc:71:  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
> OvmfPkg/OvmfXen.dsc:72:  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
> OvmfPkg/OvmfXen.dsc:73:  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> 
> [2] $ git show bbcafc442b2db
> commit bbcafc442b2db91322dd3ba04e166236a41b111d
> Author: mdkinney <mdkinney@6f19259b-4bc3-4df7-8a09-765794883524>
> Date:   Wed Oct 3 21:00:26 2012 +0000
> 
>     The exception table information in X64 PE/COFF images is being stripped by default in the OvmfPkg.
>     
>     This patch preserves this information when SOURCE_DEBUG_ENABLE is set.
>     
>     Contributed-under: TianoCore Contribution Agreement 1.0
>     Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
>     Reviewed-by: Laszlo Ersek
>     
>     git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@13780 6f19259b-4bc3-4df7-8a09-765794883524
> 
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 9b2ba8626ab3..40fd5e97b24e 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -41,7 +41,12 @@ [BuildOptions]
>    INTEL:RELEASE_*_*_CC_FLAGS           = /D MDEPKG_NDEBUG
>    MSFT:RELEASE_*_*_CC_FLAGS            = /D MDEPKG_NDEBUG
>    GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
> -
> +!ifdef $(SOURCE_DEBUG_ENABLE)
> +  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
> +  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
> +  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> +!endif
> +  
>  ################################################################################
>  #
>  # SKU Identification section - list of all SKU IDs supported by this Platform.
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 9ff4a5de1005..c61d41dccbbe 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -41,6 +41,11 @@ [BuildOptions]
>    INTEL:RELEASE_*_*_CC_FLAGS           = /D MDEPKG_NDEBUG
>    MSFT:RELEASE_*_*_CC_FLAGS            = /D MDEPKG_NDEBUG
>    GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
> +!ifdef $(SOURCE_DEBUG_ENABLE)
> +  MSFT:*_*_X64_GENFW_FLAGS  = --keepexceptiontable
> +  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
> +  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> +!endif
>  
>  ################################################################################
>  #
> 
> 
> Thanks,
> 
> Andrew Fish
> 
>> On Mar 24, 2021, at 8:38 PM, Andrew Fish <afish@apple.com> wrote:
>>
>> Liming,
>>
>> I understand how the tool works, but that is not how it is used. To make it work like other toolchains *_*_*_GENFW_FLAGS for ELF targets should pass —zero for builds that do not contain symbols. This is how it works today [1].
>>
>> The proposed fix is a global hammer ` RELEASE_*_*_GENFW_FLAGS = --zero` and it prevents some one from overriding the toolchain definition to turn on source level debugging for Release builds. In all the other places this is a tool chain choice. So my complaint is solving this globally vs. on a per toolchain basis. All the other toolchains don’t produce.
>>
>> If you look at the XCODE5 toolchian that we override the edk2 default   DEBUG_XCODE5_X64_CC_FLAGS has -g and RELEASE_XCODE5_X64_CC_FLAGS does not. So the source level debugging or not is part of the compiler target choice. 
>>   DEBUG_XCODE5_*_MTOC_FLAGS = -align 0x20 -d $(DEBUG_DIR)/$(MODULE_NAME).dll
>> RELEASE_XCODE5_*_MTOC_FLAGS = -align 0x20
>>
>> For our override we pass -g to RELEASE builds CC_FLAGS and do `RELEASE_XCODE5_*_MTOC_FLAGS = -align 0x20 -d $(MODULE_GUID)` so the override to force --zero on every call to GenFw will break this. 
>>
>> So my point is if the toolchain is generating a Debug Directory entry in a RELEASE target that does not support debugging then that target should setting `RELEASE_<target>_*_GENFW_FLAGS = —zero` to undo the unwanted work. I understand why the debug info gets added as it reduces the complexity of the ELF to PE/COFF conversion, so I’m not complaining about that just that the ELF targets don’t handle this themselves. So to me this fix is working the bug of the ELF targets not passing —zero on RELEASE builds to GenFw.
>>
>> [1] $ git grep _GENFW_FLAGS -- *.template
>> BaseTools/Conf/tools_def.template:2872:DEBUG_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
>> BaseTools/Conf/tools_def.template:2877:RELEASE_CLANGPDB_X64_GENFW_FLAGS    =
>> BaseTools/Conf/tools_def.template:2882:NOOPT_CLANGPDB_X64_GENFW_FLAGS      = --keepexceptiontable
>> BaseTools/Conf/tools_def.template:3124:*_*_*_GENFW_FLAGS                  =
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>> On Mar 24, 2021, at 7:24 PM, gaoliming <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>> wrote:
>>>
>>> Andrew:
>>>  GenFw generates debug entry, and zero debug entry. Its functionality is same to the image without debug entry. So, new option is not introduced to control whether GenFw generates debug entry when it converts ELF image to EFI image. 
>>>  
>>> Thanks
>>> Liming
>>> 发件人: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> 代表 Andrew Fish via groups.io <http://groups.io/>
>>> 发送时间: 2021年3月25日 7:26
>>> 收件人: edk2-devel-groups-io <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>; ross@burtonini.com <mailto:ross@burtonini.com>
>>> 抄送: lersek@redhat.com <mailto:lersek@redhat.com>
>>> 主题: Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
>>>  
>>> This breaks some usage we have have in our fork. We have symbols turned on for Release builds, so this change would break that. 
>>>  
>>> It looks to me that the root cause of this issue might be that the GenFw is blindly writing the debug directory entry into the PE/COFF? For native PE/COFF I think this is controlled by linker flags? For Xcode/clang it is controlled by the *_XCODE5_*_MTOC_FLAGS. So at this point it is kind of up to each toolchain how they want to deal with symbols on release builds. 
>>>  
>>>  
>>> It seems kind of strange to insert a section and then zero it. Almost seems like the intent of —zero was to post process compare images? 
>>>
>>>
>>>   -z, --zero            Zero the Debug Data Fields in the PE input image file.
>>>                         It also zeros the time stamp fields.
>>>                         This option can be used to compare the binary efi image.
>>>                         It can't be combined with other action options
>>>                         except for -o, -r option. It is a action option.
>>>                         If it is combined with other action options, the later
>>>                         input action option will override the previous one.
>>>
>>>
>>> And in case you are going to ask our fork uses relative paths from the Build directory and/or a UUID string for the Debug Directory entry file name so it is a constant value and does not impact build reproducibility. 
>>>
>>>
>>> From a feature stand point this change will break any hope of source level debugging with RELEASE builds. I also think it changes the exception handler code output in OVMF [1] for ELF toolchains. You are going to get the (No PDB) vs. the file and path you were getting today. I assume if you had tools that natively produce PE/COFF and did not have a Debug Directory entry the same thing would happen prior to this change. 
>>>
>>>
>>>     Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
>>>     if (EFI_ERROR (Status)) {
>>>       EntryPoint = NULL;
>>>     }
>>>     InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip);
>>>     PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
>>>     if (PdbPointer != NULL) {
>>>       InternalPrintMessage ("%a", PdbPointer);
>>>     } else {
>>>       InternalPrintMessage ("(No PDB) " );
>>>     }
>>>     InternalPrintMessage (
>>>       " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
>>>       (VOID *) Pe32Data,
>>>       EntryPoint
>>>       );
>>>
>>>
>>> Not saying we have to "stop the presses", but just trying to point out the side effects of this change. It is not so much that this change is bad, but that we have no way to turn off the Debug Directory Entry for ELF conversion, so we seem to be working around that issue with a bigger hammer?
>>>  
>>> [1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117 <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117>
>>>  
>>> Thanks,
>>>
>>>
>>> Andrew Fish
>>>
>>>
>>>> On Mar 24, 2021, at 4:58 AM, Ross Burton <ross@burtonini.com <mailto:ross@burtonini.com>> wrote:
>>>>  
>>>> GenFw will embed a NB10 section which contains the path to the input file,
>>>> which means the output files have build paths embedded in them.  To reduce
>>>> information leakage and ensure reproducible builds, pass --zero in release
>>>> builds to remove this information.
>>>>
>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3256 <https://bugzilla.tianocore.org/show_bug.cgi?id=3256>
>>>> Signed-off-by: Ross Burton <ross.burton@arm.com <mailto:ross.burton@arm.com>>
>>>> ---
>>>> OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
>>>> OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
>>>> OvmfPkg/OvmfPkgIa32.dsc      | 1 +
>>>> OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
>>>> OvmfPkg/OvmfPkgX64.dsc       | 1 +
>>>> OvmfPkg/OvmfXen.dsc          | 1 +
>>>> 6 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>>>> index 65c42284d9..69a05feea9 100644
>>>> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
>>>> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
>>>> @@ -78,6 +78,7 @@
>>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>>> !endif
>>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>>>
>>>>   #
>>>>   # Disable deprecated APIs.
>>>> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
>>>> index 4a1cdf5aca..132f55cf69 100644
>>>> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
>>>> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
>>>> @@ -76,6 +76,7 @@
>>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>>> !endif
>>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>>>
>>>>   #
>>>>   # Disable deprecated APIs.
>>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>>> index 1eaf3e99c6..93c209950c 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>>> @@ -80,6 +80,7 @@
>>>> !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(TOOL_CHAIN_TAG) != "CLANGPDB"
>>>>   GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
>>>> !endif
>>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>>>
>>>>   #
>>>>   # Disable deprecated APIs.
>>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> index 4a5a430147..97cc438250 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> @@ -84,6 +84,7 @@
>>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>>> !endif
>>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>>>
>>>>   #
>>>>   # Disable deprecated APIs.
>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>>> index d4d601b444..f544fb04bf 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>> @@ -84,6 +84,7 @@
>>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>>> !endif
>>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>>>
>>>>   #
>>>>   # Disable deprecated APIs.
>>>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
>>>> index 507029404f..fcaa35acf1 100644
>>>> --- a/OvmfPkg/OvmfXen.dsc
>>>> +++ b/OvmfPkg/OvmfXen.dsc
>>>> @@ -74,6 +74,7 @@
>>>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>>> !endif
>>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>>>
>>>>   #
>>>>   # Disable deprecated APIs.
>>>> -- 
>>>> 2.25.1
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>  
>>> 
>>
> 
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
  2021-03-24 23:25 ` Andrew Fish
  2021-03-25  2:24   ` 回复: " gaoliming
@ 2021-03-25 17:19   ` Laszlo Ersek
  2021-03-25 19:43     ` Andrew Fish
  1 sibling, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2021-03-25 17:19 UTC (permalink / raw)
  To: Andrew Fish
  Cc: edk2-devel-groups-io, ross, ross.burton,
	Ard Biesheuvel (TianoCore)

On 03/25/21 00:25, Andrew Fish wrote:
> This breaks some usage we have have in our fork. We have symbols turned on for Release builds, so this change would break that. 
> 
> It looks to me that the root cause of this issue might be that the GenFw is blindly writing the debug directory entry into the PE/COFF?

Yes, that's my understanding, from TianoCore#3256.

> For native PE/COFF I think this is controlled by linker flags? For Xcode/clang it is controlled by the *_XCODE5_*_MTOC_FLAGS. So at this point it is kind of up to each toolchain how they want to deal with symbols on release builds. 
> 
> 
> It seems kind of strange to insert a section and then zero it. Almost seems like the intent of —zero was to post process compare images? 
> 
>   -z, --zero            Zero the Debug Data Fields in the PE input image file.
>                         It also zeros the time stamp fields.
>                         This option can be used to compare the binary efi image.
>                         It can't be combined with other action options
>                         except for -o, -r option. It is a action option.
>                         If it is combined with other action options, the later
>                         input action option will override the previous one.
> 
> And in case you are going to ask our fork uses relative paths from the Build directory and/or a UUID string for the Debug Directory entry file name so it is a constant value and does not impact build reproducibility. 

I'd like if we could satisfy both your use case and Ross's (Yocto's).

Until we have a technical solution for that, is it important that we
revert the patch upstream? (If it's urgent, I'm going to ask someone
else to do that, because I'll be back in April.)

> From a feature stand point this change will break any hope of source level debugging with RELEASE builds. I also think it changes the exception handler code output in OVMF [1] for ELF toolchains. You are going to get the (No PDB) vs. the file and path you were getting today. I assume if you had tools that natively produce PE/COFF and did not have a Debug Directory entry the same thing would happen prior to this change. 
> 
>     Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
>     if (EFI_ERROR (Status)) {
>       EntryPoint = NULL;
>     }
>     InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip);
>     PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
>     if (PdbPointer != NULL) {
>       InternalPrintMessage ("%a", PdbPointer);
>     } else {
>       InternalPrintMessage ("(No PDB) " );
>     }
>     InternalPrintMessage (
>       " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
>       (VOID *) Pe32Data,
>       EntryPoint
>       );
> 
> Not saying we have to "stop the presses", but just trying to point out the side effects of this change. It is not so much that this change is bad, but that we have no way to turn off the Debug Directory Entry for ELF conversion, so we seem to be working around that issue with a bigger hammer?

I don't have suggestions alas, but am open to any solution that works
for you and Ross both.

Thanks (and my apologies for breaking your process!),
Laszlo

> 
> [1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117
> 
> Thanks,
> 
> Andrew Fish
> 
>> On Mar 24, 2021, at 4:58 AM, Ross Burton <ross@burtonini.com> wrote:
>>
>> GenFw will embed a NB10 section which contains the path to the input file,
>> which means the output files have build paths embedded in them.  To reduce
>> information leakage and ensure reproducible builds, pass --zero in release
>> builds to remove this information.
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3256
>> Signed-off-by: Ross Burton <ross.burton@arm.com>
>> ---
>> OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
>> OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
>> OvmfPkg/OvmfPkgIa32.dsc      | 1 +
>> OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
>> OvmfPkg/OvmfPkgX64.dsc       | 1 +
>> OvmfPkg/OvmfXen.dsc          | 1 +
>> 6 files changed, 6 insertions(+)
>>
>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> index 65c42284d9..69a05feea9 100644
>> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
>> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> @@ -78,6 +78,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
>> index 4a1cdf5aca..132f55cf69 100644
>> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
>> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
>> @@ -76,6 +76,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 1eaf3e99c6..93c209950c 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -80,6 +80,7 @@
>> !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(TOOL_CHAIN_TAG) != "CLANGPDB"
>>   GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 4a5a430147..97cc438250 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -84,6 +84,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index d4d601b444..f544fb04bf 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -84,6 +84,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>
>>   #
>>   # Disable deprecated APIs.
>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
>> index 507029404f..fcaa35acf1 100644
>> --- a/OvmfPkg/OvmfXen.dsc
>> +++ b/OvmfPkg/OvmfXen.dsc
>> @@ -74,6 +74,7 @@
>>   GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>> !endif
>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>
>>   #
>>   # Disable deprecated APIs.
>> -- 
>> 2.25.1
>>
>>
>>
>> 
>>
>>
> 
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds
  2021-03-25 17:19   ` Laszlo Ersek
@ 2021-03-25 19:43     ` Andrew Fish
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Fish @ 2021-03-25 19:43 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: edk2-devel-groups-io, ross, ross.burton,
	Ard Biesheuvel (TianoCore)

[-- Attachment #1: Type: text/plain, Size: 7246 bytes --]



> On Mar 25, 2021, at 10:19 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 03/25/21 00:25, Andrew Fish wrote:
>> This breaks some usage we have have in our fork. We have symbols turned on for Release builds, so this change would break that. 
>> 
>> It looks to me that the root cause of this issue might be that the GenFw is blindly writing the debug directory entry into the PE/COFF?
> 
> Yes, that's my understanding, from TianoCore#3256.
> 
>> For native PE/COFF I think this is controlled by linker flags? For Xcode/clang it is controlled by the *_XCODE5_*_MTOC_FLAGS. So at this point it is kind of up to each toolchain how they want to deal with symbols on release builds. 
>> 
>> 
>> It seems kind of strange to insert a section and then zero it. Almost seems like the intent of —zero was to post process compare images? 
>> 
>>  -z, --zero            Zero the Debug Data Fields in the PE input image file.
>>                        It also zeros the time stamp fields.
>>                        This option can be used to compare the binary efi image.
>>                        It can't be combined with other action options
>>                        except for -o, -r option. It is a action option.
>>                        If it is combined with other action options, the later
>>                        input action option will override the previous one.
>> 
>> And in case you are going to ask our fork uses relative paths from the Build directory and/or a UUID string for the Debug Directory entry file name so it is a constant value and does not impact build reproducibility. 
> 
> I'd like if we could satisfy both your use case and Ross's (Yocto's).
> 
> Until we have a technical solution for that, is it important that we
> revert the patch upstream? (If it's urgent, I'm going to ask someone
> else to do that, because I'll be back in April.)
> 

Laszlo,

Per my other email about —keepexceptiontable it looks like source level debug is intertwined with GenFw flags that are global, and this has been going on for a long time. So I think I’ll just file a BZ about this issue in general and not ask for changes in this patch. 

Thanks,

Andrew Fish

>> From a feature stand point this change will break any hope of source level debugging with RELEASE builds. I also think it changes the exception handler code output in OVMF [1] for ELF toolchains. You are going to get the (No PDB) vs. the file and path you were getting today. I assume if you had tools that natively produce PE/COFF and did not have a Debug Directory entry the same thing would happen prior to this change. 
>> 
>>    Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
>>    if (EFI_ERROR (Status)) {
>>      EntryPoint = NULL;
>>    }
>>    InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip);
>>    PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
>>    if (PdbPointer != NULL) {
>>      InternalPrintMessage ("%a", PdbPointer);
>>    } else {
>>      InternalPrintMessage ("(No PDB) " );
>>    }
>>    InternalPrintMessage (
>>      " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
>>      (VOID *) Pe32Data,
>>      EntryPoint
>>      );
>> 
>> Not saying we have to "stop the presses", but just trying to point out the side effects of this change. It is not so much that this change is bad, but that we have no way to turn off the Debug Directory Entry for ELF conversion, so we seem to be working around that issue with a bigger hammer?
> 
> I don't have suggestions alas, but am open to any solution that works
> for you and Ross both.
> 
> Thanks (and my apologies for breaking your process!),
> Laszlo
> 
>> 
>> [1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c#L117
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>>> On Mar 24, 2021, at 4:58 AM, Ross Burton <ross@burtonini.com> wrote:
>>> 
>>> GenFw will embed a NB10 section which contains the path to the input file,
>>> which means the output files have build paths embedded in them.  To reduce
>>> information leakage and ensure reproducible builds, pass --zero in release
>>> builds to remove this information.
>>> 
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3256
>>> Signed-off-by: Ross Burton <ross.burton@arm.com>
>>> ---
>>> OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
>>> OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
>>> OvmfPkg/OvmfPkgIa32.dsc      | 1 +
>>> OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
>>> OvmfPkg/OvmfPkgX64.dsc       | 1 +
>>> OvmfPkg/OvmfXen.dsc          | 1 +
>>> 6 files changed, 6 insertions(+)
>>> 
>>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>>> index 65c42284d9..69a05feea9 100644
>>> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
>>> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
>>> @@ -78,6 +78,7 @@
>>>  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>  #
>>>  # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
>>> index 4a1cdf5aca..132f55cf69 100644
>>> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
>>> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
>>> @@ -76,6 +76,7 @@
>>>  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>  #
>>>  # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>> index 1eaf3e99c6..93c209950c 100644
>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>> @@ -80,6 +80,7 @@
>>> !if $(TOOL_CHAIN_TAG) != "XCODE5" && $(TOOL_CHAIN_TAG) != "CLANGPDB"
>>>  GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>  #
>>>  # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> index 4a5a430147..97cc438250 100644
>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> @@ -84,6 +84,7 @@
>>>  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>  #
>>>  # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index d4d601b444..f544fb04bf 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -84,6 +84,7 @@
>>>  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>  #
>>>  # Disable deprecated APIs.
>>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
>>> index 507029404f..fcaa35acf1 100644
>>> --- a/OvmfPkg/OvmfXen.dsc
>>> +++ b/OvmfPkg/OvmfXen.dsc
>>> @@ -74,6 +74,7 @@
>>>  GCC:*_*_X64_GENFW_FLAGS   = --keepexceptiontable
>>>  INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
>>> !endif
>>> +  RELEASE_*_*_GENFW_FLAGS = --zero
>>> 
>>>  #
>>>  # Disable deprecated APIs.
>>> -- 
>>> 2.25.1
>>> 
>>> 
>>> 
>>> 


[-- Attachment #2: Type: text/html, Size: 22468 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-25 19:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-24 11:58 [PATCH v2] OvmfPkg: strip build paths in release builds Ross Burton
2021-03-24 22:23 ` [edk2-devel] " Laszlo Ersek
2021-03-24 23:25 ` Andrew Fish
2021-03-25  2:24   ` 回复: " gaoliming
2021-03-25  3:38     ` Andrew Fish
2021-03-25  3:47       ` Andrew Fish
2021-03-25 17:26         ` Laszlo Ersek
2021-03-25 17:19   ` Laszlo Ersek
2021-03-25 19:43     ` Andrew Fish

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox