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

[Leif Lindholm <leif.lindholm@linaro.org>]

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