build: Remove --enable-gprof

maflcko commented at 8:50 pm on June 9, 2024: member

It is unclear what benefit this option has, given that:

gprof requires re-compilation (perf and other tools can instead be used on existing executables)
gprof requires hardening to be disabled
gprof doesn’t work with clang
perf is documented in the dev-notes, and test notes, and embedded into the functional test framework; gprof isn’t
Anyone really wanting to use it could pass the required flags to ./configure
I couldn’t find any mention of the use of gprof in the discussions in this repo, apart from the initial pull request adding it (cfaac2a60f3ac63ae8deccb03d88bd559449b78c)
Keeping it means that it needs to be maintained and ported to CMake

Fix all issues by removing it.

This reverts cfaac2a60f3ac63ae8deccb03d88bd559449b78c

fa780e1c25

DrahtBot commented at 8:50 pm on June 9, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, hebasto, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Build system on Jun 9, 2024

maflcko added the label DrahtBot Guix build requested on Jun 9, 2024

TheCharlatan approved

TheCharlatan commented at 9:39 pm on June 9, 2024: contributor

ACK fa780e1c25e8e98253e32d93db65f78a0092433f

DrahtBot commented at 3:00 am on June 10, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit a44b0f771f2130b89b608f39055a355693c91a8c(master)	commit c38a5b6550d1d91a97f721637d55f975e1d5b7ce(master and this pull)
SHA256SUMS.part	`29ec0df7b72af61c...`	`b3dfb11a916f483a...`
*-aarch64-linux-gnu-debug.tar.gz	`613f148809a14528...`	`1e8c0cad38e0705d...`
*-aarch64-linux-gnu.tar.gz	`ce90b1bc39406af4...`	`0f842e2eae81b6b1...`
*-arm-linux-gnueabihf-debug.tar.gz	`c0d6ed69cd8e0962...`	`84d4e1a9570c6414...`
*-arm-linux-gnueabihf.tar.gz	`2d81424a57a3c457...`	`e1e333519913a51e...`
*-arm64-apple-darwin-unsigned.tar.gz	`e0b0f0f596409f9e...`	`fee48f23f3c1e623...`
*-arm64-apple-darwin-unsigned.zip	`43400520f6c26c22...`	`39271dc0a86d051a...`
*-arm64-apple-darwin.tar.gz	`462991bf8f00988b...`	`6879d1ca3ed1e686...`
*-powerpc64-linux-gnu-debug.tar.gz	`66f40010d05091de...`	`cc8b7a3541c3edca...`
*-powerpc64-linux-gnu.tar.gz	`2388f61b843b6ebd...`	`078a05139c3888c9...`
*-riscv64-linux-gnu-debug.tar.gz	`05b6fc6a6472fc10...`	`0cfbf0616e778407...`
*-riscv64-linux-gnu.tar.gz	`7cd0a8901378ece8...`	`1a788e281e28c58c...`
*-x86_64-apple-darwin-unsigned.tar.gz	`25d7915284e29114...`	`d03d1f23cc76810f...`
*-x86_64-apple-darwin-unsigned.zip	`4f06c1c88da0565b...`	`b17129a7db7e1c2e...`
*-x86_64-apple-darwin.tar.gz	`ea6093c1cbfa8d7e...`	`c01857a38e512020...`
*-x86_64-linux-gnu-debug.tar.gz	`f8c04b7636dd9e26...`	`9d1521e5d3d675a4...`
*-x86_64-linux-gnu.tar.gz	`a850d92278e3a9e0...`	`896b55b8a904a2e0...`
*.tar.gz	`d4b7da6b59937b55...`	`a6e49e06b3d1fb93...`
guix_build.log	`574639cac76c0144...`	`37dd9c54ba16c51b...`
guix_build.log.diff		`6eca5b5a222d1f71...`

DrahtBot removed the label DrahtBot Guix build requested on Jun 10, 2024

maflcko added the label Needs CMake port on Jun 10, 2024

hebasto approved

hebasto commented at 8:11 am on June 10, 2024: member

ACK fa780e1c25e8e98253e32d93db65f78a0092433f, I have reviewed the code and it looks OK.

willcl-ark approved

willcl-ark commented at 9:29 am on June 10, 2024: member

crACK fa780e1c25e8e98253e32d93db65f78a0092433f

Agree on the rationale given here for removal.

I’ve previously tried to use gprof in place of perf (when addr2line was broken/unusably slow on my Ubuntu kernel) and hit against many of the pain points (“requirements”) @maflcko mentions in OP, which also had me wondering how often this is really used…

I did find a reference to gprof remaining in depends: https://github.com/bitcoin/bitcoin/blob/7fd4905c403ccb233f489e2f6a6aa3cd53b033ed/depends/packages/qrencode.mk#L9

I think this is also unneeded, as libqrencode seems to default to off in the configure script:

0  --enable-gprof          generate extra code to write profile information
1                          suitable for gprof [default=no]

maflcko commented at 9:38 am on June 10, 2024: member

I did find a reference to gprof remaining in depends:

I think this is unrelated, because it relates to compiling the qrencode package, not Bitcoin Core itself. Happy to review a separate pull, if there is one.

I think this is also unneeded, as libqrencode seems to default to off in the configure script:

depends uses cmake to compile the qrencode package, but it seems off there as well https://github.com/fukuchi/libqrencode/blame/715e29fd4cd71b6e452ae0f4e36d917b43122ce8/CMakeLists.txt#L8 . Again, happy to review a separate pull, if there is one.

fanquake commented at 9:40 am on June 10, 2024: member

Nothing should be required here in regards to qrencode. I agree that this can just be removed.

maflcko added this to the milestone 28.0 on Jun 10, 2024

fanquake merged this on Jun 10, 2024

fanquake closed this on Jun 10, 2024

fanquake removed the label Needs CMake port on Jun 10, 2024

fanquake commented at 11:03 am on June 10, 2024: member

Removed the label because nothing was ever ported.

maflcko commented at 11:29 am on June 10, 2024: member

It needs to be ported to the table in https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e

maflcko deleted the branch on Jun 10, 2024

hebasto commented at 11:47 am on June 10, 2024: member

It needs to be ported to the table in https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e

Done.

build: Remove –enable-gprof #30257

Code Coverage

Reviews

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]