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: Avoid fcntl64@GLIBC_2.28 symbol when --enable-glibc-back-compat #22287

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jun 20, 2021

This PR is a partial fix for #21454 (see #22281 for the complete fix for the bitcoind compatibility).

Gitian builds:

Generating report
5d2b1cf6c0a59796e0e087877faff290a252f79bae1a61aeceeda15e5c316186  bitcoin-530bc278b9fa-aarch64-linux-gnu-debug.tar.gz
a67a2a8a160d289fabe786e00715eeccf800e418a305d35a774721a74f738a4a  bitcoin-530bc278b9fa-aarch64-linux-gnu.tar.gz
315f787b8f77b55a37bf56201c747f77c5641c6eab1779049fdc65ef6e3aa162  bitcoin-530bc278b9fa-arm-linux-gnueabihf-debug.tar.gz
58d1b886f10b9a6011645bb65795ed0d418433c44b83fe8359ed4b31f501c167  bitcoin-530bc278b9fa-arm-linux-gnueabihf.tar.gz
31eedc8531fca0b4bd76c5b0cbc8799f6621a758e5fe9abda3aef3f264832c89  bitcoin-530bc278b9fa-powerpc64-linux-gnu-debug.tar.gz
79b8ca4bee6fc24a5c3216fed95f341510f15cdaaf04e2b6d4db991a3d077450  bitcoin-530bc278b9fa-powerpc64-linux-gnu.tar.gz
0fcd01009aa80b9c4b567a241d0d56b579756d9f70a1a78e0fa8c015b99d9bfa  bitcoin-530bc278b9fa-powerpc64le-linux-gnu-debug.tar.gz
af63f25b3de98cf2f25b6592dec9c3b4f95021fcc11396fb345c592979b9e4b7  bitcoin-530bc278b9fa-powerpc64le-linux-gnu.tar.gz
539d6317a7c2cfe6cbecb07eb82ef9b039010ef544cc9a74984a7cb23c03b58a  bitcoin-530bc278b9fa-riscv64-linux-gnu-debug.tar.gz
75ce0a0eabb4555589e632676b2e44df8365f320dea28d9edcd54ec78b4038d1  bitcoin-530bc278b9fa-riscv64-linux-gnu.tar.gz
9dc543d3b0c9ea39e153616132b0c41205a639d0129b752e7151712b8eda54b7  bitcoin-530bc278b9fa-x86_64-linux-gnu-debug.tar.gz
ba5452615ebd1c1398916837ee1a36233721819c08f48ea12ac7c03ac67526c7  bitcoin-530bc278b9fa-x86_64-linux-gnu.tar.gz
2b618a7b6e99bffc15522ca9f4bc0bfb1187a9bb0b005cc32d23127289644cc1  src/bitcoin-530bc278b9fa.tar.gz
9db429d8b661e2d7a48be8b8f9bcbb3df4e8a6b8cce0bd0660d54c1422c0d696  bitcoin-core-linux-22-res.yml
Done.

I tested only:

  • bitcoin-530bc278b9fa-arm-linux-gnueabihf.tar.gz
  • bitcoin-530bc278b9fa-x86_64-linux-gnu.tar.gz

@sipa
Copy link
Member

sipa commented Jun 20, 2021

Perhaps you want to make it print the value of cmd, so we can figure out which F_ constants are used but not handled by the wrapper. The partial approach is pretty uncomfortable though.

case F_SET_FILE_RW_HINT: goto takes_uint64_t_ptr;

default:
fprintf(stderr, "fcntl64 workaround got unknown F_XXX constant");
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove the exit here. This is sort of an assert; if you continue it'll invoke fcntl with who knows what behavior.

@hebasto hebasto force-pushed the 210619-fcntl branch 3 times, most recently from ef22154 to f0ca8bc Compare June 20, 2021 04:24
@hebasto hebasto marked this pull request as ready for review June 20, 2021 05:02
@hebasto
Copy link
Member Author

hebasto commented Jun 20, 2021

Updated 639bfc4 -> f0ca8bc (pr22287.01 -> pr22287.03, diff):

  • addressed @sipa's comments
  • fixed CI builds

@sipa

The partial approach is pretty uncomfortable though.

Here are some possible alternatives:

@sipa
Copy link
Member

sipa commented Jun 20, 2021

How did you fix the failures?

@hebasto
Copy link
Member Author

hebasto commented Jun 20, 2021

How did you fix the failures?

  • ARM hosts are excluded from this solution
  • #if !defined(__arm__) && defined(__GLIBC__) && (__GLIBC__ == 2) && (__GLIBC_MINOR__ >= 28)

@hebasto
Copy link
Member Author

hebasto commented Jun 20, 2021

Closing. Binaries are broken (

@hebasto hebasto closed this Jun 20, 2021
@sipa
Copy link
Member

sipa commented Jun 20, 2021

How are they broken?

@hebasto
Copy link
Member Author

hebasto commented Jun 20, 2021

How are they broken?

On x86_64 machine:

$ bin/bitcoind -signet
2021-06-20T05:45:29Z Bitcoin Core version v21.99.0-f0ca8bc08c0d (release build)
2021-06-20T05:45:29Z Signet derived magic (message start): 0a03cf40
2021-06-20T05:45:29Z Assuming ancestors of block 0000002a1de0f46379358c1fd09906f7ac59adf3712323ed90eb59e4c183c020 have valid signatures.
2021-06-20T05:45:29Z Setting nMinimumChainWork=00000000000000000000000000000000000000000000000000000019fd16269a
2021-06-20T05:45:29Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
2021-06-20T05:45:29Z Using RdSeed as additional entropy source
2021-06-20T05:45:29Z Using RdRand as an additional entropy source
2021-06-20T05:45:29Z Default data directory /home/hebasto/.bitcoin
2021-06-20T05:45:29Z Using data directory /home/hebasto/.bitcoin/signet
2021-06-20T05:45:29Z Config file: /home/hebasto/.bitcoin/bitcoin.conf
2021-06-20T05:45:29Z Config file arg: dbcache="8192"
2021-06-20T05:45:29Z Config file arg: logips="1"
2021-06-20T05:45:29Z Config file arg: logthreadnames="1"
2021-06-20T05:45:29Z Config file arg: maxmempool="500"
2021-06-20T05:45:29Z Config file arg: server="1"
2021-06-20T05:45:29Z Config file arg: [main] addnode="ejxefzf5fpst4mg2rib7grksvscl7p6fvjp6agzgfc2yglxnjtxc3aid.onion"
2021-06-20T05:45:29Z Config file arg: [main] addnode="rp7k2go3s5lyj3fnj6zn62ktarlrsft2ohlsxkyd7v3e3idqyptvread.onion"
2021-06-20T05:45:29Z Config file arg: [main] addnode="mwmfluek4au6mxxpw6fy7sjhkm65bdfc7izc7lpz3trewfdghyrzsbid.onion"
2021-06-20T05:45:29Z Config file arg: [main] blocksdir="/home/hebasto/Blockchain"
2021-06-20T05:45:29Z Config file arg: [main] dnsseed="0"
2021-06-20T05:45:29Z Config file arg: [main] listenonion="1"
2021-06-20T05:45:29Z Config file arg: [main] onion="127.0.0.1:9050"
2021-06-20T05:45:29Z Config file arg: [main] uacomment="h42"
2021-06-20T05:45:29Z Config file arg: [main] wallet=false
2021-06-20T05:45:29Z Config file arg: [regtest] blocksdir="/home/hebasto/VMs/bitcoin"
2021-06-20T05:45:29Z Config file arg: [signet] blocksdir="/home/hebasto/VMs/bitcoin"
2021-06-20T05:45:29Z Config file arg: [test] blocksdir="/home/hebasto/VMs/bitcoin"
2021-06-20T05:45:29Z Config file arg: [test] listen="1"
2021-06-20T05:45:29Z Config file arg: [test] onlynet="ipv4"
2021-06-20T05:45:29Z Config file arg: [test] server="1"
2021-06-20T05:45:29Z Setting file arg: wallet = ["210528d","210528-bdb"]
2021-06-20T05:45:29Z Command-line arg: signet=""
2021-06-20T05:45:29Z Using at most 125 automatic connections (1024 file descriptors available)
2021-06-20T05:45:29Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2021-06-20T05:45:29Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2021-06-20T05:45:29Z Script verification uses 7 additional threads
2021-06-20T05:45:29Z scheduler thread start
2021-06-20T05:45:29Z HTTP: creating work queue of depth 16
2021-06-20T05:45:29Z Using random cookie authentication.
2021-06-20T05:45:29Z Generated RPC authentication cookie /home/hebasto/.bitcoin/signet/.cookie
2021-06-20T05:45:29Z HTTP: starting 4 worker threads
2021-06-20T05:45:29Z Using wallet directory /home/hebasto/.bitcoin/signet/wallets
2021-06-20T05:45:29Z init message: Verifying wallet(s)…
2021-06-20T05:45:29Z Using SQLite Version 3.32.1
2021-06-20T05:45:29Z Using wallet /home/hebasto/.bitcoin/signet/wallets/210528d
fcntl64 hack can't use glibc flock directlyterminate called without an active exception
Aborted (core dumped)

@sipa
Copy link
Member

sipa commented Jun 20, 2021

You should compile with the change that prints the cmd value, so the missing cmd can be added. This is entirely expected - the wrapper needs to know all the used cmd values, and the code you copied only has a few.

@hebasto
Copy link
Member Author

hebasto commented Jun 20, 2021

You should compile with the change that prints the cmd value, so the missing cmd can be added. This is entirely expected - the wrapper needs to know all the used cmd values, and the code you copied only has a few.

Ok. I'll do.

FWIW, it is not a missed cmd value, rather this stub for file locking features:

takes_flock_ptr_INCOMPATIBLE:
    //
    // !!! This is the breaking case: the size of the flock
    // structure changed to accommodate larger files.  If you
    // need this, you'll have to define a compatibility struct
    // with the older glibc and make your own entry point using it,
    // then call fcntl64() with it directly (bear in mind that has
    // been remapped to the old fcntl())
    //
    fprintf(stderr, "fcntl64 hack can't use glibc flock directly");
    exit(1);

@hebasto hebasto reopened this Jun 20, 2021
@sipa
Copy link
Member

sipa commented Jun 20, 2021

Oh, you're right. That's a bigger problem.

@hebasto hebasto marked this pull request as draft June 20, 2021 06:06
@sipa
Copy link
Member

sipa commented Jun 20, 2021

Reverting #21036 isn't an option for guix, I assume.

@hebasto
Copy link
Member Author

hebasto commented Jun 20, 2021

Oh, you're right. That's a bigger problem.

Btw, the offensive cmd is F_SETLK 6 /* Set record locking info (non-blocking). */

@sipa
Copy link
Member

sipa commented Jun 20, 2021

Yes, so we'll need to implement a wrapper for that. None of the call sites do anything complicated (and in particular, none pass file ranges, which could lead to actual imcompatibilities).

@hebasto hebasto marked this pull request as ready for review June 20, 2021 12:49
@hebasto
Copy link
Member Author

hebasto commented Jun 20, 2021

@sipa

Yes, so we'll need to implement a wrapper for that. None of the call sites do anything complicated (and in particular, none pass file ranges, which could lead to actual imcompatibilities).

Done. OP updated.

@sipa
Copy link
Member

sipa commented Jun 20, 2021

I don't think that's correct. The old comment for the incompatible case explained that the flock data structure had changed in old glibc vs new one, so you'd need to convert between the two in the wrapper.

@hebasto
Copy link
Member Author

hebasto commented Jun 21, 2021

Btw, if we stop release ARM-32bit binaries, the fcntl64@GLIBC_2.28 symbol could be completely eliminated with $(package)_cppflags_linux = -DSQLITE_DISABLE_LFS in depends/packages/sqlite.mk.

@hebasto
Copy link
Member Author

hebasto commented Jun 21, 2021

@sipa

I don't think that's correct. The old comment for the incompatible case explained that the flock data structure had changed in old glibc vs new one, so you'd need to convert between the two in the wrapper.

I've compared struct flock and struct flock64 in glibc 2.31 (new) to struct flock in glibc 2.27 (old) -- all member sizes are the same for all tested archs. It seems with our (default) glibc feature set no need to remap struct flock.

@laanwj
Copy link
Member

laanwj commented Jun 21, 2021

Btw, if we stop release ARM-32bit binaries, the fcntl64@GLIBC_2.28

NACK on so suddenly stopping support for ARM 32 bit before the release. If there is a reason to phase it out we should at least announce it a release in advance. But not sure this is a good idea. For the record I'm still running a node on 32-bit ARM hardware myself, and remember that 64-bit RPi's used to ship with 32 bit software stack until very recently.

@hebasto
Copy link
Member Author

hebasto commented Jun 21, 2021

For the record I'm still running a node on 32-bit ARM hardware myself...

So am I :)

@laanwj
Copy link
Member

laanwj commented Jun 21, 2021

could be completely eliminated with $(package)_cppflags_linux = -DSQLITE_DISABLE_LFS in depends/packages/sqlite.mk.

Having thought about it a bit I think for other platforms this is a good angle though. The only use of this function in our distributed binary is from sqlite, after all. Not using the symbol would be more comfortable to me than providing our own (potentially buggy) implementation. For ARM32 we could bump the minimum GLIBC version.

@DrahtBot
Copy link
Contributor

Guix builds

File commit 6a67366
(master)
commit b5b94f1
(master and this pull)
SKIPATTEST.TAG e3b0c44298fc1c14... e3b0c44298fc1c14...
*.tar.gz ed3096aa949f1b56... 4a313dce87f47a84...
guix_build.log 7920a375a22476b1... 7d5cc645c0a46904...
guix_build.log.diff ec96553de4e951ec...

@hebasto
Copy link
Member Author

hebasto commented Jun 22, 2021

could be completely eliminated with $(package)_cppflags_linux = -DSQLITE_DISABLE_LFS in depends/packages/sqlite.mk.

Having thought about it a bit I think for other platforms this is a good angle though. The only use of this function in our distributed binary is from sqlite, after all. Not using the symbol would be more comfortable to me than providing our own (potentially buggy) implementation. For ARM32 we could bump the minimum GLIBC version.

#22305 is an alternative approach.

@DrahtBot
Copy link
Contributor

Gitian builds

File commit 398dd67
(master)
commit af38111
(master and this pull)
*-osx-unsigned.dmg 92e2d116b58d4214... 84609ca486e37151...
*-osx64.tar.gz 02578406a266bd45... edf43cc82a17ce60...
*.tar.gz b16fc2a40cba5b8c... a154737ea6fb8a77...
bitcoin-core-osx-22-res.yml f2972b31a41adb54... 09a3a0c97a691317...
linux-build.log 6c8e74cad31e39a3... ed443242cb7063fd...
osx-build.log bb8f3e7c16328172... b83096dfc1d68087...
win-build.log 2eea226b75083143... c97c905ce7e176db...
bitcoin-core-osx-22-res.yml.diff 65aeb4d4d464eee3...
linux-build.log.diff b140ad14e0f0c7b5...
osx-build.log.diff 1d12c97443fcdbee...
win-build.log.diff 82e48d93bb7e9584...

@hebasto
Copy link
Member Author

hebasto commented Jul 1, 2021

Closing in favor of #22365.

@hebasto hebasto closed this Jul 1, 2021
@hebasto hebasto mentioned this pull request Jul 8, 2021
@hebasto hebasto deleted the 210619-fcntl branch October 5, 2021 11:38
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 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.

5 participants