build: Fix check of __builtin functions #247
Draft
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
This removes a check for $ac_cv_prog_cc_c89 which is set by AC_PROG_CC if defined(__STDC__) in the preprocessor. (Standard compliant compilers are supposed to define __STDC__ to 1 but the value is actually not checked here.) Unfortunately, MSVC doesn't define it, so configure fails for MSVC. This check is not very useful in practice. Over 30 years after C89 has been released, there are no C compilers out there that are not sufficiently compliant with C89 for the project. The only practically relevant case was that the check rejected C++ compilers. A different method to reject C++ compilers will be introduced in a later commit.
- Updated _gej_add_var, _gej_add_ge_var, _gej_add_zinv_var - 2 fewer _fe_negate in each method - Updated operation counts and standardize layout - Added internal benchmark for _gej_add_zinv_var - Update sage files (fixed by Tim Ruffing)
…ddition 2f984ff Save negations in var-time group addition (Peter Dettman) Pull request description: - Updated _gej_add_var, _gej_add_ge_var, _gej_add_zinv_var - 2 fewer _fe_negate in each method - Updated operation counts and standardize layout - Added internal benchmark for _gej_add_zinv_var benchmark_internal shows about 2% speedup in each method as a result (64bit). ACKs for top commit: real-or-random: ACK 2f984ff jonasnick: ACK 2f984ff Tree-SHA512: 01366fa23c83a8dd37c9a0a24e0acc53ce38a201607fe4da6672ea5618d82c62d1299f0e0aa50317883821539af739ea52b6561faff230c148e6fdc5bc5af30b
…BIT_ASM_CHECK` 7efc983 Fix the false positive of `SECP_64BIT_ASM_CHECK` (Sprite) Pull request description: I'm trying to compile this project for RISC-V architecture, and I encountered errors: ``` src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r15' in 'asm' 28 | __asm__ __volatile__( | ^ src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r14' in 'asm' src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r13' in 'asm' src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r12' in 'asm' src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r11' in 'asm' src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r10' in 'asm' src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r9' in 'asm' src/field_5x52_asm_impl.h:28:1: error: unknown register name '%r8' in 'asm' src/field_5x52_asm_impl.h:28:1: error: unknown register name '%rdx' in 'asm' src/field_5x52_asm_impl.h:28:1: error: unknown register name '%rcx' in 'asm' src/field_5x52_asm_impl.h:28:1: error: unknown register name '%rax' in 'asm' src/field_5x52_asm_impl.h:28:1: error: output number 0 not directly addressable src/field_5x52_asm_impl.h: In function 'secp256k1_fe_sqr': src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r15' in 'asm' 298 | __asm__ __volatile__( | ^ src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r14' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r13' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r12' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r11' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r10' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r9' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%r8' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%rdx' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%rcx' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%rbx' in 'asm' src/field_5x52_asm_impl.h:298:1: error: unknown register name '%rax' in 'asm' src/field_5x52_asm_impl.h:298:1: error: output number 0 not directly addressable ``` After further investigation I found that for RISC-V, macro `USE_ASM_X86_64` was defined unexpectedly, and `checking for x86_64 assembly availability... yes` appeared in the compilation log file, which means `SECP_64BIT_ASM_CHECK` was not working as expected. For unknown reasons, `AC_COMPILE_IFELSE` does not check if `__asm__` can be compiled, and an example can verify this point: ```m4 AC_DEFUN([SECP_64BIT_ASM_CHECK],[ AC_MSG_CHECKING(for x86_64 assembly availability) AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ #include <stdint.h>]],[[ __asm__ __volatile__("this is obviously wrong"); ]])],[has_64bit_asm=yes],[has_64bit_asm=no]) AC_MSG_RESULT([$has_64bit_asm]) ]) ``` It always gives results: `checking for x86_64 assembly availability... yes` After testing, replacing `AC_COMPILE_IFELSE` with `AC_LINK_IFELSE` can correctly check if `__asm__` can be compiled and make the project able to compile for RISC-V. ACKs for top commit: real-or-random: ACK 7efc983 Tree-SHA512: 7318dd42004b2930cfcd6541c5a9ce0aa186e2179a668b76089a908bea8d9f70fcfdb264512f971e395a3ce9dc7f9ca24c8f3d46175cad2972a2d713f518ed85
libtool takes care of building both object versions, we just need to pick the right one to export symbols.
…raries 6f6cab9 abi: Don't export symbols in static Windows libraries (Cory Fields) Pull request description: For context, Bitcoin Core has recently merged [libbitcoin-kernel](bitcoin/bitcoin#24322), a small library that intends to eventually minimally encompass Core's validation engine. This kernel lib includes a static libsecp256k1. Without this change, because libsecp256k1.a ends up with exported symbols, we end up with libsecp256k1 symbols exported by our libbitcoin-kernel library (which causes unrelated problems not worth getting into here). libtool takes care of building both object versions, and it automatically builds objects for shared libs with -DDLL_EXPORT. We just need to opt-in to its functionality. I can't imagine this having any negative impact on any current statically-linking applications, if anything they'll just be a tiny bit smaller because they can now strip unused symbols. ACKs for top commit: real-or-random: utACK 6f6cab9 theuni: > Not sure what other changes made compilation on CI fail but Concept ACK [6f6cab9](bitcoin-core/secp256k1@6f6cab9). This should be entirely harmless. sipa: utACK 6f6cab9 laanwj: utACK 6f6cab9 Tree-SHA512: 39f240046639738f7a8c01068e728b2f9ceac2754cc4b0a5fa46c28f6f57a8c4124653b56dfbf5c13106b07c11ac599cc41b508e16862d539ce1af6c3365a205
This adds MSVC builds built on Linux using wine. This requires some settings of tools and flags because the autotools support for MSVC is naturally somewhat limited. The advantage of this approach is that it is compatible with our existing CI scripts, so there's no need to write a Windows CI script (in PowerShell or similar). If we want to test building and running on Windows native (e.g., as supported by Cirrus CI) we could still do this in the future. Another advantage of this approach is that contributors can simply use the docker image if they need a MSVC installation in a non-Windows environment. This commit also improves the Dockerfile by grouping RUN commands according to Docker docs: https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run
This commit also raises the TEST_ITERS for wine tasks to the default. The overhead of wine is negligible, so we can certainly afford the same number of iterations as for native Linux tests.
49e2acd configure: Improve rationale for WERROR_CFLAGS (Tim Ruffing) 8dc4b03 ci: Add a C++ job that compiles the public headers without -fpermissive (Tim Ruffing) 51f296a ci: Run persistent wineserver to speed up wine (Tim Ruffing) 3fb3269 ci: Add 32-bit MinGW64 build (Tim Ruffing) 9efc2e5 ci: Add MSVC builds (Tim Ruffing) 2be6ba0 configure: Convince autotools to work with MSVC's archiver lib.exe (Tim Ruffing) bd81f41 schnorrsig bench: Suppress a stupid warning in MSVC (Tim Ruffing) 09f3d71 configure: Add a few CFLAGS for MSVC (Tim Ruffing) 3b4f3d0 build: Reject C++ compilers in the preprocessor (Tim Ruffing) 1cc0941 configure: Don't abort if the compiler does not define __STDC__ (Tim Ruffing) cca8cbb configure: Output message when checking for valgrind (Tim Ruffing) 1a6be57 bench: Make benchmarks compile on MSVC (Tim Ruffing) Pull request description: ACKs for top commit: jonasnick: ACK 49e2acd Tree-SHA512: 986c498fb218231fff3519167d34a92e11dea6a4383788a9723be105c20578cd483c6b06ba5686c6669e3a02cfeebc29b8e5f1428552ebf4ec67fa7a86957548
…HECK` after invalid scrach space check 1827c9b scratch_destroy: move VERIFY_CHECK after invalid scrach space check (siv2r) Pull request description: ACKs for top commit: sipa: utACK 1827c9b jonasnick: ACK 1827c9b Tree-SHA512: 5c4f97ca16f46380b30d1d077a54e25eab21efb10e0ce3ca27e7f73a2318d130f0f0773e26b2fdfc8f9d98c1334880f9d80a51b0941be3ba0af2b656b7c0ea7e
This simplifies building without a build system. This is in line with #925; the paths fixed here were either forgotten there or only introduced later. This commit also makes the Makefile stricter so that further "wrong" #include paths will lead to build errors even in autotools builds. This belongs to #929. Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
… get rid of further -I arguments 40a3473 build: Fix #include "..." paths to get rid of further -I arguments (Tim Ruffing) Pull request description: This simplifies building without a build system. This is in line with #925; the paths fixed here were either forgotten there or only introduced later. This commit also makes the Makefile stricter so that further "wrong" #include paths will lead to build errors even in autotools builds. This belongs to #929. ACKs for top commit: hebasto: ACK 40a3473 Tree-SHA512: 6f4d825ea3cf86b13f294e2ec19fafc29660fa99450e6b579157d7a6e9bdb3404d761edf89c1135fa89b984d6431a527beeb97031dc90f2fae9761528f4d06d1
Running the RNG is pointless if no seed is available because the key will be fixed. The computation just wastes time. Previously, users could avoid this computation at least by asking for a context without signing capabilities. But since 3b0c218 we always build an ecmult_gen context, ignoring the context flags. Moreover, users could never avoid this pointless computation when asking for the creation of a signing context.
Whenever I read this code, I first think that rescaling ctx->initial is a dead store because we overwrite it later with gb. But that's wrong. The rescaling blinds the computation of gb and affects its result.
This simplifies manual builds and solves one item in #929.
… blinding if no seed is available 55f8bc9 ecmult_gen: Improve comments about projective blinding (Tim Ruffing) 7a86955 ecmult_gen: Simplify code (no observable change) (Tim Ruffing) 4cc0b1b ecmult_gen: Skip RNG when creating blinding if no seed is available (Tim Ruffing) Pull request description: Running the RNG is pointless if no seed is available because the key will be fixed. The computation just wastes time. Previously, users could avoid this computation at least by asking for a context without signing capabilities. But since 3b0c218 we always build an ecmult_gen context, ignoring the context flags. Moreover, users could never avoid this pointless computation when asking for the creation of a signing context. This fixes one item in #1065. ACKs for top commit: sipa: ACK 55f8bc9 apoelstra: ACK 55f8bc9 Tree-SHA512: 5ccba56041f94fa8f40a8a56ce505369ff2e0ed20cd7f0bfc3fdfffa5fa7bf826a93602b9b2455a352865a9548ab4928e858c19bb5af7ec221594a3bf25c4f3d
…nset variables as an error 7a74688 ci: add missing CFLAGS & CPPFLAGS variable to print_environment (Jonas Nick) c2e0fda ci: set -u in cirrus.sh to treat unset variables as an error (Jonas Nick) Pull request description: This PR is supposed to prevent accidental misuse of cirrus.sh. Maybe there is a way to check if `CC`, `AR` and `NM` are set within the loop that deals with the other variables, but so far I did not come up with one (that's POSIX shell compliant). ACKs for top commit: real-or-random: ACK 7a74688 hebasto: re-ACK 7a74688 Tree-SHA512: 91e42b3f1192fbf86e6fb43942713e78b2bee977ddd95256ea7448f84324369399d31ec4eedd47af595bf994bbc9396e26bb5c93bdb7f58c4310b5d3d5d66731
9c5a4d2 Do not define unused `HAVE_VALGRIND` macro (Hennadii Stepanov) ad8647f Drop no longer relevant files from `.gitignore` (Hennadii Stepanov) b627ba7 Remove dependency on `src/libsecp256k1-config.h` (Hennadii Stepanov) Pull request description: Cherry-picked the first commit from #1142 and addressed a [comment](bitcoin-core/secp256k1#1142 (comment)). ACKs for top commit: sipa: utACK 9c5a4d2 real-or-random: utACK 9c5a4d2 Tree-SHA512: c6f268261fc5edee855a7e69fdf9f6c5f4b859eb1e078e3c44c3ee4c9c445738af3de9fc2fbcca90db9b9e38681da8217faaeb0735201052b16ea397a7817db9
c30b889 Clarify that the ABI-incompatible versions are earlier (Pieter Wuille) 881fc33 Consistency in naming of modules (Pieter Wuille) 9ecf814 Reduce font size in changelog (Pieter Wuille) 2dc133a Add more changelog entries (Pieter Wuille) ac233e1 Add links to diffs to changelog (Pieter Wuille) cee8223 Mention semantic versioning in changelog (Pieter Wuille) Pull request description: ACKs for top commit: real-or-random: ACK c30b889 jonasnick: ACK c30b889 Tree-SHA512: 0f753eae0ea4d65035bfbcd81b90169111ea030cf7196dd072fb1ccc8aac1437768031f3fcef431584028da68b66873204e16e03bcde4a6ae96b08ab7f97a480
…CHECK_VOID which returns (void) a49e094 docs: Fix typo (Tim Ruffing) 2551cda tests: Fix code formatting (Tim Ruffing) c635c1b Change ARG_CHECK_NO_RETURN to ARG_CHECK_VOID which returns (void) (Tim Ruffing) cf66f23 refactor: Add helper function secp256k1_context_is_proper() (Tim Ruffing) Pull request description: ACKs for top commit: sipa: utACK a49e094 jonasnick: ACK a49e094 Tree-SHA512: 0fd4ee88510f2de0de96378ae69ce6e610a446000bb78597026c5924803e1ce5a4f76303fc6446233a6129f9c42dce1b1549f93bef935131101e47b5a69cdf2f
d216475 test secp256k1_i128_to_i64 (Russell O'Connor) 4bc4290 Add a secp256k1_i128_to_u64 function. (Russell O'Connor) Pull request description: I wanted to experiment with what would be required to split up `secp256k1_i128_to_i64` between those cases when a signed 64 bit value is being demoted, versus an unsigned 64 bit value is being extracted from the lower bits, and this is the result. I'm not sure this is a useful PR, so feel free to close it. However, since it is already written, I figured it is worth at least discussing. ACKs for top commit: sipa: utACK d216475 real-or-random: ACK d216475 Tree-SHA512: 41dbb1d33b3078bee8e71a838cfad6f1859c0bba602ae061259add8e9e8ea5aa482daa41de79dbd7433ddbef4a0bc52757f3c45d63acc9c0eb05aa3ca891b922
…o bench compilation c0a555b Bugfix: pass SECP_CONFIG_DEFINES to bench compilation (Pieter Wuille) Pull request description: ACKs for top commit: real-or-random: utACK c0a555b apoelstra: ACK c0a555b Tree-SHA512: 4ec6ca4c012166beb6c5bdd1b2ed939554415e03545c176cf281000145c4000a460e231d5da26f617a81b048cd0fa3f8f16b61a207aed9479fdd854483e35ded
User applications shouldn't need or rely on `SECP_CONFIG_DEFINES`.
…amples 2f9ca28 Drop `SECP_CONFIG_DEFINES` from examples (Hennadii Stepanov) Pull request description: User applications shouldn't need or rely on `SECP_CONFIG_DEFINES`. See bitcoin-core/secp256k1#1178 (comment). ACKs for top commit: sipa: utACK 2f9ca28 real-or-random: utACK 2f9ca28 Tree-SHA512: c8e81e6842b31e7f4ebcbb18d5962f7d7308f024025d6225330a7ec099739278bb43ad98243698c5802bcc49bf7e247ab7cae7f40008fbba87f0d0e46cbe1e85
39e8f0e refactor: Separate run_context_tests into static vs proper contexts (Tim Ruffing) a4a0937 tests: Clean up and improve run_context_tests() further (Tim Ruffing) fc90bb5 refactor: Tidy up main() (Tim Ruffing) f32a36f tests: Don't use global context for context tests (Tim Ruffing) ce4f936 tests: Tidy run_context_tests() by extracting functions (Tim Ruffing) 18e0db3 tests: Don't recreate global context in scratch space test (Tim Ruffing) b198061 tests: Use global copy of secp256k1_context_static instead of clone (Tim Ruffing) Pull request description: This is an improved version of some of the tidying/refactoring in #1170. I think it's enough to deserve a separate PR. Once this is merged, I'll get back to the actual goal of #1170 (namely, forbidding cloning and randomizing static contexts.) This PR is a general clean up of the context tests. A notable change is that this avoids a code smell where `run_context_tests()` would use the global `ctx` variable like a local one (i.e., create a context in it and destroy it afterwards). After this PR, the global `ctx` is properly initialized for all the other tests, and they can decide whether they want to use it or not. Same for a global `sttc`, which is a memcpy of the static context (we need a writable copy in order to be able to set callbacks). Note that this touches code which is also affected by #1167 but I refrained from trying to solve this issue. The goal of this PR is simply not to worsen the situation w.r.t. #1167. We should really introduce a macro to solve #1167 but that's another PR. ACKs for top commit: sipa: utACK 39e8f0e apoelstra: ACK 39e8f0e Tree-SHA512: a22471758111061a062b126a52a0de24a1a311d1a0332a4ef006882379a4f3f2b00e53089e3c374bf47c4051bb10bbc6a9fdbcf6d0cd4eca15b5703590395fba
… like tests but without VERIFY 2037600 tests: Add noverify_tests which is like tests but without VERIFY (Tim Ruffing) Pull request description: mentioned in bitcoin-core/secp256k1#1037 (comment) Let's see how this affects CI time ACKs for top commit: sipa: ACK 2037600 apoelstra: ACK 2037600 Tree-SHA512: fab1ce1499d418671d3d0ecfddf15d75b7c2bbfbfb4be958a95730491244185a906c7133aba4d0bec56ee6c721cb525750eef4cafc12f386484af931e34b0e8e
…in tests 9a93f48 refactor: Rename STTC to STATIC_CTX in tests (Tim Ruffing) 3385a26 refactor: Rename global variables to uppercase in tests (Tim Ruffing) Pull request description: On top of #1186 . I feel that this is an improvement, but it touches a lot of lines and so it deserves a separate discussion. ACKs for top commit: sipa: ACK 9a93f48 Tree-SHA512: b6dad2ffff2267034bf8cefdd3ef7ea11e9bcb8142d64b460ca61e0d3ab8de22fb3ee994dea0fb32feee3864d07395c070abffab318690d09d104294895300c4
Linking cannot possibly work without a main() function. Use AC_LANG_PROGRAM, which takes care of this automatically.
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.
Linking cannot possibly work without a main() function.
Use AC_LANG_PROGRAM, which takes care of this automatically.
Draft because it builds on an umerged branch that should be merged into master first.