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.

Sign up for GitHub

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

Jump to bottom

cmake: Add --no-undefined linker option #1559

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

cmake: Add --no-undefined linker option #1559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1abae91
cmake: Add `--no-undefined` linker option
hebasto Jun 28, 2024
bc995fd
autotools: Add `--no-undefined` linker option
hebasto Jun 28, 2024
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
5 changes: 5 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,11 @@ if(SECP256K1_BUILD_CTIME_TESTS)
unset(msan_enabled)
endif()

if(NOT SECP256K1_USE_EXTERNAL_DEFAULT_CALLBACKS)
include(TryAppendLinkerFlags)
try_append_linker_flags(-Wl,--no-undefined)
endif()

include(CTest)
# We do not use CTest's BUILD_TESTING because a single toggle for all tests is too coarse for our needs.
mark_as_advanced(BUILD_TESTING)
Expand Down
2 changes: 1 addition & 1 deletion Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ endif
libsecp256k1_la_SOURCES = src/secp256k1.c
libsecp256k1_la_CPPFLAGS = $(SECP_CONFIG_DEFINES)
libsecp256k1_la_LIBADD = $(COMMON_LIB) $(PRECOMPUTED_LIB)
libsecp256k1_la_LDFLAGS = -no-undefined -version-info $(LIB_VERSION_CURRENT):$(LIB_VERSION_REVISION):$(LIB_VERSION_AGE)
libsecp256k1_la_LDFLAGS = -no-undefined -version-info $(LIB_VERSION_CURRENT):$(LIB_VERSION_REVISION):$(LIB_VERSION_AGE) $(SECP_LIB_LDFLAGS)

noinst_PROGRAMS =
if USE_BENCHMARK
Expand Down
16 changes: 16 additions & 0 deletions build-aux/m4/bitcoin_secp.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,22 @@ AC_DEFUN([SECP_TRY_APPEND_CFLAGS], [
AC_SUBST($2)
])

dnl SECP_TRY_APPEND_LDFLAGS(flags, VAR)
dnl Append flags to VAR if a linker accepts them.
AC_DEFUN([SECP_TRY_APPEND_LDFLAGS], [
AC_MSG_CHECKING([if linker supports $1])
SECP_TRY_APPEND_LDFLAGS_saved_LDFLAGS="$LDFLAGS"
LDFLAGS="$1 $LDFLAGS"
AC_LINK_IFELSE([AC_LANG_PROGRAM()], [flag_works=yes], [flag_works=no])
AC_MSG_RESULT($flag_works)
LDFLAGS="$SECP_TRY_APPEND_LDFLAGS_saved_LDFLAGS"
if test x"$flag_works" = x"yes"; then
$2="$$2 $1"
fi
unset flag_works
AC_SUBST($2)
])

dnl SECP_SET_DEFAULT(VAR, default, default-dev-mode)
dnl Set VAR to default or default-dev-mode, depending on whether dev mode is enabled
AC_DEFUN([SECP_SET_DEFAULT], [
Expand Down
32 changes: 32 additions & 0 deletions cmake/TryAppendLinkerFlags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
include_guard(GLOBAL)

include(CheckCSourceCompiles)

function(secp256k1_check_linker_flags_internal flags output)
string(MAKE_C_IDENTIFIER "${flags}" result)
string(TOUPPER "${result}" result)
set(result "LINKER_SUPPORTS_${result}")
set(CMAKE_REQUIRED_FLAGS "${flags}")
if(MSVC)
string(APPEND " /WX")
elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
string(APPEND " -Wl,-fatal_warnings")
else()
string(APPEND " -Wl,--fatal-warnings")
Comment on lines +12 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this depend on the linker and not on the OS?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it can be implemented in a simple way; see https://gitlab.kitware.com/cmake/cmake/-/issues/18209.

However, we can just check each variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I think checking CMAKE_SYSTEM_NAME is good enough and pragmatic. GNU ld doesn't even support the creation of macOS binaries, and even clang's macOS linker ld64.lld uses -fatal_warnings...

But shouldn't we append -Wl,-fatal_warnings or -Wl,--fatal-warnings also for the autotools build?

endif()

# This ensures running a linker.
set(CMAKE_TRY_COMPILE_TARGET_TYPE EXECUTABLE)
check_c_source_compiles("int main() { return 0; }" ${result})

set(${output} ${${result}} PARENT_SCOPE)
endfunction()

# Append flags to the LINK_OPTIONS directory property if a linker accepts them.
macro(try_append_linker_flags)
secp256k1_check_linker_flags_internal("${ARGV}" result)
if(result)
add_link_options(${ARGV})
endif()
unset(result)
endmacro()
3 changes: 3 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,8 @@ fi

if test x"$enable_external_default_callbacks" = x"yes"; then
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DUSE_EXTERNAL_DEFAULT_CALLBACKS=1"
else
SECP_TRY_APPEND_LDFLAGS([-Wl,--no-undefined], [SECP_LIB_LDFLAGS])
fi

###
Expand Down Expand Up @@ -486,6 +488,7 @@ echo " CC = $CC"
echo " CPPFLAGS = $CPPFLAGS"
echo " SECP_CFLAGS = $SECP_CFLAGS"
echo " CFLAGS = $CFLAGS"
echo " SECP_LIB_LDFLAGS = $SECP_LIB_LDFLAGS"
echo " LDFLAGS = $LDFLAGS"

if test x"$print_msan_notice" = x"yes"; then
Expand Down
Loading