Skip to content

Build: Add build support for profiling. #12373

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
Merged
merged 1 commit into from
Mar 6, 2018
Merged

Build: Add build support for profiling. #12373

merged 1 commit into from
Mar 6, 2018

Conversation

murrayn
Copy link
Contributor

@murrayn murrayn commented Feb 7, 2018

Support for profiling build: ./configure --enable-profiling

@laanwj laanwj requested a review from theuni February 15, 2018 16:12
@laanwj
Copy link
Member

laanwj commented Feb 15, 2018

Should this mention in the help message that this is specifically for profiling with gprof? E.g. Valgrind is also used quite frequently for profiling.

Added @theuni as reviewer because of build system change.

@murrayn
Copy link
Contributor Author

murrayn commented Feb 15, 2018 via email

@maflcko maflcko added the Tests label Feb 16, 2018
jonasschnelli
jonasschnelli reviewed Feb 17, 2018
View reviewed changes
configure.ac Outdated
@@ -1330,6 +1348,7 @@ echo " with bench = $use_bench"
echo " with upnp = $use_upnp"
echo " use asm = $use_asm"
echo " debug enabled = $enable_debug"
echo " profiling enabled = $enable_profiling"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alignment

theuni
theuni reviewed Feb 20, 2018
View reviewed changes
configure.ac Outdated
@@ -224,6 +231,17 @@ AC_ARG_ENABLE([werror],
AC_LANG_PUSH([C++])
AX_CHECK_COMPILE_FLAG([-Werror],[CXXFLAG_WERROR="-Werror"],[CXXFLAG_WERROR=""])

if test "x$enable_gprof" = xyes; then
LDFLAGS="$LDFLAGS -pg"
Copy link
Member

Choose a reason for hiding this comment

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

Please use AX_CHECK_LINK_FLAG here to make sure that it actually works. Also, let's stash -pg in PROFILE_LDFLAGS rather than LDFLAGS. Otherwise, the rest of the configure checks will be linked with -pg. So something like:

Something like:

AX_CHECK_LINK_FLAG([[-pg]],[PROFILE_LDFLAGS="$PROFILE_LDFLAGS -pg"],[AC_MSG_ERROR("profiling requested but not available"])

Then the same for CXXFLAGS.

@bitcoin bitcoin deleted a comment from dmoney0546 Feb 20, 2018
@jamesob
Copy link
Contributor

jamesob commented Feb 20, 2018 •
edited
Loading

Unsure if this is working with clang; getting some warnings:

 $ ./configure LDFLAGS="-L${BDB_PREFIX}/lib/" CPPFLAGS="-I${BDB_PREFIX}/include/" CXXFLAGS="${CXXFLAGS}" --enable-wallet --enable-gprof --with-daemon
[...]
Options used to compile and link:
  with wallet   = yes
  with gui / qt = yes
    qt version  = 5
    with qr     = auto
  with zmq      = yes
  with test     = yes
  with bench    = yes
  with upnp     = auto
  use asm       = yes
  debug enabled = no
  gprof enabled = yes
  werror        = no

  target os     = linux
  build os      =

  CC            = /usr/bin/clang-4.0
  CFLAGS        = -g -O2 -pg
  CPPFLAGS      = -I/home/james/tmp/bitcoin/db4/include/ -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
  CXX           = /usr/bin/clang++-4.0 -std=c++11
  CXXFLAGS      = -std=c++11  -pg
  LDFLAGS       = -L/home/james/tmp/bitcoin/db4/lib/ -pg
  ARFLAGS       = cr

$ make -j 3
[...]
  CXX      bench/bench_bench_bitcoin-base58.o
  CXX      bench/bench_bench_bitcoin-coin_selection.o
  CXX      qt/qt_bitcoin_qt-bitcoin.o
  CXX      qt/qt_libbitcoinqt_a-bantablemodel.o
  CXX      qt/qt_libbitcoinqt_a-bitcoinaddressvalidator.o
  CXXLD    libbitcoinconsensus.la
clang: warning: argument unused during compilation: '-pg' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-pthread' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-pg' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-pthread' [-Wunused-command-line-argument]
/usr/bin/ar: `u' modifier ignored since `D' is the default (see `U')
[...]

Edit: my mistake, clang doesn't accept -pg and (apparently?) doesn't work with gprof. Worth adding an explicit warning about this for clang users?

@theuni
Copy link
Member

theuni commented Feb 20, 2018

@jamesob The suggestion above should take care of it.

@murrayn
Copy link
Contributor Author

murrayn commented Feb 22, 2018

@theuni If this latest change (or any related assignment to LDFLAGS as was done with CXXFLAGS) occurs later in configure.ac, gprof profiling fails. Perhaps there is some order of arguments issue with ld.

@theuni
Copy link
Member

theuni commented Feb 22, 2018

@murrayn Please try this on top of yours: theuni@bb91da4. Works for me.

@murrayn
Copy link
Contributor Author

murrayn commented Feb 23, 2018

@theuni This doesn't work for me. It compiles, and the binaries run, but if you look at the gprof output, it is empty. My test case:

$ make
$ cd src/qt
$ ./bitcoin-qt --help
$ gprof bitcoin-qt

@theuni
Copy link
Member

theuni commented Feb 23, 2018

@murrayn turns out the problem is that -pg is incompatible with -pie. Could you please give theuni@38450db a try on top of yours? I believe that now accounts for everything.

@murrayn
Copy link
Contributor Author

murrayn commented Feb 24, 2018

Works for me now.

@theuni
Copy link
Member

theuni commented Feb 26, 2018

@murrayn Could you please squash all of our changes into your first commit?

@theuni
Copy link
Member

theuni commented Mar 5, 2018

Thanks for sticking with this! utACK cfaac2a

@laanwj laanwj merged commit cfaac2a into bitcoin:master Mar 6, 2018
laanwj added a commit that referenced this pull request Mar 6, 2018
@laanwj
Merge #12373: Build: Add build support for profiling.
f13d756 
cfaac2a Add build support for 'gprof' profiling. (murrayn)

Pull request description:

  Support for profiling build: `./configure --enable-profiling`

Tree-SHA512: ea983cfce385f1893bb4ab7f94ac141b7d620951dc430da3bbc92ae1357fb05521eac689216e66dc87040171a8a57e76dd7ad98036e12a2896cfe5ab544347f0
@murrayn murrayn deleted the profiling branch March 7, 2018 01:14
@murrayn murrayn restored the profiling branch March 7, 2018 05:11
@str4d str4d mentioned this pull request Nov 12, 2019
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Jan 30, 2020
@zkbot
Auto merge of #4222 - str4d:configure-updates, r=str4d
8e8a935 
./configure updates

Includes code cherry-picked from the following upstream Bitcoin Core PRs:

- bitcoin/bitcoin#6748
- bitcoin/bitcoin#12373
- bitcoin/bitcoin#12692
- bitcoin/bitcoin#12901
- bitcoin/bitcoin#13005
- bitcoin/bitcoin#13445
- bitcoin/bitcoin#12686
- bitcoin/bitcoin#16435

Part of #2074.
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 10, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12373: Build: Add build support for profiling.
94c47e5 
cfaac2a Add build support for 'gprof' profiling. (murrayn)

Pull request description:

  Support for profiling build: `./configure --enable-profiling`

Tree-SHA512: ea983cfce385f1893bb4ab7f94ac141b7d620951dc430da3bbc92ae1357fb05521eac689216e66dc87040171a8a57e76dd7ad98036e12a2896cfe5ab544347f0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12373: Build: Add build support for profiling.
bc3751b 
cfaac2a Add build support for 'gprof' profiling. (murrayn)

Pull request description:

  Support for profiling build: `./configure --enable-profiling`

Tree-SHA512: ea983cfce385f1893bb4ab7f94ac141b7d620951dc430da3bbc92ae1357fb05521eac689216e66dc87040171a8a57e76dd7ad98036e12a2896cfe5ab544347f0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 13, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12373: Build: Add build support for profiling.
fa50d7b 
cfaac2a Add build support for 'gprof' profiling. (murrayn)

Pull request description:

  Support for profiling build: `./configure --enable-profiling`

Tree-SHA512: ea983cfce385f1893bb4ab7f94ac141b7d620951dc430da3bbc92ae1357fb05521eac689216e66dc87040171a8a57e76dd7ad98036e12a2896cfe5ab544347f0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 14, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12373: Build: Add build support for profiling.
0849ffc 
cfaac2a Add build support for 'gprof' profiling. (murrayn)

Pull request description:

  Support for profiling build: `./configure --enable-profiling`

Tree-SHA512: ea983cfce385f1893bb4ab7f94ac141b7d620951dc430da3bbc92ae1357fb05521eac689216e66dc87040171a8a57e76dd7ad98036e12a2896cfe5ab544347f0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 14, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12373: Build: Add build support for profiling.
78ef3fd 
cfaac2a Add build support for 'gprof' profiling. (murrayn)

Pull request description:

  Support for profiling build: `./configure --enable-profiling`

Tree-SHA512: ea983cfce385f1893bb4ab7f94ac141b7d620951dc430da3bbc92ae1357fb05521eac689216e66dc87040171a8a57e76dd7ad98036e12a2896cfe5ab544347f0
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 26, 2021
@laanwj
Merge bitcoin#12373: Build: Add build support for profiling.
1f7a3a7 
cfaac2a Add build support for 'gprof' profiling. (murrayn)

Pull request description:

  Support for profiling build: `./configure --enable-profiling`

Tree-SHA512: ea983cfce385f1893bb4ab7f94ac141b7d620951dc430da3bbc92ae1357fb05521eac689216e66dc87040171a8a57e76dd7ad98036e12a2896cfe5ab544347f0
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@jonasschnelli jonasschnelli jonasschnelli left review comments

@theuni theuni theuni left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@murrayn @laanwj @jamesob @theuni @jonasschnelli @fanquake @maflcko