[PATCH] ArmPkg/ArmLib ARM: set .fpu to let Clang 7 assemble ArmV7Support.S

[PATCH] ArmPkg/ArmLib ARM: set .fpu to let Clang 7 assemble ArmV7Support.S

[Ard Biesheuvel]

Clang 7 complains about the vmsr instruction in ArmV7Support.S,
which is only available on cores that implement some flavour of
VFP. So set the .fpu to NEON like we do in some other places.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
index 281499b46cbc..1808962ee3e2 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
@@ -268,6 +268,7 @@ ASM_FUNC(ArmEnableVFP)
 #ifndef __clang__
   mcr     p10,#0x7,r0,c8,c0,#0
 #else
+  .fpu    neon
   vmsr    fpexc, r0
 #endif
   bx      lr
-- 
2.19.2

Re: [PATCH] ArmPkg/ArmLib ARM: set .fpu to let Clang 7 assemble ArmV7Support.S

[Leif Lindholm]

On Thu, Dec 20, 2018 at 08:16:53PM +0100, Ard Biesheuvel wrote:
> Clang 7 complains about the vmsr instruction in ArmV7Support.S,
> which is only available on cores that implement some flavour of
> VFP. So set the .fpu to NEON like we do in some other places.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
> index 281499b46cbc..1808962ee3e2 100644
> --- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
> +++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
> @@ -268,6 +268,7 @@ ASM_FUNC(ArmEnableVFP)
>  #ifndef __clang__
>    mcr     p10,#0x7,r0,c8,c0,#0
>  #else
> +  .fpu    neon
>    vmsr    fpexc, r0
>  #endif

No objection from me. But I would point out that the special clang
filtering here could possibly be dropped.

I mean, theoretically someone could have an even older binutils, but
Linaro GCC 4.8-2013.05 will happily assemble the clang side of this
conditional.

/
    Leif

Re: [PATCH] ArmPkg/ArmLib ARM: set .fpu to let Clang 7 assemble ArmV7Support.S

[Ard Biesheuvel]

On Fri, 21 Dec 2018 at 18:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Dec 20, 2018 at 08:16:53PM +0100, Ard Biesheuvel wrote:
> > Clang 7 complains about the vmsr instruction in ArmV7Support.S,
> > which is only available on cores that implement some flavour of
> > VFP. So set the .fpu to NEON like we do in some other places.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
> > index 281499b46cbc..1808962ee3e2 100644
> > --- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
> > +++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
> > @@ -268,6 +268,7 @@ ASM_FUNC(ArmEnableVFP)
> >  #ifndef __clang__
> >    mcr     p10,#0x7,r0,c8,c0,#0
> >  #else
> > +  .fpu    neon
> >    vmsr    fpexc, r0
> >  #endif
>
> No objection from me. But I would point out that the special clang
> filtering here could possibly be dropped.
>
> I mean, theoretically someone could have an even older binutils, but
> Linaro GCC 4.8-2013.05 will happily assemble the clang side of this
> conditional.
>

Yeah, but that would be a separate patch, no?

Re: [PATCH] ArmPkg/ArmLib ARM: set .fpu to let Clang 7 assemble ArmV7Support.S

[Leif Lindholm]

On Fri, Dec 21, 2018 at 07:00:04PM +0100, Ard Biesheuvel wrote:
> On Fri, 21 Dec 2018 at 18:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Thu, Dec 20, 2018 at 08:16:53PM +0100, Ard Biesheuvel wrote:
> > > Clang 7 complains about the vmsr instruction in ArmV7Support.S,
> > > which is only available on cores that implement some flavour of
> > > VFP. So set the .fpu to NEON like we do in some other places.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
> > > index 281499b46cbc..1808962ee3e2 100644
> > > --- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
> > > +++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
> > > @@ -268,6 +268,7 @@ ASM_FUNC(ArmEnableVFP)
> > >  #ifndef __clang__
> > >    mcr     p10,#0x7,r0,c8,c0,#0
> > >  #else
> > > +  .fpu    neon
> > >    vmsr    fpexc, r0
> > >  #endif
> >
> > No objection from me. But I would point out that the special clang
> > filtering here could possibly be dropped.
> >
> > I mean, theoretically someone could have an even older binutils, but
> > Linaro GCC 4.8-2013.05 will happily assemble the clang side of this
> > conditional.
> 
> Yeah, but that would be a separate patch, no?

Oh, sure. Just, it would be nice if that could precede this change.

That said, the other two .fpu neon instances in edk2 are both hidden
behind #if !defined(__APPLE__). Should we check with Andrew
(post-wilderbeest) if this is something we need to keep (and if so add
it here) or whether we can delete it across the board?

/
    Leif