Allow field_10x26_arm.s to compile for ARMv7 architecture #612

pull romanz wants to merge 1 commits into bitcoin-core:master from romanz:arm-assembly changing 1 files +0 −6
  1. 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.
  2. sipa commented at 8:02 pm on April 21, 2019: contributor
    @laanwj Do you understand the implications of this?
  3. romanz referenced this in commit dc2f591587 on Apr 21, 2019
  4. 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 .
  5. 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.
  6. 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.
  7. sipa commented at 9:19 pm on April 26, 2019: contributor
    That makes sense; concept ACK
  8. 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
  9. 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 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)

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

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

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

  13. 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
    
  14. 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
    
  15. 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.

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

  17. romanz force-pushed on May 7, 2019
  18. 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.
  19. Allow field_10x26_arm.s to compile for ARMv7 architecture d4d270a59c
  20. romanz force-pushed on May 7, 2019
  21. laanwj commented at 10:42 am on May 8, 2019: member
    LGTM now ACK
  22. gmaxwell commented at 2:29 am on May 9, 2019: contributor
    ACK
  23. 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.

  24. gmaxwell merged this on May 9, 2019
  25. gmaxwell closed this on May 9, 2019

  26. gmaxwell referenced this in commit 84a808598b on May 9, 2019
  27. sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa
  28. fanquake referenced this in commit 8c97780db8 on Jun 13, 2020
  29. sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020
  30. ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020
  31. real-or-random cross-referenced this on Jul 27, 2020 from issue Improve constant-timeness on PowerPC by real-or-random
  32. UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021
  33. 5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021
  34. gades referenced this in commit d855cc511d on May 8, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 05:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me