On 07/22/15 23:07, David Woodhouse wrote: > On Tue, 2015-06-23 at 09:54 -0400, Peter Jones wrote: >> On Tue, Jun 23, 2015 at 03:34:25PM +0200, Laszlo Ersek wrote: >>> Git commit f93f78ea70 (SVN r17633), with subject "CryptoPkg: Update >>> openssl patch file from 0.9.8zf to 1.0.2c", replaced the file >>> >>> CryptoPkg/Library/OpensslLib/EDKII_openssl-0.9.8zf.patch >>> >>> with >>> >>> CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2c.patch >>> >>> In the process, two hunks were lost that used to add EFIAPI to the >>> declaration of the variadic function ERR_add_error_data(). >>> >>> The VA_START() macro, from "MdePkg/Include/Base.h", expands to an >>> EFIAPI-dependent implementation when >>> >>> !defined(__CC_ARM) && (!defined(__GNUC__) || >>> defined(NO_BUILTIN_VA_FUNCS)) >>> >>> Under such circumstances, the va_start() macro invocation in >>> ERR_add_error_data() -- which is translated to VA_START() by >>> "CryptoPkg/Include/OpenSslSupport.h" -- results in EFIAPI-dependent code, >>> but callers of the function pass the arguments incorrectly, because the >>> declaration doesn't state EFIAPI. >>> >>> This leads to crashes when ERR_add_error_vdata(), called by >>> ERR_add_error_data(), tries to access the arguments forwarded to it. >>> >>> Restore the missing hunk from before SVN r17633. >>> >>> Cc: Qin Long <qin.l...@intel.com> >>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >>> Cc: Gary Ching-Pang Lin <g...@suse.com> >>> Cc: Peter Jones <pjo...@redhat.com> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <ler...@redhat.com> >> >> Yeah, EFIAPI to get the right calling conventions on x86_64 is >> definitely the right thing here. Is this what was breaking shim for >> you? >> >> It's mildly annoying that EFIAPI is pretty much 100% about ABI calling >> conventions and has nothing to do with actual API, but that ship has >> long past sailed. >> >> Acked-by: Peter Jones <pjo...@redhat.com> > > Alternatively (as I baulk at trying to send that particular one-liner > upstream to OpenSSL), would it be possible to simply *avoid* defining > NO_BUILTIN_VA_FUNCS for the OpenSSL part of the build?
I "guess" it should work at the moment (and -UNO_BUILTIN_VA_FUNCS would be easy enough to add under [BuildOptions] in "CryptoPkg/Library/OpensslLib/OpensslLib.inf"). But, as soon as a "genuinely" EFIAPI function with a variable argument list appeared in "OpensslLib.inf", things would break again. Any particular reason for disliking these hunks of "EDKII_openssl-1.0.2d.patch" (over the other hunks)? ... The best thing would be to avoid NO_BUILTIN_VA_FUNCS *completely*. If we did that, then the macros in question would be defined as: > typedef __builtin_va_list VA_LIST; > > #define VA_START(Marker, Parameter) __builtin_va_start (Marker, Parameter) > > #define VA_ARG(Marker, TYPE) ((sizeof (TYPE) < sizeof (UINTN)) ? > (TYPE)(__builtin_va_arg (Marker, UINTN)) : (TYPE)(__builtin_va_arg (Marker, > TYPE))) > > #define VA_END(Marker) __builtin_va_end (Marker) > > #define VA_COPY(Dest, Start) __builtin_va_copy (Dest, Start) Unfortunately, this is good for *non*-EFIAPI functions only. For EFIAPI functions, you'd have to define them as __builtin_ms_va* (note the _ms). Which, in turn, would be good only for EFIAPI functions. (See "/usr/lib/gcc/*/*/include/cross-stdarg.h". See also <https://bugzilla.redhat.com/show_bug.cgi?id=871889#c3>.) So, this issue is not fixable globally under gcc. With gcc (compiling for x86_64), EFIAPI and non-EFIAPI differ from each other, and the VA_* macros would have to expand to different gcc builtins dependent *on the function*, not some global (or even source file level) #define. And, since the UEFI spec declares a number of functions that take variable arguments, like InstallMultipleProtocolInterfaces(), we are forced to stick with EFIAPI as default, and that compels all other varargs functions (that use the VA_* macros internally) to be EFIAPI too. This could only be avoided if __builtin_va* considered the containing function's calling convention automatically, but that's not going to happen (see bug 871889 again). ... Rather than undefining NO_BUILTIN_VA_FUNCS for openssl, we should instead remove > // > // Map all va_xxxx elements to VA_xxx defined in MdePkg/Include/Base.h > // > #if !defined(__CC_ARM) // if va_list is not already defined > #define va_list VA_LIST > #define va_arg VA_ARG > #define va_start VA_START > #define va_end VA_END from "CryptoPkg/Include/OpenSslSupport.h" (in the gcc case). Because, without the VA_* macros (of which OpenSSL itself contains no instances), there would be no requirement for EFIAPI either. In other words, the EFIAPI hunks in "EDKII_openssl-1.0.2d.patch" are a solution to a problem that "CryptoPkg/Include/OpenSslSupport.h" creates. Thanks Laszlo ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
