Follow-ups to making all tables fully static #1041
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sipa
force-pushed
the
202112_precdotc
branch
3 times, most recently
from
6f8faeb to
f233242
Compare
sipa
force-pushed
the
202112_precdotc
branch
from
f233242 to
51512b0
Compare
sipa
force-pushed
the
202112_precdotc
branch
from
51512b0 to
cf38e94
Compare
|
Sigh, this isn't a solution either. |
|
Going to try a different approach. |
|
What was the problem with this approach? Just asking because I was in the middle of reviewing this. |
|
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. |
|
I see. Thanks. |
real-or-random
added a commit
that referenced
this pull request
Dec 20, 2021
e05da9e Fix c++ build (Pieter Wuille) c45386d Cleanup preprocessor indentation in precompute{,d}_ecmult{,_gen} (Pieter Wuille) 19d96e1 Split off .c file from precomputed_ecmult.h (Pieter Wuille) 1a6691a Split off .c file from precomputed_ecmult_gen.h (Pieter Wuille) bb36331 Simplify precompute_ecmult_print_* (Pieter Wuille) 38cd84a Compute ecmult tables at runtime for tests_exhaustive (Pieter Wuille) e458ec2 Move ecmult table computation code to separate file (Pieter Wuille) fc1bf9f Split ecmult table computation and printing (Pieter Wuille) 31feab0 Rename function secp256k1_ecmult_gen_{create_prec -> compute}_table (Pieter Wuille) 725370c Rename ecmult_gen_prec -> ecmult_gen_compute_table (Pieter Wuille) 075252c Rename ecmult_static_pre_g -> precomputed_ecmult (Pieter Wuille) 7cf47f7 Rename ecmult_gen_static_prec_table -> precomputed_ecmult_gen (Pieter Wuille) f95b810 Rename gen_ecmult_static_pre_g -> precompute_ecmult (Pieter Wuille) bae7768 Rename gen_ecmult_gen_static_prec_table -> precompute_ecmult_gen (Pieter Wuille) Pull request description: This PR implements a number of changes to follow up after merging #988: * Naming consistency: * All precomputed table files now have name `precomputed_*.*` * All source files related to the creation of the precomputed table files have name `precompute_*.*`. * All source files related to the computation of tables (whether they go in precomputed files or not) have name `*_compute_table.*`. * 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 point, as the alternative would be to have separate precomputed libraries for the exhaustive tests and other binaries. * Moves the actual tables to separate `precomputed_*.c` files, which are compiled only once as part of a new `libsecp256k1_precomputed.la`, included where relevant. The corresponding `precomputed_*.h` file are normal source files. Retry of #1041. ACKs for top commit: real-or-random: ACK e05da9e jonasnick: ACK e05da9e Tree-SHA512: 71eadd66e30e511b786e910755e0eda53330dfa163b37e33602c3392f7b893569f56d3ca9870e85cbb3de83880fc5aef61ac3d55d759d7395086a69023f13f03
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements a number of changes to follow up after merging #988:
precompute_ecmultandprecompute_ecmult_genprefixes (avoiding the "gen" ambiguity).precomputed_ecmultandprecomputed_ecmult_genprefixes.precompute.precomputed_ecmult.candprecomputed_ecmult_gen.cfiles, which are compiled only once as part of a newlibsecp256k1_precomputed.la, included where relevant.