cmake: Add hardening, reduce exports, werror and install #32

hebasto · 2023-10-17T22:17:17Z

New CMake variables that affect the build system configuration:

HARDENING
REDUCE_EXPORTS
WERROR

hebasto · 2023-10-17T22:25:01Z

cc @fanquake @theuni @TheCharlatan @luke-jr @willcl-ark @stickies-v @ajtowns @w0xlt @pablomartin4btc @vasild

CMakeLists.txt

…_CXXFLAGS 8cfa22a build: move -fstack-reuse=none to CORE_CXXFLAGS (fanquake) Pull request description: This is not a hardening specific flag, it should be used at all times, regardless of if hardening is enabled or not. Note that this was still the case here, but having this exist in the hardening flags is confusing, and may lead someone to move it inside one of the `use_hardening` blocks, where it would become unused, with `--disable-hardening`. Noticed while reviewing hebasto/bitcoin#32 (comment). ACKs for top commit: theuni: ACK 8cfa22a. Agree it's confusing as-is and this better matches the intent. hebasto: ACK 8cfa22a luke-jr: utACK 8cfa22a TheCharlatan: ACK 8cfa22a Tree-SHA512: 74c3219301398361d06b1ef2257fc9ec18055b1661f8733ee909adefee61e458d70991c32adf0e0450905a7ffbddc99799f5fdac894f4896cfade19f961818df

vasild

Posting review mid-way

CMakeLists.txt

hebasto · 2023-10-20T08:51:30Z

I've dropped the "cmake: Build libbitcoinconsensus library" commit from this PR because I feel like it deserves its own PR, which will be submitted shortly.

And I apologise for a delay with the answer to this comment.

hebasto · 2023-10-20T14:23:35Z

And I apologise for a delay with the answer to this comment.

Answered.

fanquake · 2023-10-20T15:22:05Z

Where is -D_FORTIFY_SOURCE=3? I can't find any mention of it in the CMake files in this branch.

hebasto · 2023-10-20T19:34:38Z

Where is -D_FORTIFY_SOURCE=3? I can't find any mention of it in the CMake files in this branch.

Thanks! Added.

theuni · 2023-10-20T19:56:49Z

CMakeLists.txt

+    try_append_linker_flag(core "/HIGHENTROPYVA")
+    try_append_linker_flag(core "/NXCOMPAT")
+  else()
+    set(fortfy_flags "-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3")


I wonder if these could work the same way as what we discussed with hardening? Add debug/release interface libs?

8cfa22a build: move -fstack-reuse=none to CORE_CXXFLAGS (fanquake) Pull request description: This is not a hardening specific flag, it should be used at all times, regardless of if hardening is enabled or not. Note that this was still the case here, but having this exist in the hardening flags is confusing, and may lead someone to move it inside one of the `use_hardening` blocks, where it would become unused, with `--disable-hardening`. Noticed while reviewing hebasto#32 (comment). ACKs for top commit: theuni: ACK 8cfa22a. Agree it's confusing as-is and this better matches the intent. hebasto: ACK 8cfa22a luke-jr: utACK 8cfa22a TheCharlatan: ACK 8cfa22a Tree-SHA512: 74c3219301398361d06b1ef2257fc9ec18055b1661f8733ee909adefee61e458d70991c32adf0e0450905a7ffbddc99799f5fdac894f4896cfade19f961818df

hebasto · 2023-10-21T22:15:42Z

@theuni

I'm suggesting nested interfaces (if possible). something like:

add_library(hardening INTERFACE)
if(HARDENING)
  try_append_linker_flag(hardening "-Wl,--enable-reloc-section")
endif()
target_link_libraries(core PRIVATE hardening)

I wonder if these could work the same way as what we discussed with hardening? Add debug/release interface libs?

How does this alternative implementation look for you?

theuni · 2023-10-24T17:51:18Z

@theuni
I'm suggesting nested interfaces (if possible). something like:
add_library(hardening INTERFACE)
if(HARDENING)
  try_append_linker_flag(hardening "-Wl,--enable-reloc-section")
endif()
target_link_libraries(core PRIVATE hardening)
I wonder if these could work the same way as what we discussed with hardening? Add debug/release interface libs?

How does this alternative implementation look for you?

Yes, please! Much better :)

hebasto · 2023-10-24T20:37:48Z

Fellow reviewers,

~~Please review #38 first.~~ Thank you!

hebasto · 2023-10-26T10:50:16Z

Rebased.

How does this alternative implementation look for you?

Yes, please! Much better :)

Implemented.

theuni · 2023-10-26T16:56:56Z

Rebased.

How does this alternative implementation look for you?

Yes, please! Much better :)

Implemented.

Note to other reviewers: The "core" lib has become the catch-all lib. Anything that doesn't apply globally can be broken out, as (for ex) hardening is here.

theuni

Makes sense. Left a few questions.

I didn't review the first two commits deeply, so for now I'm mostly just assuming those functions work as intended.

theuni · 2023-10-26T17:05:24Z

CMakeLists.txt

+      try_append_cxx_flags("-fstack-clash-protection" TARGET hardening)
+    endif()
+
+    if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64" OR CMAKE_SYSTEM_PROCESSOR STREQUAL "arm64")


Could you explain why this checks for both? Do we need to do this everywhere we want aarch64?

Could you explain why this checks for both? Do we need to do this everywhere we want aarch64?

It is "arm64" on macOS, and "aarch64" on Linux.

We can use CMAKE_SYSTEM_PROCESSOR MATCHES "^(aarch64|arm64)" though.

Got it. Nah, thanks, it's good as-is.

theuni · 2023-10-26T17:26:50Z

src/CMakeLists.txt

+unset(installable_targets)
+
+if(INSTALL_MAN)
+  install(DIRECTORY ../doc/man/


I'm not really familiar with how our manfiles are handled, but it seems strange to me that we'd install the placeholders rather than files generated at install-time by gen-manpages.py

Presumably the answer comes from the commit message in daf1ebf: "We can't remove them completely because make dist relies on the files being present."

So now that we won't have that restriction anymore, do we need these placeholders?

I'm not really familiar with how our manfiles are handled, but it seems strange to me that we'd install the placeholders rather than files generated at install-time by gen-manpages.py

This branch mimics the Autotoos-based build system in master right now: https://github.com/hebasto/bitcoin/blob/master/doc/man/Makefile.am

Presumably the answer comes from the commit message in daf1ebf: "We can't remove them completely because make dist relies on the files being present."

So now that we won't have that restriction anymore, do we need these placeholders?

That would be possible after migration to CMake, right?

Yeah, I guess this is just copying our current behavior. It's just that our current behavior is only needed (afact) because of autotools.

Maybe just add: "TODO: these stubs are no longer needed. man pages should be generated at install time."

Yeah, I guess this is just copying our current behavior. It's just that our current behavior is only needed (afact) because of autotools.

Maybe just add: "TODO: these stubs are no longer needed. man pages should be generated at install time."

Thanks! Done.

theuni · 2023-10-26T17:40:39Z

CMakeLists.txt

@@ -278,6 +279,12 @@ if(HARDENING)
  target_link_libraries(core INTERFACE hardening)
 endif()

+if(REDUCE_EXPORTS)


Note that this should probably become on by default in the future (once libbitcoinkernel is ok with it).

Historically it was only off by default because of an ancient broken boost impl that didn't properly export its exceptions.

vasild · 2023-10-27T12:00:30Z

Light ACK e4204f4, I don't fully understand all the stuff.

Some comparisons on FreeBSD:

[diff] install autotools vs cmake

--- /tmp/autotools.install      2023-10-27 12:31:40.242919000 +0200
+++ /tmp/cmake.install  2023-10-27 12:32:07.594775000 +0200
@@ -2,22 +2,11 @@
 bin
 bin/bench_bitcoin
 bin/bitcoin-cli
-bin/bitcoin-qt
 bin/bitcoin-tx
 bin/bitcoin-util
 bin/bitcoin-wallet
 bin/bitcoind
 bin/test_bitcoin
-include
-include/bitcoinconsensus.h
-lib
-lib/libbitcoinconsensus.a
-lib/libbitcoinconsensus.la
-lib/libbitcoinconsensus.so
-lib/libbitcoinconsensus.so.0
-lib/libbitcoinconsensus.so.0.0.0
-lib/pkgconfig
-lib/pkgconfig/libbitcoinconsensus.pc
 share
 share/man
 share/man/man1

[diff] compile flags for net.cpp autotools vs cmake

--- /tmp/autotools.compile.flags	2023-10-27 13:25:45.576600000 +0200
+++ /tmp/cmake.compile.flags	2023-10-27 13:29:07.406586000 +0200
@@ -4,53 +4,29 @@
 -DDEBUG
 -DDEBUG_LOCKCONTENTION
 -DDEBUG_LOCKORDER
--DHAVE_BUILD_INFO
+-DENABLE_ZMQ=1  # not sure why ENABLE_ZMQ, USE_NATPMP and USE_UPNP are not in autotools
 -DHAVE_CONFIG_H
--DPROVIDE_FUZZ_MAIN_FUNCTION
 -DRPC_DOC_CHECK
+-DUSE_NATPMP=1
+-DUSE_UPNP=1
 -fcf-protection=full
--fPIE
+-fPIC
 -fstack-clash-protection
 -fstack-protector-all
 -ftrapv
 -g3
--I.
--I.
--I../src/config
--I./leveldb/include
--I./minisketch/include
--I./secp256k1/include
--I./univalue/include
--I/usr/local/include
--I/usr/local/include
+-I/path/to/bitcoin_source/src
+-I/path/to/bitcoin_source/src/leveldb/include
+-I/path/to/bitcoin_source/src/minisketch/include
+-I/path/to/bitcoin_source/src/univalue/include
+-I/path/to/build/src
 -isystem /usr/local/include
--isystem /usr/local/include
 -MD
--MF .deps/libbitcoin_node_a-net.Tpo
--MP
--MT libbitcoin_node_a-net.o
+-MF src/CMakeFiles/bitcoin_node.dir/net.cpp.o.d
+-MT src/CMakeFiles/bitcoin_node.dir/net.cpp.o
 -O0
 -pthread
 -std=c++17
--Wall  # why these -W options are missing from cmake?
--Wconditional-uninitialized
--Wdate-time
--Wdocumentation
 -Werror
--Wextra
--Wformat
--Wformat-security
--Wgnu
--Wimplicit-fallthrough
--Wloop-analysis
--Wno-self-assign
--Wno-unused-parameter
--Woverloaded-virtual
--Wredundant-decls
--Wshadow-field
 -Wstack-protector
--Wsuggest-override
--Wthread-safety
--Wunreachable-code-loop-increment
--Wunused-member-function
--Wvla

[diff] link flags for bitcoind autotools vs cmake

--- /tmp/autotools.link.flags	2023-10-27 13:40:53.495687000 +0200
+++ /tmp/cmake.link.flags	2023-10-27 13:46:14.900102000 +0200
@@ -1,46 +1,16 @@
--fcf-protection=full  # even though HARDENING is turned on this is not used?
 -fPIE
--fstack-clash-protection
--fstack-protector-all
 -ftrapv
 -g3
--L/usr/local/lib
--levent  # these libs are given as e.g. /usr/local/lib/libevent.so, see at the bottom
--levent
--levent_pthreads
--lminiupnpc
--lminiupnpc
--lminiupnpc
--lnatpmp
--lpthread
--lsqlite3
--lzmq
 -O0
 -pie
 -pthread
--std=c++17
--Wall  # all these -W are compilation options and autotools wrongly uses them when linking?
--Wconditional-uninitialized
--Wdate-time
--Wdocumentation
--Werror
--Wextra
--Wformat
--Wformat-security
--Wgnu
--Wimplicit-fallthrough
+-Wl,-rpath,/usr/local/lib:  # the : at the end looks like a typo?
 -Wl,-z,now
 -Wl,-z,relro
 -Wl,-z,separate-code
--Wloop-analysis
--Wno-self-assign
--Wno-unused-parameter
--Woverloaded-virtual
--Wredundant-decls
--Wshadow-field
--Wstack-protector
--Wsuggest-override
--Wthread-safety
--Wunreachable-code-loop-increment
--Wunused-member-function
--Wvla
+/usr/local/lib/libevent_pthreads.so
+/usr/local/lib/libevent.so
+/usr/local/lib/libminiupnpc.so
+/usr/local/lib/libnatpmp.so
+/usr/local/lib/libsqlite3.so
+/usr/local/lib/libzmq.so

hebasto · 2023-10-27T12:26:21Z

@vasild

Thank you for testing!

not sure why ENABLE_ZMQ, USE_NATPMP and USE_UPNP are not in autotools

In Autotools, those definitions are defined in the config/bitcoin-config.h header, for example:

bitcoin/configure.ac

Line 1599 in e4204f4

AC_DEFINE([ENABLE_ZMQ], [1], [Define this symbol to enable ZMQ functions]),

However, CMake uses the "target -- properties" approach and binds definitions to specific targets, for example:

bitcoin/src/zmq/CMakeLists.txt

Lines 12 to 15 in e4204f4

    
           target_compile_definitions(bitcoin_zmq 
        
             INTERFACE 
        
               ENABLE_ZMQ=1 
        
           )

This approach guarantees that a particular property (i.e., a precompiler definition) will be applied iif it is required.

hebasto · 2023-10-27T12:32:05Z

@vasild

--fcf-protection=full # even though HARDENING is turned on this is not used?

What version of FreeBSD do you use?

UPD. As discussed on IRC, CMake is smart enough to avoid providing compiler options to a linker.

hebasto · 2023-10-27T12:59:52Z

+-Wl,-rpath,/usr/local/lib: # the : at the end looks like a typo?

See https://gitlab.kitware.com/cmake/cmake/-/issues/18852.

theuni · 2023-10-27T15:10:27Z

Thanks @vasild for the comparison :)

I'd really like to spend more time reviewing the details here, but I think it's more important to get through everything at a high level once.

Personally at least, I'm happy now with the approach for adding groups of compiler/linker flags. ACK to keep things moving.

vasild · 2023-10-27T15:29:43Z

Maybe I was not clear - my ACK implies that the diffs are ok and I do not see them as a show stopper. Some of the diffs are due to autotools doing nonsensical stuff and other due to stuff not implemented yet in cmake, as expected.

theuni · 2023-10-27T15:34:03Z

Maybe I was not clear - my ACK implies that the diffs are ok and I do not see them as a show stopper. Some of the diffs are due to autotools doing nonsensical stuff and other due to stuff not implemented yet in cmake, as expected.

That was clear to me, thanks :)

hebasto mentioned this pull request Oct 17, 2023

CMake-based build system tracking issue bitcoin/bitcoin#28607

Closed

16 tasks

fanquake reviewed Oct 18, 2023

View reviewed changes

fanquake mentioned this pull request Oct 18, 2023

build: move -fstack-reuse=none to CORE_CXXFLAGS bitcoin/bitcoin#28672

Merged

hebasto force-pushed the 230715-cmake-K branch from f2f4d3f to 8390dc4 Compare October 18, 2023 17:45

fanquake reviewed Oct 18, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

fanquake reviewed Oct 19, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

hebasto force-pushed the 230715-cmake-K branch from 8390dc4 to 2e82d83 Compare October 19, 2023 13:45

vasild reviewed Oct 19, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

fanquake reviewed Oct 19, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

hebasto force-pushed the 230715-cmake-K branch 2 times, most recently from f228f51 to 5219f18 Compare October 19, 2023 15:39

hebasto changed the title ~~cmake: Add hardening, reduce exports, werror, libbitcoinconsensus and install~~ cmake: Add hardening, reduce exports, werror and install Oct 20, 2023

hebasto force-pushed the 230715-cmake-K branch from 5219f18 to 07be478 Compare October 20, 2023 08:48

hebasto force-pushed the 230715-cmake-K branch from 07be478 to c15fad2 Compare October 20, 2023 19:34

theuni reviewed Oct 20, 2023

View reviewed changes

hebasto mentioned this pull request Oct 24, 2023

Rebased cmake-staging branch (post PRs 33, 37) #38

Closed

hebasto force-pushed the cmake-staging branch from 618eec0 to f4162d0 Compare October 26, 2023 10:44

hebasto force-pushed the 230715-cmake-K branch from c15fad2 to 3ae4384 Compare October 26, 2023 10:49

hebasto force-pushed the 230715-cmake-K branch from 3ae4384 to b88ee3b Compare October 26, 2023 13:22

hebasto added 8 commits October 26, 2023 14:53

fixup! cmake: Add TryAppendCXXFlags module

40973d6

fixup! cmake: Add TryAppendLinkerFlag module

e3a378b

fixup! cmake: Add platform-specific linker flags

4375266

fixup! cmake: Redefine configuration flags

ac1e28e

cmake: Add general compile options

574d66d

cmake: Add HARDENING option

643decf

cmake: Add REDUCE_EXPORTS option

1ea5f1c

cmake: Add WERROR option

c9decae

hebasto force-pushed the 230715-cmake-K branch from b88ee3b to e4204f4 Compare October 26, 2023 13:55

theuni reviewed Oct 26, 2023

View reviewed changes

cmake: Implement make install

45f8a3c

hebasto force-pushed the 230715-cmake-K branch from e4204f4 to 45f8a3c Compare October 27, 2023 13:48

hebasto merged commit 6e20a0f into cmake-staging Oct 29, 2023
5 of 6 checks passed

hebasto mentioned this pull request Oct 29, 2023

cmake: Build libbitcoinconsensus library #41

Closed

hebasto mentioned this pull request Aug 5, 2024

cmake: Drop extraneous -fvisibility-inlines-hidden #303

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Add hardening, reduce exports, werror and install #32

cmake: Add hardening, reduce exports, werror and install #32

hebasto commented Oct 17, 2023 •

edited

Loading

hebasto commented Oct 17, 2023 •

edited

Loading

vasild left a comment

hebasto commented Oct 20, 2023

hebasto commented Oct 20, 2023

fanquake commented Oct 20, 2023

hebasto commented Oct 20, 2023

theuni Oct 20, 2023

hebasto Oct 26, 2023

hebasto commented Oct 21, 2023

theuni commented Oct 24, 2023

hebasto commented Oct 24, 2023 •

edited

Loading

hebasto commented Oct 26, 2023

theuni commented Oct 26, 2023

theuni left a comment

theuni Oct 26, 2023

hebasto Oct 26, 2023 •

edited

Loading

theuni Oct 26, 2023

theuni Oct 26, 2023

hebasto Oct 26, 2023

theuni Oct 26, 2023

hebasto Oct 27, 2023

theuni Oct 26, 2023

vasild commented Oct 27, 2023

hebasto commented Oct 27, 2023 •

edited

Loading

hebasto commented Oct 27, 2023 •

edited

Loading

hebasto commented Oct 27, 2023

theuni commented Oct 27, 2023

vasild commented Oct 27, 2023

theuni commented Oct 27, 2023

cmake: Add hardening, reduce exports, werror and install #32

cmake: Add hardening, reduce exports, werror and install #32

Conversation

hebasto commented Oct 17, 2023 • edited Loading

hebasto commented Oct 17, 2023 • edited Loading

vasild left a comment

Choose a reason for hiding this comment

hebasto commented Oct 20, 2023

hebasto commented Oct 20, 2023

fanquake commented Oct 20, 2023

hebasto commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Oct 21, 2023

theuni commented Oct 24, 2023

hebasto commented Oct 24, 2023 • edited Loading

hebasto commented Oct 26, 2023

theuni commented Oct 26, 2023

theuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild commented Oct 27, 2023

hebasto commented Oct 27, 2023 • edited Loading

hebasto commented Oct 27, 2023 • edited Loading

hebasto commented Oct 27, 2023

theuni commented Oct 27, 2023

vasild commented Oct 27, 2023

theuni commented Oct 27, 2023

hebasto commented Oct 17, 2023 •

edited

Loading

hebasto commented Oct 17, 2023 •

edited

Loading

hebasto commented Oct 24, 2023 •

edited

Loading

hebasto Oct 26, 2023 •

edited

Loading

hebasto commented Oct 27, 2023 •

edited

Loading

hebasto commented Oct 27, 2023 •

edited

Loading