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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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?

~Andrew