Ard: The error message shows the error happens in the first loop in VA_START and VA_END. Could we update the logic to use single va list and verify it again? If the single va list passes, it can prove your suspect. If the single va list still fail, it may be other issue. I think we need to know which usage will cause break with your patches.
Thanks Liming > -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Friday, July 15, 2016 2:07 PM > To: Laszlo Ersek <[email protected]> > Cc: Shi, Steven <[email protected]>; Zhu, Yonghong > <[email protected]>; Gao, Liming <[email protected]>; Kinney, > Michael D <[email protected]>; Justen, Jordan L > <[email protected]>; [email protected]; [email protected] > <[email protected]> > Subject: Re: [PATCH 0/5] MdePkg BaseTools: GCC optimization for X64 > > On 15 July 2016 at 01:27, Laszlo Ersek <[email protected]> wrote: > > On 07/14/16 15:16, Ard Biesheuvel wrote: > >> This series is not an attempt to steal Steven's thunder. I was merely > >> inspired by some of the changes he is proposing for GCC 5 and Clang > >> support, which I noticed would be applicable to older versions of GCC > >> as well. > >> > >> The first patch fixes the issue that __builtin_unreachable() is not > >> implemented by GCC 4.4 or earlier. > >> > >> Patch #2 is Steven's patch to switch to the flavor of VA_LIST that is > >> explicitly modeled after the MS implementation. This by itself is an > >> improvement, since the open coded implementation that performs > arithmetic > >> on the address of explicit arguments to obtain the variadic arguments is > >> fragile and difficult to maintain, and should be best avoided. > >> > >> Patch #3 enabled -O2 optimization for X64/RELEASE all the way back to > GCC44. > >> I tested this change with both Ovmf and EmulatorPkg built in various ways > >> and with various versions, with the caveat that I did not always use a > matching > >> binutils (i.e., of the same era). Since the issues this series deal with > >> are > >> all code generation issues, I think this is reasonable, but more testing > would > >> be appreciated. > >> > >> Patch #4 applies the visibility 'hidden' GCC pragma globally. Please refer > >> to > >> the commit log for the motivation. > >> > >> Patch #5 switches GCC/X64 to the PIC small code model, which results in > smaller > >> code. > >> > >> Ard Biesheuvel (4): > >> MdePkg: avoid __builtin_unreachable() on GCC v4.4 > >> BaseTools/tools_def: enable O2 optimization for GCC X64 builds > >> MdePkg X64: force 'hidden' visibility when building with -fpic > >> BaseTools/tools_def: switch GCC/X64 to the PIC small model > >> > >> Shi, Steven (1): > >> MdePkg: Enable new MS VA intrinsics for GNUC x86 64bits build > >> > >> BaseTools/Conf/tools_def.template | 18 +++++++----- > >> MdePkg/Include/Base.h | 29 +++++++++++++++++++- > >> MdePkg/Include/X64/ProcessorBind.h | 10 +++++++ > >> 3 files changed, 49 insertions(+), 8 deletions(-) > >> mode change 100644 => 100755 MdePkg/Include/Base.h > >> > > > > So, I did some tests. > > > > Thanks a lot for the awesome work, and for the amount of effort you > have put in. I could never have tested this as methodical and with the > same coverage as you have. > > > First, the build tests. I built OVMF 84 times, with the following settings: > > > > * Dimension 1: whether your and Steven's patches were applied or not. > > I take it this means this series only? > > > The basis was always: > > - upstream 04147690b59b, > > - plus my personal patches that I constantly rebase, > > - plus my pending "feat_ctrl_issue97_v3" patches, > > - plus my work-in-progress build fixes for GCCxx and VS2015x86 warnings, > > - plus OpenSSL 1.0.2g. > > > > These patches don't interfere with your and Steven's work. So on top of > > this basis, your and Steven's patches were either applied (rebased) or > > not. This is a multiplier of 2. > > > > * Dimension 2: the GCC toolchain used. Ranging from GCC44 through > GCC49, > > plus gcc-6.1 at the end (using GCC49 settings). Multiplier of 7. > > > > * Dimension 3: DEBUG versus RELEASE build target. Factor of 2. > > > > * Dimension 4: the OVMF platform. I used the following command lines: > > > > - build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -D SMM_REQUIRE \ > > -D SECURE_BOOT_ENABLE -D HTTP_BOOT_ENABLE > > - build -a IA32 -a X64 -p OvmfPkg/OvmfPkgIa32X64.dsc -D SMM_REQUIRE \ > > -D SECURE_BOOT_ENABLE -D HTTP_BOOT_ENABLE > > - build -a X64 -p OvmfPkg/OvmfPkgX64.dsc \ > > -D SECURE_BOOT_ENABLE -D HTTP_BOOT_ENABLE > > > > As you can see, Ia32 and Ia32X64 are built with the SMM driver stack; > > X64 is built without it. (This is because edk2 S3 doesn't support > > X64+SMM at the moment, so I either have to build X64 without SMM, or > > disable S3 support on the QEMU command line. I find the latter more > > inconvenient, plus Ia32X64 is just fine for 64-bit OS + SMM + S3.) > > Anyhow, this is a factor of 3. > > > > For these builds, I collected the log files and the report files. Values > > in dimension 3 are not separated into separate files, because you can > > pass -b DEBUG -b RELEASE at once, and the build utility will do the > > right thing -- it will build both targets in succession. However, the > > log file and the report file each will contain both DEBUG and RELEASE > > output. > > > > For the builds I also collected the OVMF_CODE.fd firmware binaries. > > > > The good news is that all of the builds succeeded. You can draw size > > comparisons from the build log files: see SECFV, PEIFV and DXEFV for > > uncompressed sizes, and FVMAIN_COMPACT for the compressed size of > > PEIFV+DXEFV together. (Thanks Mike for the reminder!) > > > > I'll send you the link to the results archive off-list. (I tend not to > > post stuff publicly that I cannot review in detail, purely out of > > privacy concerns, and this archive is just too big for that. For example > > I dislike disclosing full pathnames from my system: in commit messages I > > always edit them out.) > > > > Clearly if you distill any numbers from the archive, you are going to > > review those in detail (and hopefully you won't disclose the entire > > archive), so it should suffice to constrain my privacy concerns to you > > personally. :) > > > > Noted. > > > -o- > > > > Second, the run test. I executed two test scenarios here. For these, the > > following dimensions were fixed: > > > > - Dimension 1: your and Steven's patches were always applied. > > - Dimension 2: the toolchain was always GCC48. > > - Dimension 4: the OVMF platform was always X64. > > > > * The first runtime test was repeating this test: > > > > http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10963 > > > > Obviously, I modified this test a little bit: I removed the -Os / -O0 > > parts and differences, and simply built the UEFI binary with -b DEBUG > > and -b RELEASE (--> Dimension 3 was variable). The bug that I had > > originally reported in that thread did *not* reproduce this time. The -b > > RELEASE binary worked fine. > > > > Great! > > > * However, then I built my program -- find it attached -- with -b DEBUG > > and -b RELEASE too, and the -b RELEASE binary is broken. Here's the > > difference in output: > > > > FS2:\> DebugEnrollDefaultKeys.efi > > info: SetupMode=1 SecureBoot=0 SecureBootEnable=1 CustomMode=1 > VendorKeys=0 > > info: SetupMode=0 SecureBoot=1 SecureBootEnable=1 CustomMode=0 > VendorKeys=0 > > info: success > > > > versus > > > > FS2:\> OptEnrollDefaultKeys.efi > > info: SetupMode=1 SecureBoot=0 SecureBootEnable=1 CustomMode=1 > VendorKeys=0 > > error: EnrollListOfX509Certs("db", > > D719B2CB-3D3A-4596-A3BC-DAD00E67656F): Invalid Parameter > > > > I don't know why this happens, but it definitely relates to varargs -- > > the program uses them liberally. > > > > If this code ultimately uses __builtin_ms_va_lists with -O2 and fails, > it makes sense to report it to the GCC maintainers (assuming we can > create a test case). I noticed that this code iterates over the same > VA_LIST twice, I wonder how well that was tested ... _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
