build: make HAVE_O_CLOEXEC available outside LevelDB (bugfix)

theStack commented at 2:48 am on February 21, 2021: member

The result of the O_CLOEXEC availability check is currently only set in the Makefile and passed to LevelDB (see LEVELDB_CPPFLAGS_INT in src/Makefile.leveldb.include), but not defined to be used in our codebase. This means that code within the preprocessor conditional #if HAVE_O_CLOEXEC was actually never compiled. On the master branch this is currently used for pipe creation in src/shutdown.cpp, PR #21007 moves this part to a new module (I found the issue while testing that PR).

The fix is similar to the one in #19803, which solved the same problem for HAVE_FDATASYNC.

In the course of working on the PR it turned out that pipe2 is not available an all platforms, hence a configure check and a corresponding define HAVE_PIPE2 is introduced and used.

The PR can be tested by anyone with a system that has pipe2 and O_CLOEXEC available by putting gibberish into the HAVE_O_CLOEXEC block: on master, everything should compile fine, on PR, the compiler should abort with an error. At least that’s my naive way of testing preprocessor logic, happy to hear more sophisticated ways :-)

fanquake added the label Build system on Feb 21, 2021

fanquake commented at 7:15 am on February 21, 2021: member

Both of the macOS builds are failing with:

0  CXX      libbitcoin_server_a-shutdown.o
1shutdown.cpp:38:9: error: use of undeclared identifier 'pipe2'
2    if (pipe2(g_shutdown_pipe, O_CLOEXEC) != 0) {
3        ^
41 error generated.
5make[2]: *** [Makefile:8763: libbitcoin_server_a-shutdown.o] Error 1

Looks like it has O_CLOEXEC but not pipe2.

theStack force-pushed on Feb 21, 2021

theStack closed this on Feb 21, 2021

theStack reopened this on Feb 21, 2021

theStack commented at 2:19 pm on February 21, 2021: member

Added a commit to change the preprocessor condition to only use popen2 if O_CLOEXEC is available and MacOS is not used. Also, re-opened the PR to start Cirrus again (the failure was unrelated to this PR, see #21216).

DrahtBot commented at 5:51 pm on February 21, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21007 (bitcoind: Add -daemonwait option to wait for initialization by laanwj)

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.

MarcoFalke added the label Needs Guix build on Feb 21, 2021

MarcoFalke added the label Needs gitian build on Feb 21, 2021

luke-jr changes_requested

luke-jr commented at 7:11 pm on February 21, 2021: member

At the very least, have configure check specifically for pipe2…

Ideally, a code path that sets O_CLOEXEC after the normal pipe path could be nice too.

theStack force-pushed on Feb 21, 2021

in configure.ac:1161 in 4b38e4f1fc outdated

1156+ [[ static int pipefd[2];
1157+    pipe2(pipefd, 0); ]])],
1158+ [ AC_MSG_RESULT(yes); HAVE_PIPE2=1 ],
1159+ [ AC_MSG_RESULT(no); HAVE_PIPE2=0 ]
1160+)
1161+AC_DEFINE_UNQUOTED([HAVE_PIPE2], [$HAVE_PIPE2], [Define to 1 if pipe2 is available.])

luke-jr commented at 0:14 am on February 22, 2021:

0AC_CHECK_DECLS([pipe2])

theStack commented at 1:28 pm on February 22, 2021:

Heh, nice that sometimes the solution can be so simple, thanks. Any reason why the same approach is not used for also e.g. fdatasync? (That’s what I used as template in the previous version).

laanwj commented at 1:07 pm on February 22, 2021: member

Concept ACK

This means that code within the preprocessor conditional #if HAVE_O_CLOEXEC was actually never compiled

I’m confused—how does that compile at all if the preprocessor flag is mot set? Our reason for using #if X insteadof #ifdef X is to prevent issues like this. But apparently that doesn’t work?

Edit: OK, that works differently than I thought apparently. I was under the impression that using #if would detect issues such as forgetting to include bitcoin-config.h.

init: only use pipe2 if availabile, check in configure 584fd91d2d

build: make HAVE_O_CLOEXEC available outside LevelDB (bugfix) 9bac71350d

theStack force-pushed on Feb 22, 2021

theStack commented at 1:34 pm on February 22, 2021: member

@laanwj: Yes, I would have also expected #if to fail if a symbol wasn’t declared at all, but apparently that’s not the case :/

Force-pushed now with a much simpler pipe2 detection in configure, as suggested by luke-jr.

luke-jr approved

luke-jr commented at 3:41 pm on February 22, 2021: member

utACK

DrahtBot removed the label Needs Guix build on Feb 23, 2021

MarcoFalke deleted a comment on Feb 23, 2021

MarcoFalke added the label Needs Guix build on Feb 23, 2021

laanwj commented at 2:49 pm on February 23, 2021: member

Code review ACK 9bac71350d98580cc7441957fc7c3fa2f4158553

@laanwj: Yes, I would have also expected #if to fail if a symbol wasn’t declared at all, but apparently that’s not the case :/

I feel kind of ashamed for not realizing this for so long. I wonder if we can come up with some new convention that does ensure this! (anything I can think of inside #if would become kind of long…) and document that in the developer notes.

Not in this PR, of course.

laanwj merged this on Feb 23, 2021

laanwj closed this on Feb 23, 2021

sidhujag referenced this in commit 0939a207f6 on Feb 24, 2021

MarcoFalke removed the label Needs Guix build on Feb 24, 2021

MarcoFalke removed the label Needs gitian build on Feb 24, 2021

DrahtBot commented at 6:59 pm on February 25, 2021: member

Guix builds

File	commit c263c3d7d2a4b85fe133c5d8018c6ec53b8c942a(master)	commit da2bfcaed32837e085a4a9c708b9721bf2b479ee(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`f70493c5704d6edd...`	`6c54ef89ac5d656b...`
*-aarch64-linux-gnu.tar.gz	`2e2078290084bf4a...`	`afa7b26a5b30c406...`
*-arm-linux-gnueabihf-debug.tar.gz	`aa58ae15ed23e2dd...`	`4c37979b342edcec...`
*-arm-linux-gnueabihf.tar.gz	`d84cb6002bdf9630...`	`21ac56d1bf448d22...`
*-osx-unsigned.dmg	`15a9d0887a08363a...`	`277fe412c72e8f14...`
*-osx-unsigned.tar.gz	`11407e6335944da9...`	`08ec891d426824dd...`
*-osx64.tar.gz	`e50ac7e302d3e146...`	`4846f639b12933ad...`
*-powerpc64-linux-gnu-debug.tar.gz	`f354ceeb911fc12a...`	`c69f99f7c1dbb856...`
*-powerpc64-linux-gnu.tar.gz	`c51b5dcac01b08a5...`	`1d03b6ff50e0d91f...`
*-powerpc64le-linux-gnu-debug.tar.gz	`f481a004f8be01a5...`	`080a7aeded2ea1d0...`
*-powerpc64le-linux-gnu.tar.gz	`bfa3d432d2fd1c3a...`	`5c54279ccf06d761...`
*-riscv64-linux-gnu-debug.tar.gz	`b68485350917cb71...`	`1aaaac067b96a6e3...`
*-riscv64-linux-gnu.tar.gz	`654fa174d372c41c...`	`4013df079399365d...`
*-win-unsigned.tar.gz	`462e0fb4e0b6fce0...`	`84006b2f7c1e6155...`
*-win64-debug.zip	`d034f61b738f2a7d...`	`98ca575d8a9776ee...`
*-win64-setup-unsigned.exe	`1723f242fc4ba7e2...`	`20412a68344b7f11...`
*-win64.zip	`20b8721864cc2800...`	`c9d8b754a7fd3c01...`
*-x86_64-linux-gnu-debug.tar.gz	`b0a08fe448036636...`	`05825a00d4179904...`
*-x86_64-linux-gnu.tar.gz	`ad323f3642d34a88...`	`3704240b2a5b32d1...`
*.tar.gz	`db23011c10e3d6b3...`	`ce65b51562711ee4...`
guix_build.log	`c034b83840e96f19...`	`0c432f47fff82276...`
guix_build.log.diff		`1e35dadb39a3b213...`

DrahtBot locked this on Aug 16, 2022

build: make HAVE_O_CLOEXEC available outside LevelDB (bugfix) #21250

Conflicts

Guix builds