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

build: split pthread flags out of ldflags and dont use when building libconsensus #19558

Merged
merged 4 commits into from
Sep 15, 2020

Conversation

fanquake
Copy link
Member

TLDR: Split pthread flags out of ldflags, and stop using them when building libconsensus.

Building libconsensus on Linux using Clang currently warns. i.e:

./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).

This can be demonstrated with a patch to libconsensus:

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:

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

# 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

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 20, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, 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.

configure.ac Outdated Show resolved Hide resolved
@laanwj
Copy link
Member

laanwj commented Jul 22, 2020

Concept ACK. I don't think there are plans to ever make libconsensus use threads (definitely not directly), because this would make it harder to use from some languages. So this seems fine.

@fanquake fanquake force-pushed the split_out_pthread_flags branch from 6e6e4e9 to 9c5bef4 Compare July 24, 2020 06:32
@laanwj
Copy link
Member

laanwj commented Aug 12, 2020

@dongcarl @theuni can you take a look here please

@DrahtBot
Copy link
Contributor

Guix builds

File commit b4d0366
(master)
commit c70581b
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz f44891293fcb6802... b04cf55872990aec...
*-aarch64-linux-gnu.tar.gz e64f1f4378b738f3... 9b9b8e8de21335ac...
*-arm-linux-gnueabihf-debug.tar.gz cfa499656def350f... 08b6b3d701eafb61...
*-arm-linux-gnueabihf.tar.gz 94d27cf2a82082dc... b513f93f26ba691b...
*-riscv64-linux-gnu-debug.tar.gz 3da435713be4835e... f7e52848bd6e6a14...
*-riscv64-linux-gnu.tar.gz 7376f67d61282cdb... 9ac63f5f56bfd0c8...
*-win-unsigned.tar.gz e7abd33ebe905812... e00bd1928b334c2e...
*-win64-debug.zip c1bdd3af288c2b3c... ed68a32b2734c800...
*-win64-setup-unsigned.exe 1f452e7866f1b3c2... 3bd9d9d984389b1d...
*-win64.zip 67f5d4d31bc76406... 23e7fa49afbcac3c...
*-x86_64-linux-gnu-debug.tar.gz e0e91e785ee6d671... a5cc7946b1aa482b...
*-x86_64-linux-gnu.tar.gz 2c9d094b26f44968... 4b27418b80809894...
*.tar.gz 69ec36f4e4bee888... 53d19915ef572de8...
guix_build.log 51e3d570bbcc6dfe... 0ca38fb0d7173f62...
guix_build.log.diff 870d46915b1ae6a6...

@DrahtBot
Copy link
Contributor

Gitian builds

File commit a57af89
(master)
commit d652bc8
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz 8b4a1a1ea553f69f... 0100e3d6e6f3650f...
*-aarch64-linux-gnu.tar.gz 1f93a63d8397aff2... 4d459f3b5b5b4a6b...
*-arm-linux-gnueabihf-debug.tar.gz 5053f76c45b29f42... 9523c18a33bd7307...
*-arm-linux-gnueabihf.tar.gz af4e8080df6b4769... c94d938674bcf0f3...
*-osx-unsigned.dmg 7ee6f85d4853b531... e963cf3148aa550b...
*-osx64.tar.gz 29c33fb36c82c936... 8ec58484be56e4e8...
*-riscv64-linux-gnu-debug.tar.gz 1552e87d2e59612b... b0a10a775d3722a0...
*-riscv64-linux-gnu.tar.gz f206c62e44133795... 3f5828cc420b2295...
*-win64-debug.zip 023e60578db1b77a... 5d578d71637e65b9...
*-win64-setup-unsigned.exe 6c693991817b9e93... fddd11948a0c592e...
*-win64.zip 16b1820ff7a065b1... 7d751e438a9d91fe...
*-x86_64-linux-gnu-debug.tar.gz 74380833b29172a1... 1c50918631a05f96...
*-x86_64-linux-gnu.tar.gz 5af950dccb812a51... 62c5c03c5f28380b...
*.tar.gz 0d444abfdfc84fe3... c0e9f9df02767176...
bitcoin-core-linux-0.21-res.yml ecdf987d89eb5ffe... 19fb5a157a9ee300...
bitcoin-core-osx-0.21-res.yml 4d0d5b398e691077... 9472d1f35d58d190...
bitcoin-core-win-0.21-res.yml 80fe6cf465b08ae0... 08a3c5c6b8a3b224...
linux-build.log 2ee1352c78ea1b7b... 17c92db9472ae683...
osx-build.log 16dd5e35db85be0c... 3729d2cb9b39ac3e...
win-build.log 69ec5c8efa855981... ce8588b63a06945b...
bitcoin-core-linux-0.21-res.yml.diff 2bf76e4cd39d50cb...
bitcoin-core-osx-0.21-res.yml.diff 3792ce96aef742d3...
bitcoin-core-win-0.21-res.yml.diff 6c8babde3859707b...
linux-build.log.diff ef672e9cb9552e16...
osx-build.log.diff 562cbb7690bfe7bf...
win-build.log.diff 141b267628a20d25...

@dongcarl
Copy link
Contributor

Looks like the builds are working.
A quick skim of the code looks correct, however, I willingly admit that I'm not the most familiar with automake idiosyncrasies and conventions.

@hebasto
Copy link
Member

hebasto commented Aug 21, 2020

Concept ACK. Hope this would make #19772 fixing easier.

Copy link
Member

@hebasto hebasto left a 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-

?

@fanquake fanquake force-pushed the split_out_pthread_flags branch 2 times, most recently from abbe1e4 to d42f215 Compare August 24, 2020 13:03
@fanquake
Copy link
Member Author

Mind making 8331ad7 a scripted-diff:

It's now a scripted diff.

Copy link
Member

@hebasto hebasto left a 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)
Copy link
Member

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.

Copy link
Member Author

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.

@theuni
Copy link
Member

theuni commented Sep 4, 2020

@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.

@fanquake fanquake force-pushed the split_out_pthread_flags branch from d42f215 to f005403 Compare September 11, 2020 06:00
@hebasto
Copy link
Member

hebasto commented Sep 12, 2020

It seems in 69a933c the patched lines location should be updated.

Also moves $PTHREAD_CFLAGS to the CFLAGS.
@fanquake fanquake force-pushed the split_out_pthread_flags branch from f005403 to 6c990f3 Compare September 14, 2020 08:28
@fanquake
Copy link
Member Author

It seems in 69a933c the patched lines location should be updated.

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
@fanquake fanquake force-pushed the split_out_pthread_flags branch from 6c990f3 to fc9278d Compare September 14, 2020 08:35
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK fc9278d, only rebased and renamed s/AM_PTHREAD_FLAGS/PTHREAD_FLAGS/ since my previous review..

Copy link
Contributor

@kallewoof kallewoof left a 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.

@laanwj
Copy link
Member

laanwj commented Sep 15, 2020

Looks like a straightforward improvement.
Code review ACK fc9278d

@laanwj laanwj merged commit 48a9968 into bitcoin:master Sep 15, 2020
@fanquake fanquake deleted the split_out_pthread_flags branch September 15, 2020 12:08
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 15, 2020
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants