Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGING727eab4bdd69: [SECP256K1] CMake: Build the ARM ASM field implementation
rABC727eab4bdd69: [SECP256K1] CMake: Build the ARM ASM field implementation

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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Mar 16 2020, 11:03

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 16 2020, 11:03

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Fabien edited the test plan for this revision. (Show Details)Mar 16 2020, 11:06

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9820: Diff 16941.Mar 16 2020, 11:13

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing (UNKNOWN) started.

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing (UNKNOWN) passed.

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 summary of this revision. (Show Details)Mar 16 2020, 20:59

Fabien edited the test plan for this revision. (Show Details)

Fabien edited the test plan for this revision. (Show Details)Mar 16 2020, 21:05

Merge the options into a single one.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Forgot to remove the error message.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9832: Diff 16959.Mar 16 2020, 21:16

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9833: Diff 16960.Mar 16 2020, 21:33

deadalnix requested changes to this revision.Mar 17 2020, 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?