-
-
Notifications
You must be signed in to change notification settings - Fork 354
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
[DRAFT] secp256k1
: CMake build system added.
#1501
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
function(check_arm32_assembly) | ||
try_compile(HAVE_ARM32_ASM | ||
${PROJECT_BINARY_DIR}/check_arm32_assembly | ||
SOURCES ${PROJECT_SOURCE_DIR}/cmake/source_arm32.s | ||
) | ||
endfunction() | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
include_guard(GLOBAL) | ||
include(CheckCSourceCompiles) | ||
|
||
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() | ||
Comment on lines
+4
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix preprocessor directives and improve function documentation. The function has several issues that need to be addressed:
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() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
function(check_string_option_value option) | ||
get_property(expected_values CACHE ${option} PROPERTY STRINGS) | ||
if(expected_values) | ||
if(${option} IN_LIST expected_values) | ||
return() | ||
endif() | ||
message(FATAL_ERROR "${option} value is \"${${option}}\", but must be one of ${expected_values}.") | ||
endif() | ||
message(AUTHOR_WARNING "The STRINGS property must be set before invoking `check_string_option_value' function.") | ||
endfunction() |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,14 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
include(CheckCSourceCompiles) | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
function(check_x86_64_assembly) | ||||||||||||||||||||||||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+4
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Several improvements needed in the assembly test The assembly test has a few issues that should be addressed:
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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
set(HAVE_X86_64_ASM ${HAVE_X86_64_ASM} PARENT_SCOPE) | ||||||||||||||||||||||||||||||||||||||||||||||||||
endfunction() |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,26 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# Try to find the GNU Multiple Precision Arithmetic Library (GMP) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# See http://gmplib.org/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (GMP_INCLUDES AND GMP_LIBRARIES) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
set(GMP_FIND_QUIETLY TRUE) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
endif (GMP_INCLUDES AND GMP_LIBRARIES) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
find_path(GMP_INCLUDES | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
NAMES | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
gmp.h | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
PATHS | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
$ENV{GMPDIR} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
${INCLUDE_INSTALL_DIR} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
find_library(GMP_LIBRARIES gmp PATHS $ENV{GMPDIR} ${LIB_INSTALL_DIR}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+8
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance GMP detection robustness. The current implementation could be improved in several ways:
+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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+18
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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()
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
include(FindPackageHandleStandardArgs) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
find_package_handle_standard_args(GMP DEFAULT_MSG | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GMP_INCLUDES GMP_LIBRARIES) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
mark_as_advanced(GMP_INCLUDES GMP_LIBRARIES) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
if(CMAKE_HOST_APPLE) | ||
find_program(BREW_COMMAND brew) | ||
execute_process( | ||
COMMAND ${BREW_COMMAND} --prefix valgrind | ||
OUTPUT_VARIABLE valgrind_brew_prefix | ||
ERROR_QUIET | ||
OUTPUT_STRIP_TRAILING_WHITESPACE | ||
) | ||
endif() | ||
|
||
set(hints_paths) | ||
if(valgrind_brew_prefix) | ||
set(hints_paths ${valgrind_brew_prefix}/include) | ||
endif() | ||
|
||
find_path(Valgrind_INCLUDE_DIR | ||
NAMES valgrind/memcheck.h | ||
HINTS ${hints_paths} | ||
) | ||
|
||
if(Valgrind_INCLUDE_DIR) | ||
include(CheckCSourceCompiles) | ||
set(CMAKE_REQUIRED_INCLUDES ${Valgrind_INCLUDE_DIR}) | ||
check_c_source_compiles(" | ||
#include <valgrind/memcheck.h> | ||
#if defined(NVALGRIND) | ||
# error \"Valgrind does not support this platform.\" | ||
#endif | ||
|
||
int main() {} | ||
" Valgrind_WORKS) | ||
endif() | ||
|
||
include(FindPackageHandleStandardArgs) | ||
find_package_handle_standard_args(Valgrind | ||
REQUIRED_VARS Valgrind_INCLUDE_DIR Valgrind_WORKS | ||
) | ||
|
||
mark_as_advanced( | ||
Valgrind_INCLUDE_DIR | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
function(generate_pkg_config_file in_file) | ||
set(prefix ${CMAKE_INSTALL_PREFIX}) | ||
set(exec_prefix \${prefix}) | ||
set(libdir \${exec_prefix}/${CMAKE_INSTALL_LIBDIR}) | ||
set(includedir \${prefix}/${CMAKE_INSTALL_INCLUDEDIR}) | ||
set(PACKAGE_VERSION ${PROJECT_VERSION}) | ||
configure_file(${in_file} ${PROJECT_NAME}.pc @ONLY) | ||
endfunction() |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,24 @@ | ||||||||||||||||||||||||||||||||||||||||||||
include(CheckCCompilerFlag) | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
function(secp256k1_check_c_flags_internal flags output) | ||||||||||||||||||||||||||||||||||||||||||||
string(MAKE_C_IDENTIFIER "${flags}" result) | ||||||||||||||||||||||||||||||||||||||||||||
string(TOUPPER "${result}" result) | ||||||||||||||||||||||||||||||||||||||||||||
set(result "C_SUPPORTS_${result}") | ||||||||||||||||||||||||||||||||||||||||||||
if(NOT MSVC) | ||||||||||||||||||||||||||||||||||||||||||||
set(CMAKE_REQUIRED_FLAGS "-Werror") | ||||||||||||||||||||||||||||||||||||||||||||
endif() | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
# This avoids running a linker. | ||||||||||||||||||||||||||||||||||||||||||||
set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) | ||||||||||||||||||||||||||||||||||||||||||||
check_c_compiler_flag("${flags}" ${result}) | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
set(${output} ${${result}} PARENT_SCOPE) | ||||||||||||||||||||||||||||||||||||||||||||
endfunction() | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
# Append flags to the COMPILE_OPTIONS directory property if CC accepts them. | ||||||||||||||||||||||||||||||||||||||||||||
macro(try_append_c_flags) | ||||||||||||||||||||||||||||||||||||||||||||
secp256k1_check_c_flags_internal("${ARGV}" result) | ||||||||||||||||||||||||||||||||||||||||||||
if(result) | ||||||||||||||||||||||||||||||||||||||||||||
add_compile_options(${ARGV}) | ||||||||||||||||||||||||||||||||||||||||||||
endif() | ||||||||||||||||||||||||||||||||||||||||||||
endmacro() | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+19
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
set(CMAKE_SYSTEM_NAME Linux) | ||
set(CMAKE_SYSTEM_PROCESSOR arm) | ||
set(CMAKE_C_COMPILER arm-linux-gnueabihf-gcc) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
@PACKAGE_INIT@ | ||
|
||
include("${CMAKE_CURRENT_LIST_DIR}/@[email protected]") | ||
|
||
check_required_components(@PROJECT_NAME@) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
.syntax unified | ||
.eabi_attribute 24, 1 | ||
.eabi_attribute 25, 1 | ||
.text | ||
.global main | ||
main: | ||
ldr r0, =0x002A | ||
mov r7, #1 | ||
swi 0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
set(CMAKE_SYSTEM_NAME Windows) | ||
set(CMAKE_SYSTEM_PROCESSOR x86_64) | ||
set(CMAKE_C_COMPILER x86_64-w64-mingw32-gcc) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
# Must be included before CMAKE_INSTALL_INCLUDEDIR is used. | ||
include(GNUInstallDirs) | ||
|
||
# Add objects explicitly rather than linking to the object libs to keep them | ||
# from being exported. | ||
add_library(secp256k1 OBJECT secp256k1.c) | ||
|
||
target_link_libraries(secp256k1 PUBLIC gmp) | ||
|
||
set_property(TARGET secp256k1 PROPERTY POSITION_INDEPENDENT_CODE ON) | ||
|
||
add_library(secp256k1pp | ||
cpp/GroupElement.cpp | ||
cpp/MultiExponent.cpp | ||
cpp/Scalar.cpp | ||
) | ||
|
||
target_link_libraries(secp256k1pp | ||
PUBLIC secp256k1 | ||
) | ||
|
||
add_library(secp256k1_asm INTERFACE) | ||
if(SECP256K1_ASM STREQUAL "arm32") | ||
add_library(secp256k1_asm_arm OBJECT EXCLUDE_FROM_ALL) | ||
target_sources(secp256k1_asm_arm PUBLIC | ||
asm/field_10x26_arm.s | ||
) | ||
target_sources(secp256k1 PRIVATE $<TARGET_OBJECTS:secp256k1_asm_arm>) | ||
target_link_libraries(secp256k1_asm INTERFACE secp256k1_asm_arm) | ||
endif() | ||
|
||
if(WIN32) | ||
# Define our export symbol only for shared libs. | ||
set_target_properties(secp256k1 PROPERTIES DEFINE_SYMBOL SECP256K1_DLL_EXPORT) | ||
target_compile_definitions(secp256k1 INTERFACE $<$<NOT:$<BOOL:${BUILD_SHARED_LIBS}>>:SECP256K1_STATIC>) | ||
endif() | ||
|
||
# Object libs don't know if they're being built for a shared or static lib. | ||
# Grab the PIC property from secp256k1 which knows. | ||
get_target_property(use_pic secp256k1 POSITION_INDEPENDENT_CODE) | ||
|
||
target_include_directories(secp256k1 INTERFACE | ||
# Add the include path for parent projects so that they don't have to manually add it. | ||
$<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>> | ||
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}> | ||
) | ||
|
||
# This emulates Libtool to make sure Libtool and CMake agree on the ABI version, | ||
# see below "Calculate the version variables" in build-aux/ltmain.sh. | ||
math(EXPR ${PROJECT_NAME}_soversion "${${PROJECT_NAME}_LIB_VERSION_CURRENT} - ${${PROJECT_NAME}_LIB_VERSION_AGE}") | ||
set_target_properties(secp256k1 PROPERTIES | ||
SOVERSION ${${PROJECT_NAME}_soversion} | ||
) | ||
if(CMAKE_SYSTEM_NAME STREQUAL "Linux") | ||
set_target_properties(secp256k1 PROPERTIES | ||
VERSION ${${PROJECT_NAME}_soversion}.${${PROJECT_NAME}_LIB_VERSION_AGE}.${${PROJECT_NAME}_LIB_VERSION_REVISION} | ||
) | ||
elseif(APPLE) | ||
if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.17) | ||
math(EXPR ${PROJECT_NAME}_compatibility_version "${${PROJECT_NAME}_LIB_VERSION_CURRENT} + 1") | ||
set_target_properties(secp256k1 PROPERTIES | ||
MACHO_COMPATIBILITY_VERSION ${${PROJECT_NAME}_compatibility_version} | ||
MACHO_CURRENT_VERSION ${${PROJECT_NAME}_compatibility_version}.${${PROJECT_NAME}_LIB_VERSION_REVISION} | ||
) | ||
unset(${PROJECT_NAME}_compatibility_version) | ||
elseif(BUILD_SHARED_LIBS) | ||
message(WARNING | ||
"The 'compatibility version' and 'current version' values of the DYLIB " | ||
"will diverge from the values set by the GNU Libtool. To ensure " | ||
"compatibility, it is recommended to upgrade CMake to at least version 3.17." | ||
) | ||
endif() | ||
elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows") | ||
set(${PROJECT_NAME}_windows "secp256k1") | ||
if(MSVC) | ||
set(${PROJECT_NAME}_windows "${PROJECT_NAME}") | ||
endif() | ||
set_target_properties(secp256k1 PROPERTIES | ||
ARCHIVE_OUTPUT_NAME "${${PROJECT_NAME}_windows}" | ||
RUNTIME_OUTPUT_NAME "${${PROJECT_NAME}_windows}-${${PROJECT_NAME}_soversion}" | ||
) | ||
unset(${PROJECT_NAME}_windows) | ||
endif() | ||
unset(${PROJECT_NAME}_soversion) | ||
|
||
if(SECP256K1_BUILD_BENCHMARK) | ||
add_executable(bench_ecdh bench_ecdh.c) | ||
target_link_libraries(bench_ecdh PUBLIC secp256k1) | ||
add_executable(bench_recover bench_recover.c) | ||
target_link_libraries(bench_recover PUBLIC secp256k1) | ||
add_executable(bench_sign bench_sign.c) | ||
target_link_libraries(bench_sign PUBLIC secp256k1) | ||
add_executable(bench_verify bench_verify.c) | ||
target_link_libraries(bench_verify PUBLIC secp256k1) | ||
add_executable(bench_internal bench_internal.c) | ||
target_link_libraries(bench_internal PUBLIC secp256k1_asm gmp) | ||
endif() | ||
|
||
if(SECP256K1_BUILD_TESTS) | ||
add_executable(noverify_tests tests.c) | ||
target_compile_definitions(noverify_tests PRIVATE VERIFY) | ||
target_link_libraries(noverify_tests secp256k1_asm gmp) | ||
Comment on lines
+101
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 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
Recommendations:
🔗 Analysis chainVerify 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 executedThe 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 |
||
add_test(NAME secp256k1_noverify_tests COMMAND noverify_tests) | ||
if(NOT CMAKE_BUILD_TYPE STREQUAL "Coverage") | ||
add_executable(tests tests.c) | ||
target_compile_definitions(tests PRIVATE VERIFY) | ||
target_link_libraries(tests secp256k1_asm gmp) | ||
add_test(NAME secp256k1_tests COMMAND tests) | ||
endif() | ||
endif() | ||
|
||
if(SECP256K1_BUILD_EXHAUSTIVE_TESTS) | ||
# Note: do not include secp256k1_precomputed in exhaustive_tests (it uses runtime-generated tables). | ||
add_executable(exhaustive_tests tests_exhaustive.c) | ||
target_link_libraries(exhaustive_tests secp256k1_asm gmp) | ||
target_compile_definitions(exhaustive_tests PRIVATE $<$<NOT:$<CONFIG:Coverage>>:VERIFY>) | ||
add_test(NAME secp256k1_exhaustive_tests COMMAND exhaustive_tests) | ||
endif() | ||
|
||
if(SECP256K1_INSTALL) | ||
install(TARGETS secp256k1 | ||
EXPORT ${PROJECT_NAME}-targets | ||
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} | ||
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
) | ||
set(${PROJECT_NAME}_headers | ||
"${PROJECT_SOURCE_DIR}/include/secp256k1.h" | ||
) | ||
if(SECP256K1_ENABLE_MODULE_ECDH) | ||
list(APPEND ${PROJECT_NAME}_headers "${PROJECT_SOURCE_DIR}/include/secp256k1_ecdh.h") | ||
endif() | ||
if(SECP256K1_ENABLE_MODULE_RECOVERY) | ||
list(APPEND ${PROJECT_NAME}_headers "${PROJECT_SOURCE_DIR}/include/secp256k1_recovery.h") | ||
endif() | ||
if(SECP256K1_ENABLE_MODULE_SCHNORR) | ||
list(APPEND ${PROJECT_NAME}_headers "${PROJECT_SOURCE_DIR}/include/secp256k1_schnorr.h") | ||
endif() | ||
install(FILES ${${PROJECT_NAME}_headers} | ||
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} | ||
) | ||
|
||
install(EXPORT ${PROJECT_NAME}-targets | ||
FILE ${PROJECT_NAME}-targets.cmake | ||
NAMESPACE ${PROJECT_NAME}:: | ||
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME} | ||
) | ||
|
||
include(CMakePackageConfigHelpers) | ||
configure_package_config_file( | ||
${PROJECT_SOURCE_DIR}/cmake/config.cmake.in | ||
${PROJECT_NAME}-config.cmake | ||
INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME} | ||
NO_SET_AND_CHECK_MACRO | ||
) | ||
write_basic_package_version_file(${PROJECT_NAME}-config-version.cmake | ||
COMPATIBILITY SameMinorVersion | ||
) | ||
|
||
install( | ||
FILES | ||
${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}-config.cmake | ||
${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}-config-version.cmake | ||
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME} | ||
) | ||
|
||
include(GeneratePkgConfigFile) | ||
generate_pkg_config_file(${PROJECT_SOURCE_DIR}/libsecp256k1.pc.in) | ||
install( | ||
FILES | ||
${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}.pc | ||
DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig | ||
) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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:
Changes:
📝 Committable suggestion