Follow-ups to making all tables fully static

sipa commented at 10:50 pm on December 15, 2021: contributor

This PR implements a number of changes to follow up after merging #988:

Naming consistency:
- Renames all files related to the precomputated tables to have a consistent precompute_ecmult and precompute_ecmult_gen prefixes (avoiding the “gen” ambiguity).
- Renames all precomputed files themselves to have consistent precomputed_ecmult and precomputed_ecmult_gen prefixes.
- Merges the binaries precomputing the table files into one precompute.
Make the tables for exhaustive tests be computed at runtime rather than compile time (this was already the case for ecmult_gen, but not ecmult). This is a preparation for the next commit, as the alternative would be to have separate precomputed libraries for the exhaustive tests and other binaries.
Moves the actual tables to separate precomputed_ecmult.c and precomputed_ecmult_gen.c files, which are compiled only once as part of a new libsecp256k1_precomputed.la, included where relevant.

sipa force-pushed on Dec 15, 2021

sipa force-pushed on Dec 16, 2021

in src/gen_ecmult_gen_static_prec_table.c:14 in eb55f879e5 outdated

 9@@ -10,8 +10,8 @@
10 #include "../include/secp256k1.h"
11 #include "assumptions.h"
12 #include "util.h"
13-#include "group.h"
14-#include "ecmult_gen.h"

real-or-random commented at 9:11 am on December 16, 2021:

nit: I think ecmult_gen.h is included for ECMULT_GEN_PREC_G and ECMULT_GEN_PREC_N.

sipa commented at 9:22 pm on December 16, 2021:

Fixed.

in Makefile.am:156 in 3cc9ec987f outdated

157-src/ecmult_gen_static_prec_table.h:
158-	$(MAKE) $(AM_MAKEFLAGS) gen_ecmult_gen_static_prec_table$(EXEEXT)
159-	./gen_ecmult_gen_static_prec_table$(EXEEXT)
160-
161 PRECOMP = src/ecmult_gen_static_prec_table.h src/ecmult_static_pre_g.h
162+$(PRECOMP)&:

real-or-random commented at 9:17 am on December 16, 2021:

is the & on purpose? I’ve never seen this syntax before.

edit: Could this be the reason for the CI fail on MacOS?

sipa commented at 3:03 pm on December 16, 2021:

Yes, it means: one command builds all these targets. Otherwise, if invoked with -jN, the submake commands gets invoked multiple times in parallel, which caused race condition failures on CI.

I guess I’ll need another solution if mac doesn’t support it.

real-or-random commented at 3:29 pm on December 16, 2021:

Ah that explanation helped me search the web: https://stackoverflow.com/questions/63928066/what-is-the-canonical-way-to-simulate-grouped-targets-in-older-versions-of-make And this confirms that it does not work on Mac Make.

real-or-random commented at 3:34 pm on December 16, 2021:

I think you could make it a pattern rule, which have supported grouped targets for a long time, see last example here: https://www.gnu.org/software/make/manual/html_node/Pattern-Examples.html#Pattern-Examples

0src/precomputed_%: 
1	$(MAKE) $(AM_MAKEFLAGS) precompute$(EXEEXT)
2	./precompute$(EXEEXT)

sipa commented at 3:43 pm on December 16, 2021:

How does the syntax in your suggested code indicate grouping?

sipa commented at 3:54 pm on December 16, 2021:

Oh, they always are. TIL

sipa commented at 3:56 pm on December 16, 2021:

Pattern rules may have more than one target; however, every target must contain a % character. Pattern rules are always treated as grouped targets (see Multiple Targets in a Rule) regardless of whether they use the : or &: separator.

I interpret that as that you actually need multiple patterns on the left of the column. Otherwise rules like %.o: %.c would not do what we expect them to do.

real-or-random commented at 5:45 pm on December 16, 2021:

Oh true… One could still abuse a pattern rule with % that essentially matches only once. But then not sure whether this is ugly than a stamp file.

sipa commented at 7:49 pm on December 16, 2021:

Hmm, what about just

0precomp:
1	$(MAKE) $(AM_MAKEFLAGS) precompute$(EXEEXT)
2	./precompute$(EXEEXT)

?

sipa commented at 9:21 pm on December 16, 2021:

Switched to a different approach, where ./precompute is given the filename to generate, and it just generates that.

real-or-random commented at 9:23 am on December 16, 2021: contributor

That was quick1

Concept ACK

Cleanup helper functions for gen_ecmult_*.c 423e6015a3

sipa force-pushed on Dec 16, 2021

Combine gen_ecmult_*.c into precompute.c f9daaa548d

Rename precomputed table files to precomputed_*.h 886990770b

Split off .c file from precomputed_ecmult_gen.h e696f996e5

Split ecmult table computation and printing c6cfd8525d

Move ecmult precomputation code to separate file fd861203b8

Precompute ecmult at runtime for tests_exhaustive 73e8814985

Simplify precompute_ecmult_print_* 69b4bc97ea

Split off .c file from precomputed_ecmult.h 598a4bebe9

Simplify arguments to precompute and use make pattern cf38e94cf8

sipa force-pushed on Dec 16, 2021

sipa commented at 9:44 pm on December 16, 2021: contributor

Sigh, this isn’t a solution either.

sipa closed this on Dec 17, 2021

sipa commented at 4:05 pm on December 17, 2021: contributor

Going to try a different approach.

jonasnick commented at 6:50 pm on December 17, 2021: contributor

What was the problem with this approach? Just asking because I was in the middle of reviewing this.

sipa commented at 6:53 pm on December 17, 2021: contributor

The problem was that if you build with “make -j”, and it involves rebuilding the precomputed table files (4 in this PR as is), it’ll trigger 4 invocations of a sub-make command to build the “precompute” binary, which end up overwriting each other, and race conditions.

The solution is something called “grouped targets” in Makefiles, where you indicate that multiple files get built simultaneously by one rule, but it seems the macOs make doesn’t support this feature.

The approach I’m going to try next is keeping precompute_ecmult and precompute_ecmult_gen separate, and only having them generate the precomputed_*.c file. The corresponding .h file can be a normal checked-in file in the repository.

jonasnick commented at 7:12 pm on December 17, 2021: contributor

I see. Thanks.

sipa cross-referenced this on Dec 17, 2021 from issue Follow-ups to making all tables fully static by sipa

real-or-random referenced this in commit be6944ade9 on Dec 20, 2021

Follow-ups to making all tables fully static #1041