From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.334.1616692816049465434 for ; Thu, 25 Mar 2021 10:20:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dxZuS0X9; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1616692814; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YFOhF/PhqchjSnrp5z9b4cu6yIUzJxc4cU+W1SwXfT0=; b=dxZuS0X9AI8vzBQKcPl7KvkgQNxT0pOGtiNvclfAuQ6hYZyDoJRRQgAPKu37VPFsqOtl/o 1tUQDvIk5Q8bnKPAXADjMlS3vUr3IyqISNRzIWIouRvVZL77Cd/JrP4SDJEFZl1DSpmyqG t6P5MV7guaJPOvutH1kW8YDxwVtS/io= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-551-c_pk5ymQMB2dZphqSXR7PA-1; Thu, 25 Mar 2021 13:20:10 -0400 X-MC-Unique: c_pk5ymQMB2dZphqSXR7PA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A9DBF100960C; Thu, 25 Mar 2021 17:19:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-200.ams2.redhat.com [10.36.114.200]) by smtp.corp.redhat.com (Postfix) with ESMTP id E524460854; Thu, 25 Mar 2021 17:19:51 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds To: Andrew Fish References: <20210324115819.605436-1-ross.burton@arm.com> From: "Laszlo Ersek" Cc: edk2-devel-groups-io , ross@burtonini.com, ross.burton@arm.com, "Ard Biesheuvel (TianoCore)" Message-ID: <95cbe27d-afbe-84f3-3473-84456379ebe2@redhat.com> Date: Thu, 25 Mar 2021 18:19:51 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 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 >> --- >> 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 >> >> >> >> >> >> > >