Allow field_10x26_arm.s to compile for ARMv7 architecture

romanz commented at 7:53 pm on April 21, 2019: contributor

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

sipa commented at 8:02 pm on April 21, 2019: contributor

@laanwj Do you understand the implications of this?

romanz referenced this in commit dc2f591587 on Apr 21, 2019

real-or-random commented at 2:06 pm on April 22, 2019: contributor

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

gmaxwell commented at 8:23 pm on April 26, 2019: contributor

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 commented at 8:29 pm on April 26, 2019: contributor

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 commented at 9:19 pm on April 26, 2019: contributor

That makes sense; concept ACK

real-or-random cross-referenced this on Apr 27, 2019 from issue Allow field_10x26_arm.s to compile for ARMv7 architecture by real-or-random

gmaxwell commented at 11:27 pm on April 27, 2019: contributor

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 modeubfx 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 commented at 8:16 am on April 28, 2019: contributor

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 commented at 6:32 pm on April 28, 2019: contributor

@gmaxwell Many thanks for the testing!

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

0$ make clean build_firmware > build.log

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

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

The versions I am using are:

 0$ arm-none-eabi-gcc --version
 1arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2018-q2-update) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907]
 2Copyright (C) 2017 Free Software Foundation, Inc.
 3This is free software; see the source for copying conditions.  There is NO
 4warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 5
 6$ arm-none-eabi-as --version
 7GNU assembler (GNU Tools for Arm Embedded Processors 7-2018-q2-update) 2.30.0.20180329
 8Copyright (C) 2018 Free Software Foundation, Inc.
 9This program is free software; you may redistribute it under the terms of
10the GNU General Public License version 3 or later.
11This program has absolutely no warranty.
12This 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:

 0$ sudo apt install -y gcc-arm-none-eabi
 1$ arm-none-eabi-gcc --version
 2arm-none-eabi-gcc (15:5.4.1+svn241155-1) 5.4.1 20160919
 3Copyright (C) 2015 Free Software Foundation, Inc.
 4This is free software; see the source for copying conditions.  There is NO
 5warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 6
 7$ arm-none-eabi-as --version
 8GNU assembler (2.28-5+9+b3) 2.28
 9Copyright (C) 2017 Free Software Foundation, Inc.
10This program is free software; you may redistribute it under the terms of
11the GNU General Public License version 3 or later.
12This program has absolutely no warranty.
13This assembler was configured for a target of `arm-none-eabi'.
14
15$ arm-none-eabi-as -mthumb -mcpu=cortex-m4 -mfloat-abi=hard -mfpu=fpv4-sp-d16 -o field_10x26_arm.o field_10x26_arm.s
16$ 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 commented at 7:40 pm on April 28, 2019: contributor

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 commented at 7:54 pm on April 28, 2019: contributor

Which error message do you get?

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

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

real-or-random commented at 9:18 pm on April 28, 2019: contributor

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

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

laanwj commented at 8:34 am on May 7, 2019: member

@laanwj Do you understand the implications of this?

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

laanwj commented at 9:16 am on May 7, 2019: member

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:

0        .eabi_attribute 24, 1 @ Tag_ABI_align_needed = 8-byte
1        .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 force-pushed on May 7, 2019

romanz commented at 6:29 pm on May 7, 2019: contributor

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.

Allow field_10x26_arm.s to compile for ARMv7 architecture d4d270a59c

romanz force-pushed on May 7, 2019

laanwj commented at 10:42 am on May 8, 2019: member

LGTM now ACK

gmaxwell commented at 2:29 am on May 9, 2019: contributor

ACK

real-or-random commented at 6:40 am on May 9, 2019: contributor

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.

gmaxwell merged this on May 9, 2019

gmaxwell closed this on May 9, 2019

gmaxwell referenced this in commit 84a808598b on May 9, 2019

sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa

fanquake referenced this in commit 8c97780db8 on Jun 13, 2020

sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020

ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020

real-or-random cross-referenced this on Jul 27, 2020 from issue Improve constant-timeness on PowerPC by real-or-random

UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021

5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021

gades referenced this in commit d855cc511d on May 8, 2022

Allow field_10x26_arm.s to compile for ARMv7 architecture #612