-
Notifications
You must be signed in to change notification settings - Fork 36.4k
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
build: split pthread flags out of ldflags and dont use when building libconsensus #19558
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Concept ACK. I don't think there are plans to ever make |
6e6e4e9
to
9c5bef4
Compare
Gitian builds
|
Looks like the builds are working. |
Concept ACK. Hope this would make #19772 fixing easier. |
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.
ACK 9c5bef4, tested on Linux Mint 20 (x86_64) -- no clang warnings now. Verified that ax_pthread.m4
has no our custom patches.
Mind making 8331ad7 a scripted-diff:
-BEGIN VERIFY SCRIPT-
sed -i -e 's/\$(RELDFLAGS) \$(AM_LDFLAGS) \$(LIBTOOL_APP_LDFLAGS)$/\$(FUZZ_SUITE_LDFLAGS_COMMON)/' src/Makefile.test.include
patch -p1 << "EOF"
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -318,6 +318,8 @@ endif
if ENABLE_FUZZ
+FUZZ_SUITE_LDFLAGS_COMMON = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
+
test_fuzz_addition_overflow_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
test_fuzz_addition_overflow_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
test_fuzz_addition_overflow_LDADD = $(FUZZ_SUITE_LD_COMMON)
EOF
-END VERIFY SCRIPT-
?
abbe1e4
to
d42f215
Compare
It's now a scripted diff. |
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.
re-ACK d42f215
$ git range-diff master 9c5bef450f7bf4dd4307cae8bce76dc131643574 d42f2150496128fde0a9d2d28707a000a7a0d900
1: ae409c1b4 = 1: 3711cb7ce build: add PTHREAD_LIBS to LDFLAGS configure output
2: 8331ad753 ! 2: b3ee267a6 test: add FUZZ_SUITE_LDFLAGS_COMMON
@@ Metadata
Author: fanquake <fanquake@gmail.com>
## Commit message ##
- test: add FUZZ_SUITE_LDFLAGS_COMMON
+ scripted-diff: add FUZZ_SUITE_LDFLAGS_COMMON
+
+ -BEGIN VERIFY SCRIPT-
+ sed -i -e 's/\$(RELDFLAGS) \$(AM_LDFLAGS) \$(LIBTOOL_APP_LDFLAGS)$/\$(FUZZ_SUITE_LDFLAGS_COMMON)/' src/Makefile.test.include
+ patch -p1 << "EOF"
+ --- a/src/Makefile.test.include
+ +++ b/src/Makefile.test.include
+ @@ -318,6 +318,8 @@ endif
+
+ if ENABLE_FUZZ
+
+ +FUZZ_SUITE_LDFLAGS_COMMON = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
+ +
+ test_fuzz_addition_overflow_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
+ test_fuzz_addition_overflow_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
+ test_fuzz_addition_overflow_LDADD = $(FUZZ_SUITE_LD_COMMON)
+ EOF
+ -END VERIFY SCRIPT-
## src/Makefile.test.include ##
@@ src/Makefile.test.include: endif
3: 06e0771ec = 3: abdd60af7 build: split PTHREAD_* flags out of AM_LDFLAGS
4: 9c5bef450 = 4: d42f21504 build: AX_PTHREAD serial 27
src/Makefile.am
Outdated
AM_CXXFLAGS = $(DEBUG_CXXFLAGS) $(HARDENED_CXXFLAGS) $(WARN_CXXFLAGS) $(NOWARN_CXXFLAGS) $(ERROR_CXXFLAGS) $(GPROF_CXXFLAGS) $(SANITIZER_CXXFLAGS) | ||
AM_CPPFLAGS = $(DEBUG_CPPFLAGS) $(HARDENED_CPPFLAGS) | ||
AM_PTHREAD_FLAGS = $(PTHREAD_CFLAGS) $(PTHREAD_LIBS) |
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.
AM_FOO usually have some meaning to Automake, so this should probably drop the AM_ prefix.
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.
Good call. Switch to PTHREAD_FLAGS
.
@hebasto Good call on the scripted diff. That removes a big hurdle to this change. Pretty sure we're still not really using the pthreads macro as intended, but our usage has held up in the real-world so far, and I don't think this is likely to break anything. Concept ACK. |
d42f215
to
f005403
Compare
It seems in 69a933c the patched lines location should be updated. |
Also moves $PTHREAD_CFLAGS to the CFLAGS.
f005403
to
6c990f3
Compare
Thanks. This should be addressed now. |
-BEGIN VERIFY SCRIPT- sed -i -e 's/\$(RELDFLAGS) \$(AM_LDFLAGS) \$(LIBTOOL_APP_LDFLAGS)$/\$(FUZZ_SUITE_LDFLAGS_COMMON)/' src/Makefile.test.include patch -p1 << "EOF" --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -323,6 +323,8 @@ endif if ENABLE_FUZZ +FUZZ_SUITE_LDFLAGS_COMMON = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) + test_fuzz_addition_overflow_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) test_fuzz_addition_overflow_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) test_fuzz_addition_overflow_LDADD = $(FUZZ_SUITE_LD_COMMON) EOF -END VERIFY SCRIPT-
Note that with this change we are no-longer including PTHREAD_* flags when building libbitcoinconsensus. Also note that we are including PTHREAD_LIBS in AM_PTHREAD_FLAGS
6c990f3
to
fc9278d
Compare
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.
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.
ACK fc9278d
Code review + compiled and ran tests on mac & linux.
Looks like a straightforward improvement. |
…nt use when building libconsensus fc9278d build: AX_PTHREAD serial 27 (fanquake) 15c27c4 build: split PTHREAD_* flags out of AM_LDFLAGS (fanquake) 68e3e22 scripted-diff: add FUZZ_SUITE_LDFLAGS_COMMON (fanquake) afecde8 build: add PTHREAD_LIBS to LDFLAGS configure output (fanquake) Pull request description: TLDR: Split pthread flags out of ldflags, and stop using them when building libconsensus. Building libconsensus on Linux using Clang currently warns. i.e: ```bash ./autogen.sh ./configure --disable-tests --disable-bench --with-utils=no --with-daemon=no --with-gui=no --disable-wallet --with-libs=yes CC=clang CXX=clang++ make V=1 -j6 ... -Wl,-z -Wl,relro -Wl,-z -Wl,now -pthread -Wl,-soname -Wl,libbitcoinconsensus.so.0 -o .libs/libbitcoinconsensus.so.0.0.0 clang: warning: argument unused during compilation: '-pthread' [-Wunused-command-line-argument] clang: warning: argument unused during compilation: '-pthread' [-Wunused-command-line-argument] ``` Besides wanting to quiet the warnings, after digging into this it seemed we could clean up how we are passing around the pthread flags. I also learnt a bit more about how libtools builds shared libraries, and that passing `-pthread` on the link line wouldn't be enough to link against pthreads anyways, due to libtools usage of -nostdlib (see [related discussion where we build DLLs](https://github.com/bitcoin/bitcoin/blob/476436b2dec254bb988f8c7a6cbec1d7bb7cecfd/configure.ac#L603)). This can be demonstrated with a patch to libconsensus: ```patch diff --git a/src/script/bitcoinconsensus.cpp b/src/script/bitcoinconsensus.cpp index 15e204062..10bf3582f 100644 --- a/src/script/bitcoinconsensus.cpp +++ b/src/script/bitcoinconsensus.cpp @@ -10,6 +10,8 @@ #include <script/interpreter.h> #include <version.h> +#include <pthread.h> + namespace { /** A class that deserializes a single CTransaction one time. */ @@ -127,3 +129,10 @@ unsigned int bitcoinconsensus_version() // Just use the API version for now return BITCOINCONSENSUS_API_VER; } + +void *func_pthread(void *x) { return x; } + +void f() { + pthread_t t; + pthread_create(&t,0,func_pthread,0); +} ``` After building, you'll find you have a `libbitcoinconsensus.so` using pthread symbols, but which isn't linked against libpthread: ```bash ldd -r src/.libs/libbitcoinconsensus.so linux-vdso.so.1 (0x00007ffe49378000) libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f553cee7000) libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f553cda2000) libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f553cd88000) libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f553cbc5000) /lib64/ld-linux-x86-64.so.2 (0x00007f553d15d000) undefined symbol: pthread_create (src/.libs/libbitcoinconsensus.so) ``` This libtool behaviour has been known about for some time, i.e this [thread from 2005](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25460), describes the same issue. The suggestion from libtool maintainers at the time is to add `-lpthread` to LDFLAGS. Also worth noting is that some of the users in those threads were also using the `AX_PTHREADS` macro, same as us, to determine how to compile with/link against pthreads. This macro has [recently been updated](https://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=commitdiff;h=2fb904589643eb6ca6122f834891b58d1d51b347), with reference to this issue. You can compare the output from the version we currently use, to the new version: ```bash # our ax_pthread macro: PTHREAD_CFLAGS = -pthread PTHREAD_LIBS = PTHREAD_CC = gcc / clang # the new ax_pthread macro PTHREAD_CFLAGS = -pthread PTHREAD_LIBS = -lpthread PTHREAD_CC = gcc / clang ``` Note that as part of this PR I've also added `PTHREAD_LIBS` to the split out flags. Although we weren't using it anywhere previously (and wouldn't have seemed to matter for the most part, given it was likely empty for most builders), the macro assumes it's use. i.e: > NOTE: You are assumed to not only compile your program with these flags, > but also to link with them as well. For example, you might link with > $PTHREAD_CC $CFLAGS $PTHREAD_CFLAGS $LDFLAGS ... $PTHREAD_LIBS $LIBS ACKs for top commit: laanwj: Code review ACK fc9278d hebasto: re-ACK fc9278d, only rebased and renamed s/`AM_PTHREAD_FLAGS`/`PTHREAD_FLAGS`/ since my [previous](bitcoin#19558 (review)) review.. kallewoof: ACK fc9278d Tree-SHA512: 7c0a5b0f0de4f54b1d7dce0e69020b341c37a383bb7c715867cc96c648774a557b1ddb42eb1b676f7bb2b822b69795bec14dc6272362d80662a21f10cb80331c
TLDR: Split pthread flags out of ldflags, and stop using them when building libconsensus.
Building libconsensus on Linux using Clang currently warns. i.e:
Besides wanting to quiet the warnings, after digging into this it seemed we could clean up how we are passing around the pthread flags. I also learnt a bit more about how libtools builds shared libraries, and that passing
-pthread
on the link line wouldn't be enough to link against pthreads anyways, due to libtools usage of -nostdlib (see related discussion where we build DLLs).This can be demonstrated with a patch to libconsensus:
After building, you'll find you have a
libbitcoinconsensus.so
using pthread symbols, but which isn't linked against libpthread:This libtool behaviour has been known about for some time, i.e this thread from 2005, describes the same issue. The suggestion from libtool maintainers at the time is to add
-lpthread
to LDFLAGS.Also worth noting is that some of the users in those threads were also using the
AX_PTHREADS
macro, same as us, to determine how to compile with/link against pthreads. This macro has recently been updated, with reference to this issue. You can compare the output from the version we currently use, to the new version:Note that as part of this PR I've also added
PTHREAD_LIBS
to the split out flags. Although we weren't using it anywhere previously (and wouldn't have seemed to matter for the most part, given it was likely empty for most builders), the macro assumes it's use. i.e: