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

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

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.

> 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.

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)?

[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?

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.

/
    Leif