On 15 July 2016 at 11:06, Laszlo Ersek <[email protected]> wrote: > On 07/15/16 10:58, Ard Biesheuvel wrote: >> On 15 July 2016 at 10:48, Laszlo Ersek <[email protected]> wrote: >>> 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? >>> >> >> Well, it seems that in the success case, we are supposed to end up at line >> 692 >> >> if (EFI_ERROR (Status)) { >> goto Out; >> } >> >> and expect the condition to be FALSE. If we have successfully tallied >> the size of at least one cert, 'DataSize == sizeof *SingleHeader' will >> be FALSE, and so Status will never have been assigned a value. >> Apparently, the optimizer sees some kind of undefined behavior here >> which it can exploit to bail unconditionally (or Status happens to be >> 0 under -O0) >> > > You are perfectly right. Thank you for catching this! > > And, to our collective relief, this means that there should be no > problem with this series. :) I think I can add my Tested-by: > > Tested-by: Laszlo Ersek <[email protected]> >
Thanks a lot! I do have another question, though. Looking at your logs, I found the following results for the sizes GCC48 before: SECFV [11%Full] 212992 total, 25392 used, 187600 free FVMAIN_COMPACT [63%Full] 1753088 total, 1113976 used, 639112 free DXEFV [56%Full] 10485760 total, 5970624 used, 4515136 free PEIFV [21%Full] 917504 total, 200616 used, 716888 free GCC48 after: SECFV [7%Full] 212992 total, 15728 used, 197264 free FVMAIN_COMPACT [65%Full] 1753088 total, 1145808 used, 607280 free DXEFV [39%Full] 10485760 total, 4148384 used, 6337376 free PEIFV [14%Full] 917504 total, 131208 used, 786296 free GCC49 before: SECFV [12%Full] 212992 total, 25616 used, 187376 free FVMAIN_COMPACT [64%Full] 1753088 total, 1122760 used, 630328 free DXEFV [58%Full] 10485760 total, 6106048 used, 4379712 free PEIFV [22%Full] 917504 total, 205992 used, 711512 free GCC49 after: SECFV [7%Full] 212992 total, 15824 used, 197168 free FVMAIN_COMPACT [66%Full] 1753088 total, 1166952 used, 586136 free DXEFV [40%Full] 10485760 total, 4225728 used, 6260032 free PEIFV [14%Full] 917504 total, 133672 used, 783832 free So DXEFV is substantially smaller, but FVMAIN_COMPACT ends up slightly bigger. Which one do we care about mostly? I would assume FVMAIN_COMPACT, since that translates into flash footprint directly. So I did a quick test, comparing O0 / O1 / O2 on GCC48, and it seems the sweet spot is O1 (due to lack of support for Os in older GCCs) -O0 SECFV [11%Full] 212992 total, 23536 used, 189456 free FVMAIN_COMPACT [58%Full] 1753088 total, 1016816 used, 736272 free DXEFV [47%Full] 10485760 total, 4978656 used, 5507104 free PEIFV [14%Full] 917504 total, 133328 used, 784176 free -O1 SECFV [6%Full] 212992 total, 14768 used, 198224 free FVMAIN_COMPACT [58%Full] 1753088 total, 1017856 used, 735232 free DXEFV [37%Full] 10485760 total, 3907648 used, 6578112 free PEIFV [10%Full] 917504 total, 95696 used, 821808 free -O2 SECFV [7%Full] 212992 total, 15728 used, 197264 free FVMAIN_COMPACT [63%Full] 1753088 total, 1111144 used, 641944 free DXEFV [38%Full] 10485760 total, 4029984 used, 6455776 free PEIFV [10%Full] 917504 total, 100400 used, 817104 free Anyway, thanks again for your time and effort in testing this, much appreciated. -- Ard. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
