Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
385 changes: 385 additions & 0 deletions src/secp256k1/CMakeLists.txt

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions src/secp256k1/cmake/CheckArm32Assembly.cmake
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()
Comment on lines +1 to +6
Copy link

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:

-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:

  1. Added explicit result variable parameter for better scope control
  2. Used more specific binary directory path
  3. 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.

Suggested change
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()

18 changes: 18 additions & 0 deletions src/secp256k1/cmake/CheckMemorySanitizer.cmake
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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix preprocessor directives and improve function documentation.

The function has several issues that need to be addressed:

  1. The #elif directive is incorrect and will cause compilation errors
  2. The error message strings have escaped quotes which is unnecessary in CMake strings
  3. 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()

10 changes: 10 additions & 0 deletions src/secp256k1/cmake/CheckStringOptionValue.cmake
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()
14 changes: 14 additions & 0 deletions src/secp256k1/cmake/CheckX86_64Assembly.cmake
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
Copy link

Choose a reason for hiding this comment

The 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:

  1. The tmp variable is used uninitialized
  2. The magic number 0x100000000 lacks documentation
  3. The assembly constraints could be more specific
  4. 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.

Suggested change
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:"
)

set(HAVE_X86_64_ASM ${HAVE_X86_64_ASM} PARENT_SCOPE)
endfunction()
26 changes: 26 additions & 0 deletions src/secp256k1/cmake/FindGMP.cmake
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
Copy link

Choose a reason for hiding this comment

The 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:

  1. Add version detection
  2. Support pkg-config
  3. 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.

Suggested change
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()


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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(GMP DEFAULT_MSG
GMP_INCLUDES GMP_LIBRARIES)
mark_as_advanced(GMP_INCLUDES GMP_LIBRARIES)
41 changes: 41 additions & 0 deletions src/secp256k1/cmake/FindValgrind.cmake
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
)
8 changes: 8 additions & 0 deletions src/secp256k1/cmake/GeneratePkgConfigFile.cmake
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()
24 changes: 24 additions & 0 deletions src/secp256k1/cmake/TryAppendCFlags.cmake
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
Copy link

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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()

3 changes: 3 additions & 0 deletions src/secp256k1/cmake/arm-linux-gnueabihf.toolchain.cmake
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)
5 changes: 5 additions & 0 deletions src/secp256k1/cmake/config.cmake.in
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@)
9 changes: 9 additions & 0 deletions src/secp256k1/cmake/source_arm32.s
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
3 changes: 3 additions & 0 deletions src/secp256k1/cmake/x86_64-w64-mingw32.toolchain.cmake
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)
174 changes: 174 additions & 0 deletions src/secp256k1/src/CMakeLists.txt
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
Copy link

Choose a reason for hiding this comment

The 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 secp256k1 also exposes GMP as a PUBLIC dependency. This could indeed expose internal implementation details. Here's what I found:

  • secp256k1 library 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

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()
Loading
Loading