Page MenuHomePhabricator

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

Authored by Fabien on Mon, Mar 16, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Mon, Mar 16, 11:03
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Mar 16, 11:03
Fabien edited the test plan for this revision. (Show Details)Mon, Mar 16, 11:06
deadalnix requested changes to this revision.Mon, Mar 16, 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.Mon, Mar 16, 18:47
Fabien edited the summary of this revision. (Show Details)Mon, Mar 16, 20:59
Fabien edited the test plan for this revision. (Show Details)
Fabien edited the test plan for this revision. (Show Details)Mon, Mar 16, 21:05
Fabien updated this revision to Diff 16959.Mon, Mar 16, 21:06

Merge the options into a single one.

Fabien updated this revision to Diff 16960.Mon, Mar 16, 21:08

Forgot to remove the error message.

deadalnix requested changes to this revision.Tue, Mar 17, 02:30
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
68 ↗(On Diff #16960)

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.Tue, Mar 17, 02:30
Fabien edited the summary of this revision. (Show Details)Wed, Mar 18, 12:29
Fabien edited the test plan for this revision. (Show Details)
Fabien updated this revision to Diff 16993.Wed, Mar 18, 12:40

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

deadalnix accepted this revision.Wed, Mar 18, 16:53
This revision is now accepted and ready to land.Wed, Mar 18, 16:53
This revision was automatically updated to reflect the committed changes.