From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by mx.groups.io with SMTP id smtpd.web11.2658.1616639078018172181 for ; Wed, 24 Mar 2021 19:24:39 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: byosoft.com.cn, ip: 58.240.74.242, mailfrom: gaoliming@byosoft.com.cn) Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Thu, 25 Mar 2021 10:24:29 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 58.246.60.130 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: , , Cc: References: <20210324115819.605436-1-ross.burton@arm.com> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIHYyXSBPdm1mUGtnOiBzdHJpcCBidWlsZCBwYXRocyBpbiByZWxlYXNlIGJ1aWxkcw==?= Date: Thu, 25 Mar 2021 10:24:29 +0800 Message-ID: <00b301d7211d$fc148390$f43d8ab0$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQK0MBI6XptBEhua6czXWhDjt1q4iAJ4fWmsqMYNpAA= Content-Type: multipart/alternative; boundary="----=_NextPart_000_00B4_01D72161.0A39BF60" Content-Language: zh-cn ------=_NextPart_000_00B4_01D72161.0A39BF60 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Andrew: GenFw generates debug entry, and zero debug entry. Its functionality is s= ame to the image without debug entry. So, new option is not introduced to c= ontrol whether GenFw generates debug entry when it converts ELF image to EF= I image.=20 =20 Thanks Liming =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io = =E4=BB=A3=E8=A1=A8 Andrew Fish via groups.io =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2021=E5=B9=B43=E6=9C=8825=E6=97=A5 7= :26 =E6=94=B6=E4=BB=B6=E4=BA=BA: edk2-devel-groups-io ; = ross@burtonini.com =E6=8A=84=E9=80=81: lersek@redhat.com =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths= in release builds =20 This breaks some usage we have have in our fork. We have symbols turned on= for Release builds, so this change would break that.=20 =20 It looks to me that the root cause of this issue might be that the GenFw i= s 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 con= trolled 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.=20 =20 =20 It seems kind of strange to insert a section and then zero it. Almost seem= s like the intent of =E2=80=94zero was to post process compare images?=20 -z, --zero Zero the Debug Data Fields in the PE input image f= ile. 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 l= ater input action option will override the previous one= . And in case you are going to ask our fork uses relative paths from the Bui= ld directory and/or a UUID string for the Debug Directory entry file name s= o it is a constant value and does not impact build reproducibility.=20 >>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 handl= er 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 too= ls that natively produce PE/COFF and did not have a Debug Directory entry t= he same thing would happen prior to this change.=20 =09 Status =3D PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint); =09 if (EFI_ERROR (Status)) { =09 EntryPoint =3D NULL; =09 } =09 InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip= ); =09 PdbPointer =3D PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data); =09 if (PdbPointer !=3D NULL) { =09 InternalPrintMessage ("%a", PdbPointer); =09 } else { =09 InternalPrintMessage ("(No PDB) " ); =09 } =09 InternalPrintMessage ( =09 " (ImageBase=3D%016lp, EntryPoint=3D%016p) !!!!\n", =09 (VOID *) Pe32Data, =09 EntryPoint =09 ); =09 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, bu= t that we have no way to turn off the Debug Directory Entry for ELF convers= ion, so we seem to be working around that issue with a bigger hammer? =20 [1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuEx= ceptionHandlerLib/CpuExceptionCommon.c#L117 =20 Thanks, Andrew Fish On Mar 24, 2021, at 4:58 AM, Ross Burton > wrote: =20 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=3D3256 Signed-off-by: Ross Burton > --- 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 =3D --keepexceptiontable INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable !endif + RELEASE_*_*_GENFW_FLAGS =3D --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 =3D --keepexceptiontable INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable !endif + RELEASE_*_*_GENFW_FLAGS =3D --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) !=3D "XCODE5" && $(TOOL_CHAIN_TAG) !=3D "CLANGPDB" GCC:*_*_*_CC_FLAGS =3D -mno-mmx -mno-sse !endif + RELEASE_*_*_GENFW_FLAGS =3D --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 =3D --keepexceptiontable INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable !endif + RELEASE_*_*_GENFW_FLAGS =3D --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 =3D --keepexceptiontable INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable !endif + RELEASE_*_*_GENFW_FLAGS =3D --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 =3D --keepexceptiontable INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable !endif + RELEASE_*_*_GENFW_FLAGS =3D --zero # # Disable deprecated APIs. --=20 2.25.1 =20 ------=_NextPart_000_00B4_01D72161.0A39BF60 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable

Andrew:

= =C2=A0GenFw 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

= =E5=8F= =91=E4=BB=B6=E4=BA=BA: devel@edk2.g= roups.io <devel@edk2.groups.io> =E4=BB=A3=E8=A1=A8 Andrew= Fish via groups.io
=E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2021=E5=B9=B43=E6=9C=8825=E6=97=A5 7:26
=E6=94= = =B6=E4=BB=B6=E4=BA=BA: ed= k2-devel-groups-io <devel@edk2.groups.io>; ross@burtonini.com
=E6=8A=84=E9=80=81: = lersek@redhat.com
=E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build p= aths in release builds

 

This breaks some usage we have have in our fork. We ha= ve symbols turned on for Release builds, so this change would break that.&n= bsp;

 

= 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 cont= rolled 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.&nb= sp;

 

 

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



=   -z, --zero            Zero the De= bug Data Fields in the PE input image file.

                &= nbsp;       It also zeros the time stamp fields.

        &nbs= p;               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.

                  &n= bsp;     If it is combined with other action options, the la= ter

      &= nbsp;                 input ac= tion option will override the previous one.



And in case you a= re going to ask our fork uses relative paths from the Build directory and/o= r 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 s= tand point this change will break any hope of source level debugging with R= ELEASE 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 fil= e 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 wou= ld happen prior to this change. 



<= td valign=3Dtop style=3D'padding:0cm 7.5pt 0cm 7.5pt;box-sizing: border-box= ;color:var(--color-text-primary);overflow:visible' id=3DLC133>

=C2=A0=C2=A0=C2=A0 Status =3D PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &= EntryPoint);

= = = =

=C2=A0=C2=A0 =C2=A0if (EFI_ERROR (Status)) {

=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 EntryPoint =3D NULL;

=C2=A0=C2=A0=C2= =A0 }

=C2=A0=C2=A0 =C2=A0InternalPrintMessage ("!!!! Find image based on IP(0x%x= ) ", CurrentEip)= ;

=C2=A0=C2=A0=C2=A0 PdbPointer =3D PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);

=C2=A0=C2=A0 =C2=A0if (PdbP= ointer !=3D NULL) {

=C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0InternalPrintMessage ("%a", PdbPointer);

= =C2=A0=C2=A0=C2=A0 } else {

=C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0InternalPrintM= essage ("(No PDB= ) " );

=C2=A0=C2=A0=C2=A0 }

=C2=A0=C2=A0 =C2=A0InternalPrintMessage (

=C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0" (ImageBase=3D%016lp, EntryPoint=3D%016p) !!!!\n&q= uot;,

=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (= VOID *) Pe32Data,

=

=C2=A0=C2=A0=C2=A0=C2=A0=C2= = =A0 EntryPoint

=C2=A0=C2=A0=C2=A0=C2=A0=C2= = =A0 );



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 i= ssue with a bigger hammer?

 

 

Thanks,



Andrew Fish=


On Mar 24, 2021, at 4:5= 8 AM, Ross Burton <ross@burtonini.= com> wrote:

 

GenFw will embed a NB10 secti= on which contains the path to the input file,
which means the output fil= es have build paths embedded in them.  To reduce
information leakag= e and ensure reproducible builds, pass --zero in release
builds to remov= e this information.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D325= 6
Signed-off-by: Ross Burton <ross.burton@arm.com>
---
OvmfPkg/AmdSev/AmdSevX64.dsc | 1 = +
OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
OvmfPkg/OvmfPkgIa32.ds= c      | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc  &= nbsp;| 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..6= 9a05feea9 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/Amd= Sev/AmdSevX64.dsc
@@ -78,6 +78,7 @@
  GCC:*_*_X64_GENFW_FLA= GS   =3D --keepexceptiontable
  INTEL:*_*_X64_GENFW_= FLAGS =3D --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS= =3D --zero

  #
  # Disable deprecated APIs.<= br>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   =3D --keepexceptiontable
  INTEL= :*_*_X64_GENFW_FLAGS =3D --keepexceptiontable
!endif
+  RELEASE_= *_*_GENFW_FLAGS =3D --zero

  #
  # Disable de= precated APIs.
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa3= 2.dsc
index 1eaf3e99c6..93c209950c 100644
--- a/OvmfPkg/OvmfPkgIa32.d= sc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -80,6 +80,7 @@
!if $(TOOL_CHAI= N_TAG) !=3D "XCODE5" && $(TOOL_CHAIN_TAG) !=3D "CLAN= GPDB"
  GCC:*_*_*_CC_FLAGS      =             &nb= sp;=3D -mno-mmx -mno-sse
!endif
+  RELEASE_*_*_GENFW_FLAGS =3D -= -zero

  #
  # Disable deprecated APIs.
dif= f --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   =3D --keepexceptiontable
  INTEL:*_*_X= 64_GENFW_FLAGS =3D --keepexceptiontable
!endif
+  RELEASE_*_*_GE= NFW_FLAGS =3D --zero

  #
  # Disable deprecat= ed APIs.
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dscindex d4d601b444..f544fb04bf 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++= b/OvmfPkg/OvmfPkgX64.dsc
@@ -84,6 +84,7 @@
  GCC:*_*_X64_G= ENFW_FLAGS   =3D --keepexceptiontable
  INTEL:*_*_X6= 4_GENFW_FLAGS =3D --keepexceptiontable
!endif
+  RELEASE_*_*_GEN= FW_FLAGS =3D --zero

  #
  # Disable deprecate= d 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 &n= bsp; =3D --keepexceptiontable
  INTEL:*_*_X64_GENFW_FLAGS= =3D --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS =3D = --zero

  #
  # Disable deprecated APIs.
--=
2.25.1





 

<= /html> ------=_NextPart_000_00B4_01D72161.0A39BF60--