Re: [Xen-devel] [PATCH 8/8] x86/efi: Generate uefi_call_wrapper() when compiling with clang

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.02.16 at 20:11, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>>
>>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel