Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makefile: build precomputed_ecmult generators using build-system toolchain to support cross-compiling #1159

Closed
wants to merge 1 commit into from

Conversation

whitslack
Copy link
Contributor

When cross-compiling libsecp256k1, if the precomputed_ecmult*.c source files need to be regenerated, then the generators need to be built for the build system, not for the host system. Autoconf supports this fairly cleanly via the AX_PROG_CC_FOR_BUILD macro (from Autoconf Archive), but Automake requires some hackery. When building the generators, we override the CC, CFLAGS, CPPFLAGS, and LDFLAGS variables to their build-system counterparts, whose names are suffixed with _FOR_BUILD and whose values are populated by the aforementioned Autoconf macro and may be overridden on the make command line. Since Automake lacks support for overriding EXEEXT on a per-program basis, we define a recipe that builds the generator binaries with names suffixed with $(EXEEXT) and then renames them suffixed with $(BUILD_EXEEXT).

When cross-compiling libsecp256k1, if the `precomputed_ecmult*.c` source
files need to be regenerated, then the generators need to be built for
the *build* system, not for the *host* system. Autoconf supports this
fairly cleanly via the `AX_PROG_CC_FOR_BUILD` macro (from Autoconf
Archive), but Automake requires some hackery. When building the
generators, we override the `CC`, `CFLAGS`, `CPPFLAGS`, and `LDFLAGS`
variables to their build-system counterparts, whose names are suffixed
with `_FOR_BUILD` and whose values are populated by the aforementioned
Autoconf macro and may be overridden on the `make` command line. Since
Automake lacks support for overriding `EXEEXT` on a per-program basis,
we define a recipe that builds the generator binaries with names
suffixed with `$(EXEEXT)` and then renames them suffixed with
`$(BUILD_EXEEXT)`.
@whitslack
Copy link
Contributor Author

whitslack commented Nov 20, 2022

Note: This PR introduces a dependency on Autoconf Archive. If this is undesirable, then the ax_prog_cc_for_build.m4 file can be extracted from Autoconf Archive and added to the build-aux/m4 subdirectory of this project.

Addendum: It looks like the automatic tests are failing because the build environment lacks Autoconf Archive. I don't know enough about Cirrus to fix that. Help?

@sipa
Copy link
Contributor

sipa commented Nov 20, 2022

The precomputed_* files don't need to be regenerated. They're independent of the host or target build environment, so they can be created on any system, and compiled on any system.

@whitslack
Copy link
Contributor Author

@sipa: The precomputed files need to be regenerated if you specify non-default values for --with-ecmult-window= and --with-ecmult-gen-precision= options to configure. If you're building using a cross-compiler, then the regeneration will fail because the generators will not be executable on the system executing the build. Standard practice is to compile build-time tools using a toolchain that targets the build system.

@whitslack
Copy link
Contributor Author

An example illustrating the failure:

$ git checkout origin/master
$ git clean -fdx
$ ./autogen.sh
$ ./configure --build=x86_64-pc-linux-gnu --host=armv6j-unknown-linux-gnueabihf --with-ecmult-window=2 --with-ecmult-gen-precision=2
$ make clean-precomp
$ make
make  precompute_ecmult_gen
make[1]: Entering directory '/example'
  CC       src/precompute_ecmult_gen-precompute_ecmult_gen.o
  CCLD     precompute_ecmult_gen
make[1]: Leaving directory '/example'
./precompute_ecmult_gen
./precompute_ecmult_gen: 1: Syntax error: word unexpected (expecting ")")
make: *** [Makefile:1979: src/precomputed_ecmult_gen.c] Error 2

The same example after applying this PR:

$ git fetch origin pull/1159/head
$ git checkout FETCH_HEAD
$ git clean -fdx
$ ./autogen.sh
$ ./configure --build=x86_64-pc-linux-gnu --host=armv6j-unknown-linux-gnueabihf --with-ecmult-window=2 --with-ecmult-gen-precision=2
$ make clean-precomp
$ make
make  precompute_ecmult_gen
make[1]: Entering directory '/example'
  CC       src/precompute_ecmult_gen-precompute_ecmult_gen.o
  CCLD     precompute_ecmult_gen
make[1]: Leaving directory '/example'
./precompute_ecmult_gen
make  precompute_ecmult
make[1]: Entering directory '/example'
  CC       src/precompute_ecmult-precompute_ecmult.o
  CCLD     precompute_ecmult
make[1]: Leaving directory '/example'
./precompute_ecmult
make  all-am
make[1]: Entering directory '/example'
  CC       src/bench.o
  CC       src/libsecp256k1_la-secp256k1.lo
  CC       src/libsecp256k1_precomputed_la-precomputed_ecmult.lo
  CC       src/libsecp256k1_precomputed_la-precomputed_ecmult_gen.lo
  CCLD     libsecp256k1_precomputed.la
  CCLD     libsecp256k1.la
  CCLD     bench
  CC       src/bench_internal-bench_internal.o
  CCLD     bench_internal
  CC       src/bench_ecmult-bench_ecmult.o
  CCLD     bench_ecmult
  CC       src/tests-tests.o
  CCLD     tests
  CC       src/exhaustive_tests-tests_exhaustive.o
  CCLD     exhaustive_tests
make[1]: Leaving directory '/example'

@real-or-random
Copy link
Contributor

real-or-random commented Nov 21, 2022

This has a bit of a history. We used to support AX_PROG_CC_FOR_BUILD for years but then removed it (d94a37a) because our experience was that it results in a lot of issues on various platforms and rare configs. The macro itself and the autoconf code we'll need is a little bit hackish, plus users often didn't understand how building is supposed to work.

The current resolution (as of #988) is to have the precomputed files in the repo. There should be only two reasons to rebuild them:

  1. As a developer, if you changed the programs that generate them.
  2. As a user, if you set --with-ecmult-window=SIZE for SIZE > 15. (But that's slower on "normal" machines.)

See the docs:

  --with-ecmult-window=SIZE|auto
                          window size for ecmult precomputation for
                          verification, specified as integer in range [2..24].
                          Larger values result in possibly better performance
                          at the cost of an exponentially larger precomputed
                          table. The table will store 2^(SIZE-1) * 64 bytes of
                          data but can be larger in memory due to
                          platform-specific padding and alignment. A window
                          size larger than 15 will require you delete the
                          prebuilt precomputed_ecmult.c file so that it can be
                          rebuilt. For very large window sizes, use "make -j
                          1" to reduce memory use during compilation. "auto"
                          is a reasonable setting for desktop machines
                          (currently 15). [default=auto]

I see in your example that you want a SIZE of 2. And I see that you run make clean-precomp. Maybe this not entirely clear from the docs you're never supposed to run make clean-precomp as a user. Just checkout the repo, configure with --ecmult-window-size=2, and make. That should work. Can you try?

(And maybe try a SIZE of 4. It should be much faster than 2 but still need only a negligible amount of memory.)

(And even if you want > 15, just delete precomputed_ecmult.c as the docs say and automake should figure out how to rebuild it -- if you're not cross-compiling.)

@whitslack
Copy link
Contributor Author

@real-or-random: Thank you for the history review. It's a shame that a few edge cases had to ruin it for everyone. I do have to wonder if the issues you mentioned were due to Autotools not being used correctly, but I guess that ship has sailed.

My interest comes not as a user but as a packager. Gentoo takes a purist approach to open-source software: anything than can be rebuilt from sources ought to be. Standard operating procedure is to discard pregenerated assets and rebuild them from sources wherever possible. This is what the ebuilds for libsecp256k1 do with the precomputed ecmult tables. We have had to employ a hack to support cross-compiling, whereas most software packages that use Autotools and that compile tools to be executed during the build apparently have no trouble using AX_PROG_CC_FOR_BUILD.

I'll withdraw my request for proper cross-compilation support and continue carrying my patch downstream. Thank you for your time and explanation.

@whitslack whitslack closed this Nov 22, 2022
@real-or-random
Copy link
Contributor

real-or-random commented Nov 22, 2022

@whitslack

Thanks for your understanding. And thanks for reaching out! We don't typically get feedback from packagers, and I see people applying (sometimes weird) patches without having talked to us.

My interest comes not as a user but as a packager. Gentoo takes a purist approach to open-source software: anything than can be rebuilt from sources ought to be. Standard operating procedure is to discard pregenerated assets and rebuild them from sources wherever possible.

I see. I was a Gentoo user in the past, but that's maybe a little bit too purist for my taste (though there's apparently a "wherever possible" exception). I'm not sure if this helps you, but maybe a simpler patch is to invoke a compiler on the build system manually. We try to make sure that the library is very easy to build manually, even though that's currently not documented.


I had a look at your ebuild at https://gitlab.com/bitcoin/gentoo/-/blob/d783d4d95cc1e7befeecec30a2d1f9e0d3a5ad72/dev-libs/libsecp256k1/libsecp256k1-0.1.0_pre20220803.ebuild#L73-74

		$(usex lowmem '--with-ecmult-window=2 --with-ecmult-gen-precision=2' '')
		$(usex precompute-ecmult '--with-ecmult-window=24 --with-ecmult-gen-precision=8' '')

This should really be

		$(usex lowmem '--with-ecmult-window=4 --with-ecmult-gen-precision=2' '')

(We should really just have a --with-low-mem option...)

And precompute-ecmult should probably be removed. I don't see what the purpose of this flag is. We always have precomputed ecmult tables and the only thing the USE flag currently does is increase these tables to a huge size, so huge that performance will suffer for sure (because the tables won't fit in the CPU cache).

The defaults are 15 and 4, and they've been selected using careful benchmarks for desktop machines. Maybe you can squeeze out a few percents on your specific machine with something like (14,4) or (16,4) or similar if you really do the benchmarks. But (24,8) is insanely high and has no benefits.

@whitslack
Copy link
Contributor Author

And precompute-ecmult should probably be removed.

@real-or-random: Thanks. I'll make those changes. Those flags originated before I got involved.

@whitslack
Copy link
Contributor Author

I'll make those changes.

Done.

gentoo-repo-qa-bot pushed a commit to gentoo-mirror/bitcoin that referenced this pull request Nov 23, 2022
And change the ecmult window size for USE="lowmem" from 2 to 4.

Suggested-by: Tim Ruffing <[email protected]>
See: bitcoin-core/secp256k1#1159 (comment)
whitslack added a commit to whitslack/gentoo-portage-repo that referenced this pull request Jan 6, 2023
whitslack added a commit to whitslack/gentoo-portage-repo that referenced this pull request Jan 6, 2023
whitslack added a commit to whitslack/gentoo-portage-repo that referenced this pull request Jan 6, 2023
whitslack added a commit to whitslack/gentoo-portage-repo that referenced this pull request Jan 6, 2023
whitslack added a commit to whitslack/gentoo-portage-repo that referenced this pull request Jan 7, 2023
whitslack added a commit to whitslack/gentoo-portage-repo that referenced this pull request Jan 7, 2023
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Jan 7, 2023
And change the ecmult window size for USE="lowmem" from 2 to 4.

Suggested-by: Tim Ruffing <[email protected]>
See: bitcoin-core/secp256k1#1159 (comment)
See: https://gitlab.com/bitcoin/gentoo/-/commit/6e39601a748f3465f66a38e7989e7414a4a1d9c0
Signed-off-by: Matt Whitlock <[email protected]>
Signed-off-by: Sam James <[email protected]>
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants