Re: [PATCH 2/3] ARM/Assembler: Correct syntax from RVCT for MSFT

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Thu, 19 Sep 2019 at 17:31, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Sep 19, 2019 at 03:36:50PM +0300, Ard Biesheuvel wrote:
> > On Thu, 19 Sep 2019 at 14:25, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > > > The problem is that the first branch instruction is patched into the
> > > > FV files by the BaseTools, and so the startup code is entered in ARM
> > > > mode by default.
> > > >
> > > > So that means we'll either have to
> > > > 1) switch to ARM mode
> > > > 2) emit one branch instruction
> > > > 3) switch back to Thumb mode
> > >
> > > I was thinking more like tying down the entry function (or as I said,
> > > the whole file) as ARM, then letting the toolchain decide for the bits
> > > where we don't have instuction-set dependent ABIs.
> > >
> > > > 4) fix up all the code so it assembles in Thumb mode
> > >
> > > Which is what Baptiste has done in this set.
> > >
> > > > 1) switch to ARM mode
> > >
> > > In all 48 files (+3 in edk2-platforms), or just the ones where not
> > > doing so triggers build errors? Currently.
> > >
> > > I'm OK with restricting ourselves to just setting ARM in the
> > > triggering files for simplicity, especially in order to streamline the
> > > toolchain migration from RVCT to VS (and the subsequent purge of PVCT
> > > support). I'm not seeing it as a solution.
> >
> > As long as we align the .asm files with the .S files, I am fine with
> > that. And since we focus on v7+ only these days, I'm fine with
> > changing existing ARM files to Thumb2.
>
> But I don't see us following this pattern today, for .S files.
>
> Frankly, looking at it, I can't quite manage to untangle how we end up
> generating A32 for .asm in the first place. Is it just the toolchain
> default?
>
> If we look for example at the post-build version of
> Build/ArmVirtQemu-ARM/RELEASE_GCC5/ARM/ArmPkg/Library/ArmHvcLib/ArmHvcLib/OUTPUT/Arm/ArmHvc.iii
> , there is no .arm directive.
>
> That gets assembled into
> Build/ArmVirtQemu-ARM/RELEASE_GCC5/ARM/ArmPkg/Library/ArmHvcLib/ArmHvcLib/OUTPUT/Arm/ArmHvc.obj
> by the step
> ---
> "arm-linux-gnueabihf-gcc"  -march=armv7-a -c -x assembler -imacros
> /work/git/tianocore/Build/ArmVirtQemu-ARM/RELEASE_GCC5/ARM/ArmPkg/Library/ArmHvcLib/ArmHvcLib/DEBUG/AutoGen.h
> -mlittle-endian -o
> /work/git/tianocore/Build/ArmVirtQemu-ARM/RELEASE_GCC5/ARM/ArmPkg/Library/ArmHvcLib/ArmHvcLib/OUTPUT/Arm/ArmHvc.obj
> -I/work/git/edk2/ArmPkg/Library/ArmHvcLib/Arm
> -I/work/git/edk2/ArmPkg/Library/ArmHvcLib
> -I/work/git/tianocore/Build/ArmVirtQemu-ARM/RELEASE_GCC5/ARM/ArmPkg/Library/ArmHvcLib/ArmHvcLib/DEBUG
> -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include
> -I/work/git/edk2/MdePkg/Include/Arm -I/work/git/edk2/ArmPkg
> -I/work/git/edk2/ArmPkg/Include /work/git/tianocore/Build/ArmVirtQemu-ARM/RELEASE_GCC5/ARM/ArmPkg/Library/ArmHvcLib/ArmHvcLib/OUTPUT/Arm/ArmHvc.iii
> ---
> (Adding hilarity to the situation is the bit where we, when
> preprocessing the file, explicitly pass -mthumb on the command line -
> which is obviously ignored at that point.)
>
> Fwiw, yes, it appears at least the Debian toolchain defaults to
> assembling A32 if no .thumb directive is explicitly in place.
>

Yes, that appears to be what happens. Since we use GCC for both
compiling and linker, but invoke the assembler directly (and don't
pass the C flags in this case), we end up with binutils using ARM by
default.

Note that the BaseMemoryLibOptDxe code uses thumb explicitly, but my
main concern is about the startup code we have in PrePi and PrePeiCore
etc

> > What I don't want is a situation where the toolchain's default decides
> > how code is assembled, since that means you have to test all your code
> > changes twice.
>
> And to me it looks like we avoid that problem today only by accident -
> we are already relying on toolchain defaults for GCC.
>

Yup

> Anyway, rewinding this about 27 steps ...
> Are you OK with Baptiste going ahead and adding the ARM statement
> *only* to the files requiring it to build[1] - and the one(s) requiring
> it to operate properly (could you point it/them out please)?
>

The correctness concern applies to files called ModuleEntryPoint.S,
but also to exception handling code that drops in and out of different
modes, and may manipulate exception return addresses etc. If we happen
to always compile that code to ARM today, I strongly suggest we keep
doing that for VS as well.

> [1]
> Then the next question is whether he should also add it to the files
> where that avoids the need for (some) refactoring of code, or whether
> he should simply mirror the changes to .asm into .S?
>

I think all of our 32-bit ARM .S files should have an explicit .arm
directive unless a .thumb directive is already in place.

> The "we must tie down so that each .asm/.S file is only ever built to
> a single instruction set" work is a substantial enough task, that I
> hope we can undertake at a later date rather that rolling it into this
> one.
>

See the above - I think it is fairly straight-forward.