Page MenuHomePhabricator

[SECP256K1] CMake: Build the ARM ASM field implementation
ClosedPublic

Authored by Fabien on Mar 16 2020, 11:03.

Details

Summary

This diff allow cmake to use the ARM optimized assembly (equivalent to autotools --with-asm=arm).

It attempts to guess the build target, and default with an error if assembly is enabled but unsupported by the target (or the target is not known).
This makes the build to succeed by default on x86_64, and on ARM when the toolchain file is used, so no user intervention is needed for the "normal" path.

Depends on D5501.

Test Plan

Should ask to use -DSECP256K1_USE_ASM=OFF:

cmake -GNinja .. -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Linux32.cmake

Should succeed:

cmake -GNinja .. -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/LinuxARM.cmake
ninja

Run the test binaries on an ARM target and check there is no error
(tested on a RPi).

Run the build on OSX.

Run the Gitian builds.

Run the Travis build (see https://travis-ci.org/github/Fabcien/secp256k1/builds/663863958).

Diff Detail

Repository
rABC Bitcoin ABC
Branch
secp256k1_arm_asm
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9833
Build 17536: Default Diff Build & Tests
Build 17535: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Mar 16 2020, 18:47
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
69 ↗(On Diff #16941)

It does not make sense to have two options. It's not like I can build on X86 with ARM asm in any way that make sense.

This revision now requires changes to proceed.Mar 16 2020, 18:47
Fabien edited the test plan for this revision. (Show Details)

Merge the options into a single one.

Forgot to remove the error message.

deadalnix requested changes to this revision.Mar 17 2020, 02:30
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
68

You did not think this through. Tis has the exact same problem as the previous one. Why would I want to do an ARM build with X86 assembly? In what situation does this makes any sense?

This revision now requires changes to proceed.Mar 17 2020, 02:30
Fabien edited the test plan for this revision. (Show Details)

Attempt to address feedback: rebase on top of D5501, updated summary and test plan.

This revision is now accepted and ready to land.Mar 18 2020, 16:53