build: Avoid fcntl64@GLIBC_2.28 symbol when --enable-glibc-back-compat

hebasto commented at 0:04 am on June 20, 2021: member

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

Gitian builds:

 0Generating report
 15d2b1cf6c0a59796e0e087877faff290a252f79bae1a61aeceeda15e5c316186  bitcoin-530bc278b9fa-aarch64-linux-gnu-debug.tar.gz
 2a67a2a8a160d289fabe786e00715eeccf800e418a305d35a774721a74f738a4a  bitcoin-530bc278b9fa-aarch64-linux-gnu.tar.gz
 3315f787b8f77b55a37bf56201c747f77c5641c6eab1779049fdc65ef6e3aa162  bitcoin-530bc278b9fa-arm-linux-gnueabihf-debug.tar.gz
 458d1b886f10b9a6011645bb65795ed0d418433c44b83fe8359ed4b31f501c167  bitcoin-530bc278b9fa-arm-linux-gnueabihf.tar.gz
 531eedc8531fca0b4bd76c5b0cbc8799f6621a758e5fe9abda3aef3f264832c89  bitcoin-530bc278b9fa-powerpc64-linux-gnu-debug.tar.gz
 679b8ca4bee6fc24a5c3216fed95f341510f15cdaaf04e2b6d4db991a3d077450  bitcoin-530bc278b9fa-powerpc64-linux-gnu.tar.gz
 70fcd01009aa80b9c4b567a241d0d56b579756d9f70a1a78e0fa8c015b99d9bfa  bitcoin-530bc278b9fa-powerpc64le-linux-gnu-debug.tar.gz
 8af63f25b3de98cf2f25b6592dec9c3b4f95021fcc11396fb345c592979b9e4b7  bitcoin-530bc278b9fa-powerpc64le-linux-gnu.tar.gz
 9539d6317a7c2cfe6cbecb07eb82ef9b039010ef544cc9a74984a7cb23c03b58a  bitcoin-530bc278b9fa-riscv64-linux-gnu-debug.tar.gz
1075ce0a0eabb4555589e632676b2e44df8365f320dea28d9edcd54ec78b4038d1  bitcoin-530bc278b9fa-riscv64-linux-gnu.tar.gz
119dc543d3b0c9ea39e153616132b0c41205a639d0129b752e7151712b8eda54b7  bitcoin-530bc278b9fa-x86_64-linux-gnu-debug.tar.gz
12ba5452615ebd1c1398916837ee1a36233721819c08f48ea12ac7c03ac67526c7  bitcoin-530bc278b9fa-x86_64-linux-gnu.tar.gz
132b618a7b6e99bffc15522ca9f4bc0bfb1187a9bb0b005cc32d23127289644cc1  src/bitcoin-530bc278b9fa.tar.gz
149db429d8b661e2d7a48be8b8f9bcbb3df4e8a6b8cce0bd0660d54c1422c0d696  bitcoin-core-linux-22-res.yml
15Done.

I tested only:

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

hebasto added the label Build system on Jun 20, 2021

hebasto force-pushed on Jun 20, 2021

hebasto marked this as a draft on Jun 20, 2021

sipa commented at 0:29 am on June 20, 2021: member

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.

in src/compat/glibc_compat.cpp:101 in 639bfc4b35 outdated

 96+    case F_SET_RW_HINT: goto takes_uint64_t_ptr;
 97+    case F_GET_FILE_RW_HINT: goto takes_uint64_t_ptr;
 98+    case F_SET_FILE_RW_HINT: goto takes_uint64_t_ptr;
 99+
100+    default:
101+        fprintf(stderr, "fcntl64 workaround got unknown F_XXX constant");

sipa commented at 0:35 am on June 20, 2021:

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 force-pushed on Jun 20, 2021

hebasto marked this as ready for review on Jun 20, 2021

hebasto commented at 5:11 am on June 20, 2021: member

Updated 639bfc4b359bd9364829d3abb8e12e16d432471c -> f0ca8bc08c0d238cdd88844190be0a11b4bcf1d5 (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:

bump minimum glibc version for the 22.0 release binaries
revert ca85449f22cb909b9ca83861aa51fbf8f40a9070 (#21036) for Linux builds
something else?

sipa commented at 5:18 am on June 20, 2021: member

How did you fix the failures?

hebasto commented at 5:25 am on June 20, 2021: member

How did you fix the failures?

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

hebasto commented at 5:42 am on June 20, 2021: member

Closing. Binaries are broken (

hebasto closed this on Jun 20, 2021

sipa commented at 5:58 am on June 20, 2021: member

How are they broken?

hebasto commented at 6:00 am on June 20, 2021: member

How are they broken?

On x86_64 machine:

 0$ bin/bitcoind -signet
 12021-06-20T05:45:29Z Bitcoin Core version v21.99.0-f0ca8bc08c0d (release build)
 22021-06-20T05:45:29Z Signet derived magic (message start): 0a03cf40
 32021-06-20T05:45:29Z Assuming ancestors of block 0000002a1de0f46379358c1fd09906f7ac59adf3712323ed90eb59e4c183c020 have valid signatures.
 42021-06-20T05:45:29Z Setting nMinimumChainWork=00000000000000000000000000000000000000000000000000000019fd16269a
 52021-06-20T05:45:29Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
 62021-06-20T05:45:29Z Using RdSeed as additional entropy source
 72021-06-20T05:45:29Z Using RdRand as an additional entropy source
 82021-06-20T05:45:29Z Default data directory /home/hebasto/.bitcoin
 92021-06-20T05:45:29Z Using data directory /home/hebasto/.bitcoin/signet
102021-06-20T05:45:29Z Config file: /home/hebasto/.bitcoin/bitcoin.conf
112021-06-20T05:45:29Z Config file arg: dbcache="8192"
122021-06-20T05:45:29Z Config file arg: logips="1"
132021-06-20T05:45:29Z Config file arg: logthreadnames="1"
142021-06-20T05:45:29Z Config file arg: maxmempool="500"
152021-06-20T05:45:29Z Config file arg: server="1"
162021-06-20T05:45:29Z Config file arg: [main] addnode="ejxefzf5fpst4mg2rib7grksvscl7p6fvjp6agzgfc2yglxnjtxc3aid.onion"
172021-06-20T05:45:29Z Config file arg: [main] addnode="rp7k2go3s5lyj3fnj6zn62ktarlrsft2ohlsxkyd7v3e3idqyptvread.onion"
182021-06-20T05:45:29Z Config file arg: [main] addnode="mwmfluek4au6mxxpw6fy7sjhkm65bdfc7izc7lpz3trewfdghyrzsbid.onion"
192021-06-20T05:45:29Z Config file arg: [main] blocksdir="/home/hebasto/Blockchain"
202021-06-20T05:45:29Z Config file arg: [main] dnsseed="0"
212021-06-20T05:45:29Z Config file arg: [main] listenonion="1"
222021-06-20T05:45:29Z Config file arg: [main] onion="127.0.0.1:9050"
232021-06-20T05:45:29Z Config file arg: [main] uacomment="h42"
242021-06-20T05:45:29Z Config file arg: [main] wallet=false
252021-06-20T05:45:29Z Config file arg: [regtest] blocksdir="/home/hebasto/VMs/bitcoin"
262021-06-20T05:45:29Z Config file arg: [signet] blocksdir="/home/hebasto/VMs/bitcoin"
272021-06-20T05:45:29Z Config file arg: [test] blocksdir="/home/hebasto/VMs/bitcoin"
282021-06-20T05:45:29Z Config file arg: [test] listen="1"
292021-06-20T05:45:29Z Config file arg: [test] onlynet="ipv4"
302021-06-20T05:45:29Z Config file arg: [test] server="1"
312021-06-20T05:45:29Z Setting file arg: wallet = ["210528d","210528-bdb"]
322021-06-20T05:45:29Z Command-line arg: signet=""
332021-06-20T05:45:29Z Using at most 125 automatic connections (1024 file descriptors available)
342021-06-20T05:45:29Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
352021-06-20T05:45:29Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
362021-06-20T05:45:29Z Script verification uses 7 additional threads
372021-06-20T05:45:29Z scheduler thread start
382021-06-20T05:45:29Z HTTP: creating work queue of depth 16
392021-06-20T05:45:29Z Using random cookie authentication.
402021-06-20T05:45:29Z Generated RPC authentication cookie /home/hebasto/.bitcoin/signet/.cookie
412021-06-20T05:45:29Z HTTP: starting 4 worker threads
422021-06-20T05:45:29Z Using wallet directory /home/hebasto/.bitcoin/signet/wallets
432021-06-20T05:45:29Z init message: Verifying wallet(s)…
442021-06-20T05:45:29Z Using SQLite Version 3.32.1
452021-06-20T05:45:29Z Using wallet /home/hebasto/.bitcoin/signet/wallets/210528d
46fcntl64 hack can't use glibc flock directlyterminate called without an active exception
47Aborted (core dumped)

sipa commented at 6:01 am on June 20, 2021: member

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 commented at 6:05 am on June 20, 2021: member

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:

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

hebasto reopened this on Jun 20, 2021

sipa commented at 6:06 am on June 20, 2021: member

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

hebasto marked this as a draft on Jun 20, 2021

sipa commented at 6:16 am on June 20, 2021: member

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

hebasto commented at 6:34 am on June 20, 2021: member

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

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

sipa commented at 6:36 am on June 20, 2021: member

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

build: Avoid fcntl64@GLIBC_2.28 symbol when --enable-glibc-back-compat 530bc278b9

hebasto force-pushed on Jun 20, 2021

hebasto marked this as ready for review on Jun 20, 2021

hebasto commented at 12:50 pm on June 20, 2021: member

@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 commented at 1:44 pm on June 20, 2021: member

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 commented at 5:33 am on June 21, 2021: member

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 commented at 9:32 am on June 21, 2021: member

@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 commented at 9:32 am on June 21, 2021: member

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 commented at 9:36 am on June 21, 2021: member

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

So am I :)

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

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

laanwj added this to the milestone 22.0 on Jun 21, 2021

laanwj commented at 2:40 pm on June 21, 2021: member

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 commented at 6:37 pm on June 21, 2021: member

Guix builds

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

DrahtBot removed the label Needs Guix build on Jun 21, 2021

hebasto commented at 1:26 am on June 22, 2021: member

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 commented at 3:54 am on June 22, 2021: member

Gitian builds

File	commit 398dd678338971d2189934713c83c184742f293f(master)	commit af38111ffb11e63339214ee1bfeffcc491d0c975(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...`

DrahtBot removed the label Needs gitian build on Jun 22, 2021

hebasto commented at 8:59 am on July 1, 2021: member

Closing in favor of #22365.

hebasto closed this on Jul 1, 2021

hebasto deleted the branch on Oct 5, 2021

DrahtBot locked this on Oct 30, 2022

build: Avoid fcntl64@GLIBC_2.28 symbol when –enable-glibc-back-compat #22287

Gitian builds:

Guix builds

Gitian builds