On 07/27/15 20:31, Scott Duplichan wrote:
> Andrew Fish [mailto:af...@apple.com] wrote:
>
> ]Sent: Friday, July 24, 2015 12:21 PM
> ]To: David Woodhouse <dw...@infradead.org>
> ]Cc: Jordan Justen <jordan.l.jus...@intel.com>; edk2-de...@ml01.01.org;
> Laszlo Ersek <ler...@redhat.com>; ]Paolo Bonzini <pbonz...@redhat.com>
> ]Subject: Re: [edk2] Enable optimization for gcc x64 builds
> ]
> ]
> ]> On Jul 24, 2015, at 3:33 AM, David Woodhouse <dw...@infradead.org> wrote:
> ]>
> ]> On Fri, 2015-07-24 at 00:08 +0200, Laszlo Ersek wrote:
> ]>>
> ]>>> Any non-EFIAPI function using varargs is broken, isn't it?
> ]>>
> ]>> Correct, with one tweak: any non-EFIAPI function accessing varargs with
> ]>> the VA_* macros from Base.h is broken.
> ]>
> ]> Can we *make* them break? I don't know, something like 'volatile
> ]> unsigned long dummy_ecx __asm__("ecx")' where ECX is a reserved
> ]> register in the SYSV ABI but not MS?
> ]>
> ]> I know, that's a *really* bad example and I don't think you can even
> ]> assign variables to registers like that any more. But it's just an
> ]> illustration; can we come up with *something* that makes the VA_LIST
> ]> macro break at *build* time from a non-EFIAPI function? Ideally on
> ]> *all* builds, but at least for x86_64 ELF.
> ]>
> ]> An artificial way to keep us honest w.r.t. EFIAPI annotations would
> ]> also be nice, if Jordan insists on keeping it working. I wonder if we
> ]> can put them in a separate section, and have a script post-process the
> ]> objects looking for discrepancies? I also thought about prepending
> ]> something to the function names, but I can't see how that could be made
> ]> to work. Any other ideas?
> ]>
> ]
> ]David,
> ]
> ]I like the creativity. The problem is optimization. Functions can get
> inlined, tail called, or the calling ]conventions can be made up by the
> compiler as an optimization. So in general you can’t assume any behavior ]is
> illegal. The flip side of this is I use a debugger that has no clue what
> EFIAPI is for x86_64, but it ]works fine as the debugger can’t make any
> assumptions due to the compiler making optimizations.
> ]
> ]The main reason we don’t map EFIAPI to anything is that the compiler is
> smart enough to know that a ]function is getting exported in some fashion and
> can’t thus can’t be optimized, and must follow the ABI. ]Maybe it would be
> possible to have the compiler warn if an ABI function is not decorated with
> EFIAPI, it ]would catch all errors, but it may get a lot of false positives
> too (might not even be possible at -O0)?
> ]
> ]Hey but if you get around to hacking the compiler it would be awesome to add
> support for the edk2 Print ]format strings. That way we would get type
> warnings if the arguments don’t match the format specifiers, ]just like
> printf.
>
> Argument checking for print formats is a great idea. I gave it a try and
> testing
> with DEBUG(()) finds plenty of format problems. Some minor, some serious.
> Probably
> the biggest EDK2 print format problem is with INTN/UINTN. If I am not
> mistaken, the
> only way to print UINTN values that works for both 32-bit and 64-bit EDK2 is:
>
> DEBUG (("UINTN values: %lx %lx\n", (UINT64)foo1, (UINT64)foo2);
You are not mistaken. Two points:
- I generally prefer to use the "L" length modifier, not the "l" one.
The reason is that "l" is defined by ISO C, and it is associated with
"long", whereas in edk2 "l" is associated with UINT64. I find that
misleading. For that reason I prefer "L" in this role, because ISO C
doesn't define the L length modifier for integer conversion
specifiers. For me it disambiguates the conversion.
- Recently %u has been implemented in PrintLib; a very welcome feature.
We can print unsigned integers in decimal now, without fear of
wraparound (and negative values appearing in the output). So, "%Lu"
is my preference in this case (as long as we want decimal).
>
> That is because while UINTN can be either 32 or 64 bit, EDK2 has no length
> prefix
> that automatically handles the difference. I suppose a macro similar to C99
> PRIxPTR
> could be an alternative to the type cast method, but that would have to be
> added.
> That method would be more efficient for 32-bit code than the type cast method.
>
> The gcc -Wformat messages are usually clear, but I found one misleading:
> format '%g' expects argument of type 'void *', but argument 4 has type
> 'EFI_GUID {aka struct <anonymous>
> In this case, the entire GUID struct is being passed instead of the GUID
> address
> (due to missing '&').
>
> The checking also catches some obvious typos:
> MdeModulePkg\Core\Dxe\Image\Image.c:1706:29: warning: unknown conversion type
> character 'h'
> MdeModulePkg\Core\Dxe\Mem\Pool.c:435:7: warning: unknown conversion type
> character ','
>
> The full lists are attached (made with GCC_EXTRA_CC_FLAGS=-Wno-error). Let me
> know of
> anything wrong you see. I am going to try to perfect this checking and put it
> into the
> Windows hosted gcc tool chains.
I'll try to go through the list soonish.
Thanks!
Laszlo
>
> Thanks,
> Scott
>
>
> ]For gcc/clang this syntax enables all the printf format string vs. argument
> warns for my_printf that exist ]with printf.
> ]int my_printf (void *my_object, const char *my_format, ...) __attribute__
> ((format (printf, 2, 3)));
> ]
> ]It might be possible to add a format attribute to a function that could
> check the calling conventions, but ]the optimizations may happen too late to
> help?
> ]
> ]Thanks,
> ]
> ]Andrew Fish
> ]
> ]> --
> ]> dwmw2
> ]>
> ]> --
> ]> David Woodhouse Open Source Technology Centre
> ]> david.woodho...@intel.com Intel Corporation
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
