build: Windows debug cross-build fails with `-O0`

hebasto commented at 8:44 am on July 20, 2023: member

Is there an existing issue for this?

I have searched the existing issues

Current behaviour

When cross-compiling for Windows with the -O0 optimization level the build fails.

The error can be fixed by the -Wa,-mbig-obj assembler flag. However, @theuni suggested:

We should definitely look into breaking up that object.

Expected behaviour

The build completes with no error.

Steps to reproduce

0$ make -C depends HOST=x86_64-w64-mingw32 DEBUG=1 NO_QT=1
1$ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site --enable-debug CFLAGS=-O0 CXXFLAGS=-O0
2$ make

Relevant log output

0...
1/usr/bin/x86_64-w64-mingw32-as: wallet/libbitcoin_wallet_a-wallet.o: too many sections (46006)
2{standard input}: Assembler messages:
3{standard input}: Fatal error: can't write 47 bytes to section .text of wallet/libbitcoin_wallet_a-wallet.o: 'file too big'
4/usr/bin/x86_64-w64-mingw32-as: wallet/libbitcoin_wallet_a-wallet.o: too many sections (46006)
5{standard input}: Fatal error: wallet/libbitcoin_wallet_a-wallet.o: file too big
6...

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

master@5608e1d3b4ba3a23c05937918046e428be890505

Operating system and version

Ubuntu 23.04

Machine specifications

No response

hebasto added the label Build system on Jul 20, 2023

hebasto added the label Windows on Jul 20, 2023

theuni commented at 1:29 pm on July 20, 2023: member

Ping @achow101.

Being unable to build with -O0 is not a big deal.

However, that means we’re relying on compiler optimizations to reduce the code size first in order for tools to be able to process them with sane defaults. Breaking up wallet.cpp somewhat is prudent, imo.

achow101 commented at 2:28 pm on July 20, 2023: member

How big is too big?

wallet.cpp is pretty much entirely CWallet’s members, so breaking it up will require breaking up CWallet which I’m not sure we want to do? I suppose not all of the members need to be in CWallet, but I think it’ll still be necessary for that class to be kinda big.

theuni commented at 2:39 pm on July 20, 2023: member

How big is too big?

Good question :)

wallet.cpp is pretty much entirely CWallet’s members, so breaking it up will require breaking up CWallet which I’m not sure we want to do?

Yeah, agreed that we shouldn’t be breaking up classes for something like this. I suspect this actually has more to do with the giant list of includes. Specifically, getting rid of boost signals (here’s a POC from a while back) would probably be enough to fix the issue for good. Another reason to kill them off :)

MarcoFalke commented at 3:25 pm on July 20, 2023: member

Maybe every class other than CWallet can be moved to a separate module?

Also, a bunch of includes can be killed, according to iwyu:

 0wallet/wallet.h should remove these lines:
 1- #include <interfaces/wallet.h>  // lines 12-12
 2- #include <psbt.h>  // lines 16-16
 3- #include <util/message.h>  // lines 20-20
 4- #include <util/strencodings.h>  // lines 22-22
 5- #include <validationinterface.h>  // lines 26-26
 6- #include <wallet/walletdb.h>  // lines 30-30
 7- #include <algorithm>  // lines 33-33
 8- #include <stdexcept>  // lines 39-39
 9- class CScript;  // lines 51-51
10- enum class FeeEstimateMode;  // lines 52-52
11- namespace wallet { class CWalletTx; }  // lines 121-121
12- namespace wallet { class ReserveDestination; }  // lines 122-122

0wallet/wallet.cpp should remove these lines:
1- #include <policy/fees.h>  // lines 20-20
2- #include <policy/policy.h>  // lines 21-21
3- #include <util/bip32.h>  // lines 31-31
4- #include <util/fees.h>  // lines 34-34
5- #include <util/rbf.h>  // lines 38-38
6- #include <wallet/fees.h>  // lines 44-44

theuni commented at 3:44 pm on July 20, 2023: member

I don’t see any reason not to do the iwyu trimming. Though sadly it doesn’t look like it’ll help too much. Preprocessed (gcc -E -O0) numbers for wallet/wallet.cpp before and after @MarcoFalke’s suggested removals:

0Before: 191,084 lines
1After: 190,899 lines

sipa commented at 3:51 pm on July 20, 2023: member

There is no requirement that an entire class is defined within a single compilation unit, I believe. If you don’t have function definitions inside the class itself, then those definitions can be given in any .cpp file.

That said, for understandability I’d strongly prefer not splitting a class over multiple .cpp files…

bitcoin deleted a comment on Jul 23, 2023

MarcoFalke commented at 1:53 pm on August 2, 2023: member

Looking at -ftime-trace=/tmp/, I don’t think there is anything that can be done, other than to nuke boost, or to define a smaller limited interface of our boost usage in a new header and hide the boost includes in the corresponding cpp file?

boost_sad

build: Windows debug cross-build fails with -O0 #28109