Allow field_10x26_arm.s to compile for ARMv7 architecture

[romanz]

It would allow using optimized field operations on the TREZOR device, which is using ARMv7 Cortex-M4.
Following trezor/trezor-core#500 and part of trezor/trezor-firmware#66.

[sipa]

@laanwj Do you understand the implications of this?

[real-or-random]

For reference, the allowed values are the same as those for -march, see https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html .

[gmaxwell]

My understanding is that this just sets the allowed instructions and sets the arch on the object files and that if it were incompatible the compilation would fail.

[real-or-random]

I have no experience with assembly but that's my understanding, too. Instructions should behave equally on different architectures except that a different set of instructions is available.

[sipa]

That makes sense; concept ACK

[gmaxwell]

NAK, doesn't work: setting arch to "armv7" limits it to thumb and the code is full of non-thumb instructions. Is there some toolchain where this does work? what is it actually doing there?

Fails for me with
gcc version 4.9.2 (Debian 4.9.2-10)
Target: arm-linux-gnueabihf
GNU assembler version 2.25 (arm-linux-gnueabihf) using BFD version (GNU Binutils for Debian) 2.25

and hundreds of errors like
src/asm/field_10x26_arm.s:47: Error: attempt to use an ARM instruction on a Thumb-only processor -- `sub sp,sp,#48'

Setting it to armv4t almost works, failing only on
src/asm/field_10x26_arm.s:473: Error: selected processor does not support ARM mode movw r14,field_R0' src/asm/field_10x26_arm.s:481: Error: selected processor does not support ARM mode ubfx r2,r3,#0,#22'
src/asm/field_10x26_arm.s:486: Error: selected processor does not support ARM mode `movw r14,field_R1<<4' (and the same instructions on other lines)

[real-or-random]

Hm, @romanz managed to build and test this for Trezor. I thought that this is related to "unified syntax", i.e., identical assembler instructions for Thumb/non-thumb: If you write asm, you don't care about how the instruction will be encoded in the end.

But our code turns this on with .syntax unified which is documented since binutils/gas 2.20 at least, and probably available even much earlier:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;f=gas/config/tc-arm.c;h=c19d120533277585363d82db26dec8f9ff603d0f

So if this fails for you on 2.25, I guess my conclusions are wrong.

I'll try to build this on my own later.

[romanz]

@gmaxwell Many thanks for the testing!

I have built the firmware using the Makefile and the ARM toolchain (following the Dockerfile):

$ make clean build_firmware > build.log

The gzipped build.log is attached here.
The assembly file is built using:

$ arm-none-eabi-as -mthumb -mcpu=cortex-m4 -mfloat-abi=hard -mfpu=fpv4-sp-d16 \
  -o build/firmware/vendor/secp256k1-zkp/src/asm/field_10x26_arm.o \
  vendor/secp256k1-zkp/src/asm/field_10x26_arm.s

The versions I am using are:

$ arm-none-eabi-gcc --version
arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2018-q2-update) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907]
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ arm-none-eabi-as --version
GNU assembler (GNU Tools for Arm Embedded Processors 7-2018-q2-update) 2.30.0.20180329
Copyright (C) 2018 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or later.
This program has absolutely no warranty.
This assembler was configured for a target of `arm-none-eabi'.

I also tried to build the assembly using Debian 9 arm-none-eabi-as, and it seems to succeed:

$ sudo apt install -y gcc-arm-none-eabi
$ arm-none-eabi-gcc --version
arm-none-eabi-gcc (15:5.4.1+svn241155-1) 5.4.1 20160919
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ arm-none-eabi-as --version
GNU assembler (2.28-5+9+b3) 2.28
Copyright (C) 2017 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or later.
This program has absolutely no warranty.
This assembler was configured for a target of `arm-none-eabi'.

$ arm-none-eabi-as -mthumb -mcpu=cortex-m4 -mfloat-abi=hard -mfpu=fpv4-sp-d16 -o field_10x26_arm.o field_10x26_arm.s
$ arm-none-eabi-as -o field_10x26_arm.o field_10x26_arm.s

Please let me know how can I help to debug this issue.

[real-or-random]

Okay, yeah I can reproduce this.

armv7 is the intersection of -A and -M, and -M only support thumb. For some reason, non-thumb (-marm) seems to be the default, at least for my toolchain, see gcc -Q --help=target (I don't know if there is an equivalent help option for as). So armv7 (only thumb) with non-thumb gives leaves you with no valid instructions, so every line is an error [1].

If you set -mcpu=cortex-m4 on the command line, this will override the specified .arch and everything is alright again...

I guess the right thing to do here is

• consider dropping the .arch directive and just let the assembler options choose it
• drop some of the .eabi_attribute directives, namely for Tag_ARM_ISA_use ,Tag_Thumb_ISA_use, and maybe Tag_FP_arch because the assembler will set them correctly automatically then. (At the moment they're set to wrong values if you override the CPU on the command line.)

Interestingly, at least secp256k1 builds for me even without this PR with -mcpu=cortex-m4. @romanz Which error message do you get? Does this fail when linking to the rest of Trezor? (Maybe the EABI attributes are the culprit here.) I'll try to build Trezor with this on my machine.

@laanwj Can you comment on this?

For reference:
arm-none-eabi-gcc (Arch Repository) 8.3.0
GNU assembler (GNU Binutils) 2.32x
it builds for me with
./configure --host=arm-none-eabi --enable-experimental --disable-benchmark --disable-tests --disable-exhaustive-tests --with-asm=arm CFLAGS="--specs=nosys.specs -mcpu=cortex-m4

[1] https://stackoverflow.com/questions/39515677/compiling-to-arm-i-get-error-attempt-to-use-an-arm-instruction-on-a-thumb-only

[romanz]

Which error message do you get?

When I use the original assembly code (with .arch armv7-a), the linker fails with:

.../ld: error: build/firmware/vendor/secp256k1-zkp/src/asm/field_10x26_arm.o: Conflicting architecture profiles A/M
.../ld: failed to merge target specific data of file build/firmware/vendor/secp256k1-zkp/src/asm/field_10x26_arm.o
collect2: error: ld returned 1 exit status
scons: *** [build/firmware/firmware.elf] Error 1
make: *** [build_firmware] Error 2

[real-or-random]

Indeed even with -mcpu=cortex-m4 it just silently outputs for -A when .arch armv7-a is set...

Funnily, if you give both -mcpu=cortex-m4 -march=armv7-a on the command line you at least get

cc1: warning: switch -mcpu=cortex-m4 conflicts with -march=armv7-a switch

[laanwj]

@laanwj Do you understand the implications of this?

Will have a look at this, sorry, missed the ping.

[laanwj]

I guess the right thing to do here is

* consider dropping the `.arch` directive and just let the assembler options choose it

* drop some of the `.eabi_attribute` directives, namely for `Tag_ARM_ISA_use` ,`Tag_Thumb_ISA_use`, and maybe `Tag_FP_arch` because the assembler will set them correctly automatically then. (At the moment they're set to wrong values if you override the CPU on the command line.)

I think this is fine, if it solves the problem. The code is not making any special ABI or architecture assumptions, besides being able to use 32-bit ARM instructions. It obviously won't work on thumb-only architectures, but if you omit the .arch it will fail at assembly time in that case instead of linker time (which is good).

These two ABI attributes are critical and will potentially result in linker issues if missing or wrong:

        .eabi_attribute 24, 1 @ Tag_ABI_align_needed = 8-byte
        .eabi_attribute 25, 1 @ Tag_ABI_align_preserved = 8-byte, except leaf SP

I've tried it out and keeping just these two (removing other eabi and arch directives) results in the same binaries (at least for arm-linux-gnueabihf, gcc Debian 6.3.0-18+deb9u1) apart from the build ID.

[romanz]

Thanks for the review and the suggestions!
I removed .arch and .eabi_attribute directives (except the two from above), and it builds and works well on the TREZOR model T device.

[laanwj]

LGTM now
ACK

[gmaxwell]

ACK

[real-or-random]

ACK

@laanwj

The code is not making any special ABI or architecture assumptions, besides being able to use 32-bit ARM instructions . It obviously won't work on thumb-only architectures

I don't see where the code makes this assumption; it's in unified syntax. The Cortex-M4 is thumb-only and the point of this PR is exactly to make it compile for this CPU.