From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang Date: Thu, 11 Feb 2016 03:45:30 -0700 Message-ID: <56BC745A02000078000D0EE9@prv-mh.provo.novell.com> References: <1455048108-5045-1-git-send-email-andrew.cooper3@citrix.com> <1455048108-5045-9-git-send-email-andrew.cooper3@citrix.com> <56BB49CA02000078000D090C@prv-mh.provo.novell.com> <56BB3DEF.9040801@citrix.com> <56BB8B56.4060703@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56BB8B56.4060703@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Xen-devel List-Id: xen-devel@lists.xenproject.org >>> On 10.02.16 at 20:11, wrote: > On 10/02/16 13:41, Andrew Cooper wrote: >> On 10/02/16 13:31, Jan Beulich wrote: >>>>>> On 09.02.16 at 21:01, wrote: >>>> Signed-off-by: Andrew Cooper >>>> --- >>>> CC: Jan Beulich >>>> >>>> What is the GCC version check supposed to be achieving here? GCC has >>>> supported this syntax since 3.0 >>> This is best answered by looking at where we've got these headers >>> from - the gnu-efi package. It has ... >>> >>>> --- a/xen/include/asm-x86/x86_64/efibind.h >>>> +++ b/xen/include/asm-x86/x86_64/efibind.h >>>> @@ -274,7 +274,7 @@ typedef uint64_t UINTN; >>>> #endif >>>> #endif >>>> >>>> -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) >>>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) || __clang__ >>>> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__) >>>> #else >>>> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */ >>> /* for x86_64, EFI_FUNCTION_WRAPPER must be defined */ >>> #if defined(HAVE_USE_MS_ABI) >>> #define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__) >>> #else >>> UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...); >>> #endif >>> #define EFI_FUNCTION __attribute__((ms_abi)) >>> >>> I think this makes clear that the needed feature here is the >>> attribute, which certainly wasn't available in older gcc. >>> >>> With that the question is whether the Clang case won't also need >>> a version check, since I can't imagine them having supported this >>> prior to gcc introducing it. >> Clang has an substantially more sane way than GCC of checking for >> individual features. I will introduce and use the >> __has_{attribute,feature}() infrastructure to tests like this. >> >> Experimentally, Clang 3.5 does have ms_abi support > > Looking at it further, this entire block is unsed. Nothing in tree > refers to uefi_call_wrapper() or EFI_FUNCTION_WRAPPER other than this > small bit; all declarations use EFIABI directly. > > i.e. this: > > diff --git a/xen/include/asm-x86/x86_64/efibind.h > b/xen/include/asm-x86/x86_64/efibind.h > index af5f424..b013db1 100644 > --- a/xen/include/asm-x86/x86_64/efibind.h > +++ b/xen/include/asm-x86/x86_64/efibind.h > @@ -274,17 +274,6 @@ typedef uint64_t UINTN; > #endif > #endif > > -#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) > -#define uefi_call_wrapper(func, va_num, ...) func(__VA_ARGS__) > -#else > -/* for x86_64, EFI_FUNCTION_WRAPPER must be defined */ > -#ifdef EFI_FUNCTION_WRAPPER > -UINTN uefi_call_wrapper(void *func, unsigned long va_num, ...); > -#else > -#error "EFI_FUNCTION_WRAPPER must be defined for x86_64 architecture" > -#endif > -#endif > - > #ifdef _MSC_EXTENSIONS > #pragma warning ( disable : 4731 ) // Suppress warnings about > modification of EBP > #endif > > works correctly for GCC and clang. Would dropping this completely be > acceptable? We perhaps might as well; I had mainly kept it to stay as close to the original header as possible (without leaving around a latent build breakage). Jan