[DRAFT] secp256k1: CMake build system added.
#1501
Conversation
WalkthroughThe changes introduce a comprehensive CMake configuration for the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
src/secp256k1/cmake/CheckMemorySanitizer.cmake (2)
5-5: Consider adding compiler check.The
STATIC_LIBRARYtarget type might not be necessary for all compilers. Consider adding a check for the compiler vendor.function(check_memory_sanitizer output) + if(CMAKE_C_COMPILER_ID MATCHES "Clang") set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) + endif()
16-17: Consider adding cache variable.The result of the MSan check could be cached to avoid repeated checks during configuration.
- " HAVE_MSAN) - set(${output} ${HAVE_MSAN} PARENT_SCOPE) + " HAVE_MSAN CACHE INTERNAL "Whether MemorySanitizer is supported") + set(${output} ${HAVE_MSAN} PARENT_SCOPE)src/secp256k1/cmake/CheckX86_64Assembly.cmake (2)
1-3: Consider a more specific function nameWhile
check_x86_64_assemblyis clear, consider renaming it tocheck_x86_64_mulq_supportto better reflect its specific purpose of testing the mulq instruction support.
13-14: Cache the result and add documentationConsider caching the result and adding documentation about the variable's purpose.
+ # Cache the result to avoid repeated checks + set(HAVE_X86_64_ASM ${HAVE_X86_64_ASM} CACHE BOOL "Whether x86_64 assembly with mulq is supported") set(HAVE_X86_64_ASM ${HAVE_X86_64_ASM} PARENT_SCOPE)src/secp256k1/cmake/source_arm32.s (1)
1-9: Simplify the ARM assembly test fileWhile the current assembly code is valid, it's more complex than needed for a simple compilation test. Since we're only checking if the compiler can handle ARM assembly syntax, we could simplify this to just architecture-specific directives.
Consider simplifying the test file to:
.syntax unified .eabi_attribute 24, 1 .eabi_attribute 25, 1 -.text -.global main -main: - ldr r0, =0x002A - mov r7, #1 - swi 0This would still verify ARM assembly support without potentially triggering actual execution during the test.
src/secp256k1/cmake/CheckStringOptionValue.cmake (2)
1-1: Add function documentation.Please add documentation above the function to describe its purpose, parameters, and usage example.
+# Validates that a CMake option's value is one of its expected values. +# The option must have the STRINGS cache property set before calling this function. +# +# Parameters: +# option - The name of the CMake option to validate +# +# Example usage: +# set_property(CACHE MY_OPTION PROPERTY STRINGS "value1" "value2") +# check_string_option_value(MY_OPTION) function(check_string_option_value option)
7-7: Improve error message formatting.The error message could be more readable with quotes around the expected values list.
- message(FATAL_ERROR "${option} value is \"${${option}}\", but must be one of ${expected_values}.") + message(FATAL_ERROR "${option} value is \"${${option}}\", but must be one of: \"${expected_values}\"")src/secp256k1/cmake/FindGMP.cmake (1)
1-3: Add module documentation.Please add comprehensive module documentation including:
- Description of what this module does
- Variables that can be set to control the module's behavior
- Variables set by the module
- Example usage
# Try to find the GNU Multiple Precision Arithmetic Library (GMP) # See http://gmplib.org/ +# This module defines the following variables: +# GMP_FOUND - System has GMP +# GMP_INCLUDES - GMP include directory +# GMP_LIBRARIES - GMP library +# GMP_VERSION - GMP version string +# +# The following variables can be set to guide the search: +# GMP_ROOT - GMP install prefix +# GMPDIR - (environment) GMP install prefix +# +# Example usage: +# find_package(GMP REQUIRED) +# target_link_libraries(myapp PRIVATE ${GMP_LIBRARIES})src/secp256k1/cmake/FindValgrind.cmake (1)
1-9: Add error handling for brew command failure.While the Homebrew detection is well implemented, it would be more robust to check if the
BREW_COMMANDwas found before executing it.if(CMAKE_HOST_APPLE) find_program(BREW_COMMAND brew) + if(BREW_COMMAND) execute_process( COMMAND ${BREW_COMMAND} --prefix valgrind OUTPUT_VARIABLE valgrind_brew_prefix ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE ) + endif() endif()src/secp256k1/CMakeLists.txt (1)
38-45: Consider using a more recent C standard.While C90 ensures wide compatibility, it might be too restrictive for modern development. Consider using C99 or C11 for better language features while maintaining good compatibility.
-set(CMAKE_C_STANDARD 90) +set(CMAKE_C_STANDARD 99) set(CMAKE_C_EXTENSIONS OFF) set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_EXTENSIONS OFF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (37)
src/secp256k1/CMakeLists.txt(1 hunks)src/secp256k1/cmake/CheckArm32Assembly.cmake(1 hunks)src/secp256k1/cmake/CheckMemorySanitizer.cmake(1 hunks)src/secp256k1/cmake/CheckStringOptionValue.cmake(1 hunks)src/secp256k1/cmake/CheckX86_64Assembly.cmake(1 hunks)src/secp256k1/cmake/FindGMP.cmake(1 hunks)src/secp256k1/cmake/FindValgrind.cmake(1 hunks)src/secp256k1/cmake/GeneratePkgConfigFile.cmake(1 hunks)src/secp256k1/cmake/TryAppendCFlags.cmake(1 hunks)src/secp256k1/cmake/arm-linux-gnueabihf.toolchain.cmake(1 hunks)src/secp256k1/cmake/config.cmake.in(1 hunks)src/secp256k1/cmake/source_arm32.s(1 hunks)src/secp256k1/cmake/x86_64-w64-mingw32.toolchain.cmake(1 hunks)src/secp256k1/src/CMakeLists.txt(1 hunks)src/secp256k1/src/bench_ecdh.c(1 hunks)src/secp256k1/src/bench_internal.c(1 hunks)src/secp256k1/src/bench_recover.c(1 hunks)src/secp256k1/src/bench_schnorr_verify.c(1 hunks)src/secp256k1/src/bench_sign.c(1 hunks)src/secp256k1/src/bench_verify.c(1 hunks)src/secp256k1/src/cpp/GroupElement.cpp(1 hunks)src/secp256k1/src/cpp/Scalar.cpp(1 hunks)src/secp256k1/src/cpp/tests.cpp(1 hunks)src/secp256k1/src/gen_context.c(1 hunks)src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java(0 hunks)src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java(0 hunks)src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Util.java(0 hunks)src/secp256k1/src/java/org/bitcoin/Secp256k1Context.java(0 hunks)src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c(0 hunks)src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.h(0 hunks)src/secp256k1/src/java/org_bitcoin_Secp256k1Context.c(0 hunks)src/secp256k1/src/java/org_bitcoin_Secp256k1Context.h(0 hunks)src/secp256k1/src/modules/ecdh/main_impl.h(1 hunks)src/secp256k1/src/modules/recovery/main_impl.h(1 hunks)src/secp256k1/src/secp256k1.c(1 hunks)src/secp256k1/src/tests.c(1 hunks)src/secp256k1/src/tests_exhaustive.c(1 hunks)
💤 Files with no reviewable changes (8)
- src/secp256k1/src/java/org_bitcoin_Secp256k1Context.h
- src/secp256k1/src/java/org_bitcoin_Secp256k1Context.c
- src/secp256k1/src/java/org/bitcoin/Secp256k1Context.java
- src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Util.java
- src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
- src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.h
- src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java
- src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c
✅ Files skipped from review due to trivial changes (17)
- src/secp256k1/src/bench_verify.c
- src/secp256k1/src/bench_recover.c
- src/secp256k1/src/cpp/GroupElement.cpp
- src/secp256k1/cmake/arm-linux-gnueabihf.toolchain.cmake
- src/secp256k1/src/secp256k1.c
- src/secp256k1/cmake/config.cmake.in
- src/secp256k1/src/bench_ecdh.c
- src/secp256k1/src/tests_exhaustive.c
- src/secp256k1/src/bench_sign.c
- src/secp256k1/cmake/x86_64-w64-mingw32.toolchain.cmake
- src/secp256k1/src/bench_schnorr_verify.c
- src/secp256k1/src/modules/ecdh/main_impl.h
- src/secp256k1/src/cpp/tests.cpp
- src/secp256k1/src/modules/recovery/main_impl.h
- src/secp256k1/src/bench_internal.c
- src/secp256k1/src/cpp/Scalar.cpp
- src/secp256k1/src/tests.c
🔇 Additional comments (13)
src/secp256k1/cmake/CheckMemorySanitizer.cmake (1)
1-2: LGTM! Good module setup practices.
The use of include_guard(GLOBAL) prevents multiple inclusions, and the required dependency is properly included.
src/secp256k1/src/gen_context.c (3)
Line range hint 1-89: LGTM for the core functionality
The core logic for generating the static context remains unchanged and correct. The implementation properly:
- Initializes and builds the context
- Generates the header with proper include guards
- Handles cleanup of resources
- Maintains constant-time operations important for cryptographic code
Line range hint 35-38: Verify output path compatibility with CMake build system
The hardcoded output path "src/ecmult_static_context.h" might need adjustment for CMake's build directory structure. Consider making this path configurable via CMake variables.
Consider updating the code to use a CMake-provided output path:
- fp = fopen("src/ecmult_static_context.h","w");
+ const char* output_path = getenv("SECP256K1_CONTEXT_OUTPUT_PATH");
+ fp = fopen(output_path ? output_path : "src/ecmult_static_context.h", "w");10-10: Verify header resolution in CMake build system
The simplified include path "secp256k1.h" aligns with CMake best practices, but we should ensure the build system correctly sets up include directories.
✅ Verification successful
Include path change is correctly configured in CMake
The simplified include path in gen_context.c is properly supported by the CMake configuration:
- The target includes both
../includeand current source directory in BUILD_INTERFACE secp256k1.his correctly located atsrc/secp256k1/include/secp256k1.h- The include directories are set up with both PUBLIC and INTERFACE visibility ensuring proper header resolution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CMake include directory setup for secp256k1.h
# Check if CMakeLists.txt properly sets up include directories
rg -l "include_directories|target_include_directories" --type cmake
# Verify the actual location of secp256k1.h
fd "secp256k1.h$"
Length of output: 185
Script:
#!/bin/bash
# Check CMake include directory configuration for secp256k1
rg "include_directories|target_include_directories" --type cmake src/secp256k1/CMakeLists.txt src/secp256k1/src/CMakeLists.txt
# Check if gen_context.c is properly included in CMake build
rg "gen_context" --type cmake src/secp256k1/src/CMakeLists.txt
Length of output: 428
Script:
#!/bin/bash
# Check the complete target_include_directories configuration
rg -A 3 "target_include_directories" src/secp256k1/src/CMakeLists.txt
# Check how gen_context is handled in the build system
rg -A 5 "add_executable|add_custom_command|add_custom_target" src/secp256k1/src/CMakeLists.txt
Length of output: 1851
src/secp256k1/cmake/CheckX86_64Assembly.cmake (1)
1-14: Ensure proper integration with build system
This assembly check is crucial for secp256k1's performance. Ensure that:
- The result is used to conditionally compile optimized assembly code paths
- There's a fallback implementation when assembly is not supported
- The check is run early enough in the configuration process
- The result is properly propagated to the secp256k1 configuration header
✅ Verification successful
Based on the analysis of the shell script outputs, I can now generate the final response:
Assembly check is properly integrated with build system
The assembly detection is correctly integrated into the build system with proper:
- Conditional compilation through
USE_ASM_X86_64define when x86_64 assembly is available - Architecture-specific field implementations (5x52 for 64-bit, 10x26 for 32-bit)
- Fallback to C implementation when assembly is not available
- Early configuration check that propagates results via CMake variables
The implementation follows best practices by:
- Checking assembly support before selecting implementation
- Using architecture-specific optimizations when available
- Maintaining portable C fallbacks
- Properly propagating configuration through the build system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the assembly detection is properly integrated
echo "Checking for CMake configuration usage..."
rg "HAVE_X86_64_ASM" src/secp256k1/
# Check for fallback implementations
echo "Checking for fallback implementations..."
rg "ifndef.*HAVE_X86_64_ASM" src/secp256k1/
Length of output: 484
Script:
#!/bin/bash
# Let's check the CMakeLists.txt to understand how the assembly check is used
echo "Checking CMakeLists.txt content..."
cat src/secp256k1/CMakeLists.txt
# Let's also check for any assembly-related source files
echo -e "\nChecking for assembly-related source files..."
fd -e asm -e s src/secp256k1/
# Let's look for C fallbacks in field implementation
echo -e "\nChecking for potential C fallbacks in field implementations..."
rg -A 5 "field_" src/secp256k1/src/
Length of output: 63104
src/secp256k1/cmake/GeneratePkgConfigFile.cmake (1)
1-8: 🛠️ Refactor suggestion
Enhance pkg-config file generation robustness and completeness
The current implementation needs several improvements for reliability and completeness:
Consider these enhancements:
-function(generate_pkg_config_file in_file)
+function(generate_pkg_config_file INPUT_FILE OUTPUT_DIR)
+ if(NOT EXISTS "${INPUT_FILE}")
+ message(FATAL_ERROR "Input file ${INPUT_FILE} does not exist")
+ endif()
+
set(prefix ${CMAKE_INSTALL_PREFIX})
set(exec_prefix \${prefix})
set(libdir \${exec_prefix}/${CMAKE_INSTALL_LIBDIR})
set(includedir \${prefix}/${CMAKE_INSTALL_INCLUDEDIR})
+
+ # Additional standard pkg-config variables
+ set(PKG_NAME ${PROJECT_NAME})
set(PACKAGE_VERSION ${PROJECT_VERSION})
- configure_file(${in_file} ${PROJECT_NAME}.pc @ONLY)
+ set(DESCRIPTION ${PROJECT_DESCRIPTION})
+
+ # Ensure output directory exists
+ file(MAKE_DIRECTORY "${OUTPUT_DIR}")
+
+ configure_file(
+ "${INPUT_FILE}"
+ "${OUTPUT_DIR}/${PROJECT_NAME}.pc"
+ @ONLY
+ )
endfunction()Changes:
- Added input file validation
- Added explicit output directory control
- Added more standard pkg-config variables
- Ensured output directory exists
- Used proper path handling
Also, verify that the template file contains all necessary pkg-config fields:
src/secp256k1/cmake/FindValgrind.cmake (3)
16-19: LGTM! Well-structured Valgrind header detection.
The use of find_path with specific header file and hint paths follows CMake best practices.
21-32: LGTM! Robust Valgrind support verification.
The compilation test properly verifies Valgrind support by checking both header inclusion and platform compatibility through the NVALGRIND macro.
34-41: LGTM! Standard-compliant package handling.
Follows CMake best practices by using FindPackageHandleStandardArgs and marking cache variables as advanced.
src/secp256k1/src/CMakeLists.txt (3)
12-16: LGTM! Well-structured include directories.
The include directories are properly configured using generator expressions to handle both build and install scenarios.
18-26: LGTM! Robust assembly support configuration.
The assembly support is well implemented with proper conditional compilation and target linking for ARM32 architecture.
116-179: LGTM! Comprehensive installation setup.
The installation configuration follows CMake best practices:
- Proper component installation
- Generates both CMake config and pkg-config files
- Handles optional module headers correctly
src/secp256k1/CMakeLists.txt (1)
316-339: LGTM! Thorough architecture-specific configuration.
The configuration properly handles:
- __int128 type verification
- Architecture-specific field implementations
- Clear error messages for unsupported configurations
| function(check_memory_sanitizer output) | ||
| set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) | ||
| check_c_source_compiles(" | ||
| #if defined(__has_feature) | ||
| # if __has_feature(memory_sanitizer) | ||
| /* MemorySanitizer is enabled. */ | ||
| # elif | ||
| # error \"MemorySanitizer is disabled.\" | ||
| # endif | ||
| #else | ||
| # error \"__has_feature is not defined.\" | ||
| #endif | ||
| " HAVE_MSAN) | ||
| set(${output} ${HAVE_MSAN} PARENT_SCOPE) | ||
| endfunction() |
There was a problem hiding this comment.
Fix preprocessor directives and improve function documentation.
The function has several issues that need to be addressed:
- The
#elifdirective is incorrect and will cause compilation errors - The error message strings have escaped quotes which is unnecessary in CMake strings
- The function lacks documentation about its purpose and parameters
Apply this diff to fix the issues:
+# Checks if MemorySanitizer (MSan) is supported by the compiler
+# Args:
+# output: Variable name to store the result (TRUE if MSan is supported, FALSE otherwise)
function(check_memory_sanitizer output)
set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
check_c_source_compiles("
#if defined(__has_feature)
# if __has_feature(memory_sanitizer)
/* MemorySanitizer is enabled. */
- # elif
- # error \"MemorySanitizer is disabled.\"
+ # else
+ # error "MemorySanitizer is disabled."
# endif
#else
- # error \"__has_feature is not defined.\"
+ # error "__has_feature is not defined."
#endif
" HAVE_MSAN)
set(${output} ${HAVE_MSAN} PARENT_SCOPE)
endfunction()Additionally, consider adding a usage example in the documentation:
# Example usage:
# check_memory_sanitizer(HAS_MSAN)
# if(HAS_MSAN)
# # Enable MSan-specific flags or options
# endif()| check_c_source_compiles(" | ||
| #include <stdint.h> | ||
| int main() | ||
| { | ||
| uint64_t a = 11, tmp; | ||
| __asm__ __volatile__(\"movq $0x100000000,%1; mulq %%rsi\" : \"+a\"(a) : \"S\"(tmp) : \"cc\", \"%rdx\"); | ||
| } | ||
| " HAVE_X86_64_ASM) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed in the assembly test
The assembly test has a few issues that should be addressed:
- The
tmpvariable is used uninitialized - The magic number
0x100000000lacks documentation - The assembly constraints could be more specific
- No error handling or messaging for compilation failures
Consider applying these improvements:
check_c_source_compiles("
#include <stdint.h>
+ /* Test if 64-bit multiplication (mulq) instruction is available and working correctly.
+ * 0x100000000 is used as it tests both the high and low bits of the result. */
int main()
{
- uint64_t a = 11, tmp;
+ uint64_t a = 11;
+ uint64_t tmp = 0;
__asm__ __volatile__(\"movq $0x100000000,%1; mulq %%rsi\" : \"+a\"(a) : \"S\"(tmp) : \"cc\", \"%rdx\");
+ return 0;
}
- " HAVE_X86_64_ASM)
+ " HAVE_X86_64_ASM
+ FAIL_REGEX "error:"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| check_c_source_compiles(" | |
| #include <stdint.h> | |
| int main() | |
| { | |
| uint64_t a = 11, tmp; | |
| __asm__ __volatile__(\"movq $0x100000000,%1; mulq %%rsi\" : \"+a\"(a) : \"S\"(tmp) : \"cc\", \"%rdx\"); | |
| } | |
| " HAVE_X86_64_ASM) | |
| check_c_source_compiles(" | |
| #include <stdint.h> | |
| /* Test if 64-bit multiplication (mulq) instruction is available and working correctly. | |
| * 0x100000000 is used as it tests both the high and low bits of the result. */ | |
| int main() | |
| { | |
| uint64_t a = 11; | |
| uint64_t tmp = 0; | |
| __asm__ __volatile__(\"movq $0x100000000,%1; mulq %%rsi\" : \"+a\"(a) : \"S\"(tmp) : \"cc\", \"%rdx\"); | |
| return 0; | |
| } | |
| " HAVE_X86_64_ASM | |
| FAIL_REGEX "error:" | |
| ) |
| function(check_arm32_assembly) | ||
| try_compile(HAVE_ARM32_ASM | ||
| ${PROJECT_BINARY_DIR}/check_arm32_assembly | ||
| SOURCES ${PROJECT_SOURCE_DIR}/cmake/source_arm32.s | ||
| ) | ||
| endfunction() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the ARM32 assembly check implementation
While the basic functionality is correct, there are several improvements we can make for robustness:
Consider these enhancements:
-function(check_arm32_assembly)
+function(check_arm32_assembly RESULT_VARIABLE)
try_compile(HAVE_ARM32_ASM
- ${PROJECT_BINARY_DIR}/check_arm32_assembly
+ "${PROJECT_BINARY_DIR}/CMakeFiles/check_arm32_assembly"
SOURCES ${PROJECT_SOURCE_DIR}/cmake/source_arm32.s
)
+ set(${RESULT_VARIABLE} ${HAVE_ARM32_ASM} PARENT_SCOPE)
endfunction()Changes:
- Added explicit result variable parameter for better scope control
- Used more specific binary directory path
- Added proper variable propagation to parent scope
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function(check_arm32_assembly) | |
| try_compile(HAVE_ARM32_ASM | |
| ${PROJECT_BINARY_DIR}/check_arm32_assembly | |
| SOURCES ${PROJECT_SOURCE_DIR}/cmake/source_arm32.s | |
| ) | |
| endfunction() | |
| function(check_arm32_assembly RESULT_VARIABLE) | |
| try_compile(HAVE_ARM32_ASM | |
| "${PROJECT_BINARY_DIR}/CMakeFiles/check_arm32_assembly" | |
| SOURCES ${PROJECT_SOURCE_DIR}/cmake/source_arm32.s | |
| ) | |
| set(${RESULT_VARIABLE} ${HAVE_ARM32_ASM} PARENT_SCOPE) | |
| endfunction() |
| macro(try_append_c_flags) | ||
| secp256k1_check_c_flags_internal("${ARGV}" result) | ||
| if(result) | ||
| add_compile_options(${ARGV}) | ||
| endif() | ||
| endmacro() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and documentation to the macro.
The macro needs proper documentation and should validate its arguments.
+# Attempts to append C compiler flags if they are supported by the compiler.
+# Multiple flags can be provided as separate arguments.
+#
+# Usage:
+# try_append_c_flags(-Wall -Wextra)
macro(try_append_c_flags)
+ if(${ARGC} EQUAL 0)
+ message(AUTHOR_WARNING "try_append_c_flags called with no arguments")
+ return()
+ endif()
secp256k1_check_c_flags_internal("${ARGV}" result)
if(result)
add_compile_options(${ARGV})
endif()
endmacro()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| macro(try_append_c_flags) | |
| secp256k1_check_c_flags_internal("${ARGV}" result) | |
| if(result) | |
| add_compile_options(${ARGV}) | |
| endif() | |
| endmacro() | |
| # Attempts to append C compiler flags if they are supported by the compiler. | |
| # Multiple flags can be provided as separate arguments. | |
| # | |
| # Usage: | |
| # try_append_c_flags(-Wall -Wextra) | |
| macro(try_append_c_flags) | |
| if(${ARGC} EQUAL 0) | |
| message(AUTHOR_WARNING "try_append_c_flags called with no arguments") | |
| return() | |
| endif() | |
| secp256k1_check_c_flags_internal("${ARGV}" result) | |
| if(result) | |
| add_compile_options(${ARGV}) | |
| endif() | |
| endmacro() |
| add_compile_definitions(HAVE_LIBGMP=1) # Define this symbol if libgmp is installed | ||
| add_compile_definitions(USE_NUM_GMP=1) # Define this symbol to use the gmp implementation for num | ||
| add_compile_definitions(USE_FIELD_INV_NUM=1) # Define this symbol to use the num-based field inverse implementation | ||
| add_compile_definitions(USE_SCALAR_INV_NUM=1) # Define this symbol to use the num-based scalar inverse implementation |
There was a problem hiding this comment.
Move compile definitions after GMP verification.
The compile definitions are currently added unconditionally before verifying if GMP is actually found. This could lead to build errors if GMP is not available.
-add_compile_definitions(HAVE_LIBGMP=1) # Define this symbol if libgmp is installed
-add_compile_definitions(USE_NUM_GMP=1) # Define this symbol to use the gmp implementation for num
-add_compile_definitions(USE_FIELD_INV_NUM=1) # Define this symbol to use the num-based field inverse implementation
-add_compile_definitions(USE_SCALAR_INV_NUM=1) # Define this symbol to use the num-based scalar inverse implementation
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(GMP DEFAULT_MSG
GMP_INCLUDES GMP_LIBRARIES)
+if(GMP_FOUND)
+ add_compile_definitions(HAVE_LIBGMP=1) # Define this symbol if libgmp is installed
+ add_compile_definitions(USE_NUM_GMP=1) # Define this symbol to use the gmp implementation for num
+ add_compile_definitions(USE_FIELD_INV_NUM=1) # Define this symbol to use the num-based field inverse implementation
+ add_compile_definitions(USE_SCALAR_INV_NUM=1) # Define this symbol to use the num-based scalar inverse implementation
+endif()Committable suggestion skipped: line range outside the PR's diff.
| find_path(GMP_INCLUDES | ||
| NAMES | ||
| gmp.h | ||
| PATHS | ||
| $ENV{GMPDIR} | ||
| ${INCLUDE_INSTALL_DIR} | ||
| ) | ||
|
|
||
| find_library(GMP_LIBRARIES gmp PATHS $ENV{GMPDIR} ${LIB_INSTALL_DIR}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance GMP detection robustness.
The current implementation could be improved in several ways:
- Add version detection
- Support pkg-config
- Add more standard search paths
+find_package(PkgConfig QUIET)
+if(PKG_CONFIG_FOUND)
+ pkg_check_modules(PC_GMP QUIET gmp)
+endif()
+
find_path(GMP_INCLUDES
NAMES
gmp.h
PATHS
$ENV{GMPDIR}
${INCLUDE_INSTALL_DIR}
+ ${PC_GMP_INCLUDE_DIRS}
+ /usr/include
+ /usr/local/include
)
-find_library(GMP_LIBRARIES gmp PATHS $ENV{GMPDIR} ${LIB_INSTALL_DIR})
+find_library(GMP_LIBRARIES
+ NAMES gmp
+ PATHS
+ $ENV{GMPDIR}
+ ${LIB_INSTALL_DIR}
+ ${PC_GMP_LIBRARY_DIRS}
+ /usr/lib
+ /usr/local/lib
+)
+
+if(GMP_INCLUDES)
+ file(STRINGS "${GMP_INCLUDES}/gmp.h" gmp_version_str REGEX "^#define[\t ]+__GNU_MP_VERSION[\t ]+[0-9]+")
+ string(REGEX REPLACE "^#define[\t ]+__GNU_MP_VERSION[\t ]+([0-9]+).*" "\\1" GMP_VERSION_MAJOR "${gmp_version_str}")
+
+ file(STRINGS "${GMP_INCLUDES}/gmp.h" gmp_version_str REGEX "^#define[\t ]+__GNU_MP_VERSION_MINOR[\t ]+[0-9]+")
+ string(REGEX REPLACE "^#define[\t ]+__GNU_MP_VERSION_MINOR[\t ]+([0-9]+).*" "\\1" GMP_VERSION_MINOR "${gmp_version_str}")
+
+ set(GMP_VERSION "${GMP_VERSION_MAJOR}.${GMP_VERSION_MINOR}")
+endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find_path(GMP_INCLUDES | |
| NAMES | |
| gmp.h | |
| PATHS | |
| $ENV{GMPDIR} | |
| ${INCLUDE_INSTALL_DIR} | |
| ) | |
| find_library(GMP_LIBRARIES gmp PATHS $ENV{GMPDIR} ${LIB_INSTALL_DIR}) | |
| find_package(PkgConfig QUIET) | |
| if(PKG_CONFIG_FOUND) | |
| pkg_check_modules(PC_GMP QUIET gmp) | |
| endif() | |
| find_path(GMP_INCLUDES | |
| NAMES | |
| gmp.h | |
| PATHS | |
| $ENV{GMPDIR} | |
| ${INCLUDE_INSTALL_DIR} | |
| ${PC_GMP_INCLUDE_DIRS} | |
| /usr/include | |
| /usr/local/include | |
| ) | |
| find_library(GMP_LIBRARIES | |
| NAMES gmp | |
| PATHS | |
| $ENV{GMPDIR} | |
| ${LIB_INSTALL_DIR} | |
| ${PC_GMP_LIBRARY_DIRS} | |
| /usr/lib | |
| /usr/local/lib | |
| ) | |
| if(GMP_INCLUDES) | |
| file(STRINGS "${GMP_INCLUDES}/gmp.h" gmp_version_str REGEX "^#define[\t ]+__GNU_MP_VERSION[\t ]+[0-9]+") | |
| string(REGEX REPLACE "^#define[\t ]+__GNU_MP_VERSION[\t ]+([0-9]+).*" "\\1" GMP_VERSION_MAJOR "${gmp_version_str}") | |
| file(STRINGS "${GMP_INCLUDES}/gmp.h" gmp_version_str REGEX "^#define[\t ]+__GNU_MP_VERSION_MINOR[\t ]+[0-9]+") | |
| string(REGEX REPLACE "^#define[\t ]+__GNU_MP_VERSION_MINOR[\t ]+([0-9]+).*" "\\1" GMP_VERSION_MINOR "${gmp_version_str}") | |
| set(GMP_VERSION "${GMP_VERSION_MAJOR}.${GMP_VERSION_MINOR}") | |
| endif() |
src/secp256k1/src/CMakeLists.txt
Outdated
| add_library(secp256k1 | ||
| secp256k1.c | ||
| cpp/GroupElement.cpp | ||
| cpp/MultiExponent.cpp | ||
| cpp/Scalar.cpp | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit dependencies to the library target.
While the library target is well-defined, it should explicitly specify its dependencies (GMP and OpenSSL) that were found in the root CMakeLists.txt.
add_library(secp256k1
secp256k1.c
cpp/GroupElement.cpp
cpp/MultiExponent.cpp
cpp/Scalar.cpp
)
+target_link_libraries(secp256k1
+ PUBLIC
+ GMP::GMP
+ PRIVATE
+ OpenSSL::Crypto
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| add_library(secp256k1 | |
| secp256k1.c | |
| cpp/GroupElement.cpp | |
| cpp/MultiExponent.cpp | |
| cpp/Scalar.cpp | |
| ) | |
| add_library(secp256k1 | |
| secp256k1.c | |
| cpp/GroupElement.cpp | |
| cpp/MultiExponent.cpp | |
| cpp/Scalar.cpp | |
| ) | |
| target_link_libraries(secp256k1 | |
| PUBLIC | |
| GMP::GMP | |
| PRIVATE | |
| OpenSSL::Crypto | |
| ) |
| find_package(GMP REQUIRED) | ||
| find_package(OpenSSL REQUIRED) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add version constraints for dependencies.
The find_package calls for GMP and OpenSSL should specify minimum version requirements to ensure compatibility.
-find_package(GMP REQUIRED)
-find_package(OpenSSL REQUIRED)
+find_package(GMP 6.0.0 REQUIRED)
+find_package(OpenSSL 1.1.0 REQUIRED)Committable suggestion skipped: line range outside the PR's diff.
aleflm
force-pushed
the
dev/aleflm/cmake-build
branch
from
a18249a
to
3e2369a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/secp256k1/cmake/TryAppendCFlags.cmake (1)
3-16: Add input validation for flags parameter.The function should validate that the flags parameter is not empty and contains valid characters to prevent potential CMake errors.
function(secp256k1_check_c_flags_internal flags output) + if("${flags}" STREQUAL "") + message(FATAL_ERROR "secp256k1_check_c_flags_internal: flags parameter cannot be empty") + endif() string(MAKE_C_IDENTIFIER "${flags}" result) string(TOUPPER "${result}" result) set(result "C_SUPPORTS_${result}")src/secp256k1/CMakeLists.txt (2)
284-288: Consider fallback for platforms without __int128.The hard requirement for __int128 might be too restrictive. Consider providing a fallback implementation for platforms that don't support it.
if(${SIZEOF__int128} EQUAL 16) add_compile_definitions(HAVE___INT128=1) else() - message(FATAL_ERROR "secp256k1 requires __int128 type.") + message(WARNING "No __int128 support, using fallback implementation") + add_compile_definitions(USE_FORCE_WIDEMUL_INT64=1) endif()
254-259: Document CACHE variables properly.The SECP256K1_APPEND_CFLAGS variable should include type and documentation in the CACHE command.
-set(SECP256K1_APPEND_CFLAGS "" CACHE STRING "Compiler flags that are appended to the command line after all other flags added by the build system. This variable is intended for debugging and special builds.") +set(SECP256K1_APPEND_CFLAGS "" + CACHE STRING + "Additional compiler flags for debugging and special builds. These flags are appended after all other flags." + FORCE +) +mark_as_advanced(SECP256K1_APPEND_CFLAGS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
src/secp256k1/CMakeLists.txt(1 hunks)src/secp256k1/cmake/CheckArm32Assembly.cmake(1 hunks)src/secp256k1/cmake/CheckMemorySanitizer.cmake(1 hunks)src/secp256k1/cmake/CheckStringOptionValue.cmake(1 hunks)src/secp256k1/cmake/CheckX86_64Assembly.cmake(1 hunks)src/secp256k1/cmake/FindGMP.cmake(1 hunks)src/secp256k1/cmake/FindValgrind.cmake(1 hunks)src/secp256k1/cmake/GeneratePkgConfigFile.cmake(1 hunks)src/secp256k1/cmake/TryAppendCFlags.cmake(1 hunks)src/secp256k1/cmake/arm-linux-gnueabihf.toolchain.cmake(1 hunks)src/secp256k1/cmake/config.cmake.in(1 hunks)src/secp256k1/cmake/source_arm32.s(1 hunks)src/secp256k1/cmake/x86_64-w64-mingw32.toolchain.cmake(1 hunks)src/secp256k1/src/CMakeLists.txt(1 hunks)src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java(0 hunks)src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java(0 hunks)src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Util.java(0 hunks)src/secp256k1/src/java/org/bitcoin/Secp256k1Context.java(0 hunks)src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c(0 hunks)src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.h(0 hunks)src/secp256k1/src/java/org_bitcoin_Secp256k1Context.c(0 hunks)src/secp256k1/src/java/org_bitcoin_Secp256k1Context.h(0 hunks)src/secp256k1/src/secp256k1.c(0 hunks)
💤 Files with no reviewable changes (9)
- src/secp256k1/src/java/org_bitcoin_Secp256k1Context.h
- src/secp256k1/src/java/org_bitcoin_Secp256k1Context.c
- src/secp256k1/src/secp256k1.c
- src/secp256k1/src/java/org/bitcoin/Secp256k1Context.java
- src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Util.java
- src/secp256k1/src/java/org/bitcoin/NativeSecp256k1.java
- src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java
- src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.h
- src/secp256k1/src/java/org_bitcoin_NativeSecp256k1.c
🚧 Files skipped from review as they are similar to previous changes (11)
- src/secp256k1/cmake/source_arm32.s
- src/secp256k1/cmake/config.cmake.in
- src/secp256k1/cmake/CheckX86_64Assembly.cmake
- src/secp256k1/cmake/CheckArm32Assembly.cmake
- src/secp256k1/cmake/CheckStringOptionValue.cmake
- src/secp256k1/cmake/x86_64-w64-mingw32.toolchain.cmake
- src/secp256k1/cmake/arm-linux-gnueabihf.toolchain.cmake
- src/secp256k1/cmake/CheckMemorySanitizer.cmake
- src/secp256k1/cmake/FindGMP.cmake
- src/secp256k1/cmake/GeneratePkgConfigFile.cmake
- src/secp256k1/cmake/FindValgrind.cmake
🔇 Additional comments (3)
src/secp256k1/cmake/TryAppendCFlags.cmake (1)
19-24: Add parameter validation and documentation to the macro.
The macro needs proper documentation and should validate its arguments.
src/secp256k1/src/CMakeLists.txt (1)
6-8: 🛠️ Refactor suggestion
Use target_include_directories instead of include_directories.
Modern CMake best practices recommend using target_include_directories instead of the global include_directories command.
-include_directories(${CMAKE_CURRENT_SOURCE_DIR})
-include_directories(${CMAKE_CURRENT_SOURCE_DIR}/include)
-include_directories(${CMAKE_CURRENT_SOURCE_DIR}/src)
+target_include_directories(secp256k1
+ PUBLIC
+ $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
+ $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
+ PRIVATE
+ ${CMAKE_CURRENT_SOURCE_DIR}
+ ${CMAKE_CURRENT_SOURCE_DIR}/src
+)Likely invalid or redundant comment.
src/secp256k1/CMakeLists.txt (1)
49-50: Add version constraints for dependencies.
The find_package calls for GMP and OpenSSL should specify minimum version requirements to ensure compatibility.
| target_compile_definitions(noverify_tests PRIVATE VERIFY) | ||
| target_link_libraries(noverify_tests secp256k1_asm gmp) |
There was a problem hiding this comment.
💡 Codebase verification
Consider using an interface library for GMP dependencies
The review concern is valid. The test targets are directly linking to GMP, while the main library secp256k1 also exposes GMP as a PUBLIC dependency. This could indeed expose internal implementation details. Here's what I found:
secp256k1library declares GMP as a PUBLIC dependency:target_link_libraries(secp256k1 PUBLIC gmp)- Test targets directly link to GMP:
target_link_libraries(noverify_tests secp256k1_asm gmp) - The FindGMP.cmake adds global compile definitions that expose GMP usage
Recommendations:
- Create a GMP interface library that encapsulates the GMP dependency and its compile definitions
- Make
secp256k1's GMP dependency PRIVATE instead of PUBLIC - Link test targets to the main library only, removing direct GMP dependencies
🔗 Analysis chain
Verify test linking security.
The test targets are directly linking to gmp, which could expose internal implementation details. Consider using an interface library to control symbol visibility.
Also applies to: 107-108
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if test binaries expose GMP symbols
nm ${CMAKE_BINARY_DIR}/bin/tests | grep -i gmp
Length of output: 122
Script:
#!/bin/bash
# Check CMake configuration for GMP library linking
fd CMakeLists.txt --exec grep -l "gmp"
# Check how GMP is used in the codebase
rg -l "gmp" --type cmake
Length of output: 165
Script:
#!/bin/bash
# Check the content of CMakeLists.txt to understand GMP usage and linking
cat src/secp256k1/src/CMakeLists.txt
# Check if there are any interface libraries defined
rg "add_library.*INTERFACE" src/secp256k1/src/CMakeLists.txt
# Check FindGMP.cmake to understand how GMP is configured
cat src/secp256k1/cmake/FindGMP.cmake
Length of output: 7744
PR intention
Adding CMake build to secp256k1 library.