| Message ID | 20210203175805.86465-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.15] x86/efi: enable MS ABI attribute on clang | expand |
On 03/02/2021 17:58, Roger Pau Monne wrote: > Or else the EFI service calls will use the wrong calling convention. > > The __ms_abi__ attribute is available on all supported versions of > clang. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > Cc: Ian Jackson <iwj@xenproject.org> > > Without this a Xen built with clang won't be able to correctly use the > EFI services, leading to weird messages from the firmware and crashes. > The impact of this fix for GCC users is exactly 0, and will fix the > build on clang. DYM "fix the compiled binary"? The build on clang isn't broken atm, but it provably has the wrong ABI. ~Andrew
On 03.02.2021 18:58, Roger Pau Monne wrote: > --- a/xen/include/asm-x86/x86_64/efibind.h > +++ b/xen/include/asm-x86/x86_64/efibind.h > @@ -172,7 +172,7 @@ typedef uint64_t UINTN; > #ifndef EFIAPI // Forces EFI calling conventions reguardless of compiler options > #ifdef _MSC_EXTENSIONS > #define EFIAPI __cdecl // Force C calling convention for Microsoft C compiler > - #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) > + #elif __clang__ || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) > #define EFIAPI __attribute__((__ms_abi__)) // Force Microsoft ABI > #else > #define EFIAPI // Substitute expresion to force C calling convention > So the problem is that some capable Clang versions set too low a __GNUC_MINOR__ (I'm observing 2 alongside __GNUC__ being 4 on Clang5). The way the description and change are written made me rather imply __GNUC__ to not be available at all, which I then thought can't be the case because iirc we use it elsewhere. Jan
On Thu, Feb 04, 2021 at 11:27:17AM +0100, Jan Beulich wrote: > On 03.02.2021 18:58, Roger Pau Monne wrote: > > --- a/xen/include/asm-x86/x86_64/efibind.h > > +++ b/xen/include/asm-x86/x86_64/efibind.h > > @@ -172,7 +172,7 @@ typedef uint64_t UINTN; > > #ifndef EFIAPI // Forces EFI calling conventions reguardless of compiler options > > #ifdef _MSC_EXTENSIONS > > #define EFIAPI __cdecl // Force C calling convention for Microsoft C compiler > > - #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) > > + #elif __clang__ || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) > > #define EFIAPI __attribute__((__ms_abi__)) // Force Microsoft ABI > > #else > > #define EFIAPI // Substitute expresion to force C calling convention > > > > So the problem is that some capable Clang versions set too low > a __GNUC_MINOR__ (I'm observing 2 alongside __GNUC__ being 4 > on Clang5). The way the description and change are written > made me rather imply __GNUC__ to not be available at all, > which I then thought can't be the case because iirc we use it > elsewhere. Yes, I also see 4.2 on Clang 11. Do you want me to expand the description by adding: "Add a specific Clang check because the GCC version reported by Clang is below the required 4.4 to use the __ms_abi__ attribute." Thanks, Roger.
On 04.02.2021 11:51, Roger Pau Monné wrote: > On Thu, Feb 04, 2021 at 11:27:17AM +0100, Jan Beulich wrote: >> On 03.02.2021 18:58, Roger Pau Monne wrote: >>> --- a/xen/include/asm-x86/x86_64/efibind.h >>> +++ b/xen/include/asm-x86/x86_64/efibind.h >>> @@ -172,7 +172,7 @@ typedef uint64_t UINTN; >>> #ifndef EFIAPI // Forces EFI calling conventions reguardless of compiler options >>> #ifdef _MSC_EXTENSIONS >>> #define EFIAPI __cdecl // Force C calling convention for Microsoft C compiler >>> - #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) >>> + #elif __clang__ || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) >>> #define EFIAPI __attribute__((__ms_abi__)) // Force Microsoft ABI >>> #else >>> #define EFIAPI // Substitute expresion to force C calling convention >>> >> >> So the problem is that some capable Clang versions set too low >> a __GNUC_MINOR__ (I'm observing 2 alongside __GNUC__ being 4 >> on Clang5). The way the description and change are written >> made me rather imply __GNUC__ to not be available at all, >> which I then thought can't be the case because iirc we use it >> elsewhere. > > Yes, I also see 4.2 on Clang 11. > > Do you want me to expand the description by adding: > > "Add a specific Clang check because the GCC version reported by Clang > is below the required 4.4 to use the __ms_abi__ attribute." I guess adding this is easy enough when done while committing. Thanks for the addition. Jan
Roger Pau Monne writes ("[PATCH for-4.15] x86/efi: enable MS ABI attribute on clang"):
> Or else the EFI service calls will use the wrong calling convention.
>
> The __ms_abi__ attribute is available on all supported versions of
> clang.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
>
> Without this a Xen built with clang won't be able to correctly use the
> EFI services, leading to weird messages from the firmware and crashes.
> The impact of this fix for GCC users is exactly 0, and will fix the
> build on clang.
Reviewed-by: Ian Jackson <iwj@xenproject.org>
> The biggest fallout from this could be using the attribute on a
> compiler that doesn't support it, which would translate into a build
> failure, but the gitlab tests have shown no issues.
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Thanks for the thorough attention to the release question in your
mails. You're making my work very easy :-).
Ian.
diff --git a/xen/include/asm-x86/x86_64/efibind.h b/xen/include/asm-x86/x86_64/efibind.h index b013db175d..ddcfae07ec 100644 --- a/xen/include/asm-x86/x86_64/efibind.h +++ b/xen/include/asm-x86/x86_64/efibind.h @@ -172,7 +172,7 @@ typedef uint64_t UINTN; #ifndef EFIAPI // Forces EFI calling conventions reguardless of compiler options #ifdef _MSC_EXTENSIONS #define EFIAPI __cdecl // Force C calling convention for Microsoft C compiler - #elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) + #elif __clang__ || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4) #define EFIAPI __attribute__((__ms_abi__)) // Force Microsoft ABI #else #define EFIAPI // Substitute expresion to force C calling convention
Or else the EFI service calls will use the wrong calling convention. The __ms_abi__ attribute is available on all supported versions of clang. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Ian Jackson <iwj@xenproject.org> Without this a Xen built with clang won't be able to correctly use the EFI services, leading to weird messages from the firmware and crashes. The impact of this fix for GCC users is exactly 0, and will fix the build on clang. The biggest fallout from this could be using the attribute on a compiler that doesn't support it, which would translate into a build failure, but the gitlab tests have shown no issues. --- xen/include/asm-x86/x86_64/efibind.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)