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: <e92ee9817a31e24eb0585fdf735412f51cb77...@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.kin...@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 <af...@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 <gaolim...@byosoft.com.cn >>> <mailto:gaolim...@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>>; r...@burtonini.com >>> <mailto:r...@burtonini.com> >>> 抄送: ler...@redhat.com <mailto:ler...@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 <r...@burtonini.com >>>> <mailto:r...@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.bur...@arm.com >>>> <mailto:ross.bur...@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 >>>> >>>> >>>> >>>> >>>> >>>> >>> >>> >>> >> > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73280): https://edk2.groups.io/g/devel/message/73280 Mute This Topic: https://groups.io/mt/81594755/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
