On 07/15/16 10:40, Ard Biesheuvel wrote: > On 15 July 2016 at 08:33, Laszlo Ersek <[email protected]> wrote: >> On 07/15/16 08:07, Ard Biesheuvel wrote: >>> On 15 July 2016 at 01:27, Laszlo Ersek <[email protected]> wrote: >> >>>> 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? >> >> Ah, yes, sorry about the ambiguity -- with "your and Steven's patches" I >> meant the five patches in this series -- one patch from Steven, four >> patches from you. >> >>>> * 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 ... >> >> You make a good point about using VA_LIST twice possibly tickling gcc >> the wrong way. Plus, there's one (VOID)VA_ARG() macro invocation in my >> code -- in order to step over some arguments --, which might not be all >> that usual as well. >> >> In any case, the way I use varargs seems to be standards conformant. >> >> With regard to reporting this to gcc developers: I won't try that. I'm >> discouraged by two facts: >> >> * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50818 -- open since >> October 2011. You'll find some recent comments from Steven and David in >> it :) >> >> * http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10963 -- >> shortly after I ran into the -Os corruption issue with GCC48, I created >> and posted this small reproducer. Creating the reproducer wasn't >> trivial. In parallel I sent the same reproducer to one of my (indirect) >> colleagues at Red Hat, who had been a veteran in upstream GCC >> development and maintenance. I also asked him how/where I should report >> the bug. I got no answer from him. >> >> If you'd like to report a GCC bug, please go ahead, but I won't waste my >> time :) >> > > It seems that it is your testcase that is broken: > > --- a/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c > +++ b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c > @@ -663,6 +663,8 @@ EnrollListOfX509Certs ( > UINT8 *Data; > UINT8 *Position; > > + Status = EFI_SUCCESS; > + > // > // compute total size first, for UINT32 range check, and allocation > // > > With that change, everything works fine for me
I agree that proving my code wrong would be the best outcome here, but can you please explain to me on what path we return from EnrollListOfX509Certs() without setting Status? Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
