| Message ID | 56BB8B56.4060703@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 10.02.16 at 20:11, <andrew.cooper3@citrix.com> 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, <andrew.cooper3@citrix.com> wrote: >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> --- >>>> CC: Jan Beulich <JBeulich@suse.com> >>>> >>>> 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
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