build: Disable AVX2 code path on mingw builds

laanwj commented at 10:24 pm on March 31, 2022: member

The stack is 16 byte aligned according to the ABI, but gcc assumes 32 byte alignment during ~~register spilling~~ write of function arguments (doesn’t re-align the stack pointer), resulting in ~50% chance of a crash.

Avoid this issue by disabling detection of AVX2 compiler support when compiling with mingw-w64. This should be enough, none of the other extended instruction sets uses 256 bit types.

Newer systems will use SHANI for SHA256 acceleration, older ones will fall back to one of the other (maybe a little slower) optimized implementations.

Upstream bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412

Fixes #24726.

build: Disable AVX2 code path on mingw builds

The stack is 16 byte aligned according to the ABI, but gcc assumes 32
byte alignment during register spilling (doesn't re-align the stack
pointer), resulting in ~50% chance of a crash.

Avoid this issue by disabling detection of AVX2 compiler support when
compiling with mingw-w64.

Upstream bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412

Fixes #24726.

fc9b4e87e6

laanwj added the label Windows on Mar 31, 2022

laanwj added the label Build system on Mar 31, 2022

laanwj added the label Needs backport (23.x) on Mar 31, 2022

laanwj added this to the milestone 23.0 on Mar 31, 2022

theuni commented at 10:36 pm on March 31, 2022: member

Code review ACK, though this still seems mysterious to me.

Any idea why we’re just now running into this? Afaics that gcc bug has been around for years.

laanwj commented at 10:39 pm on March 31, 2022: member

I don’t know. Did the previous mingw-w64 compiler have AVX2 support at all? If so, we could compare the assembly of the _ZN14sha256d64_avx214Transform_8wayEPhPKh with that of older releases.

It’s also mysterious it’s a pretty rare problem. E.g. in @hebasto’s case it only turns up on -signet. Maybe the stack is almost always aligned to 32 bytes for some reason it’s just not guaranteed.

That said, if this really fixes it, I prefer this solution. Who knows how many mysterious crashes have gone unreported. People blaming their hardware, or Windows.

jonatack commented at 10:49 pm on March 31, 2022: member

Review ACK fc9b4e87e6bc083fc39af452473d3da29438cc10 see IRC discussion from https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-686

hebasto approved

hebasto commented at 5:36 am on April 1, 2022: member

ACK fc9b4e87e6bc083fc39af452473d3da29438cc10, tested Guix build bitcoin-fc9b4e87e6bc-win64.zip on Windows 11 Pro 21H2:

0C:\Users\hebasto\Desktop\pr24727-avx2\bitcoin-fc9b4e87e6bc>bin\bitcoind.exe -signet
12022-04-01T05:32:53Z Bitcoin Core version v23.99.0-gfc9b4e87e6bc083fc39af452473d3da29438cc10 (release build)
22022-04-01T05:32:53Z Signet derived magic (message start): 0a03cf40
32022-04-01T05:32:53Z Assuming ancestors of block 00000112852484b5fe3451572368f93cfd2723279af3464e478aee35115256ef have valid signatures.
42022-04-01T05:32:53Z Setting nMinimumChainWork=000000000000000000000000000000000000000000000000000000de26b0e471
52022-04-01T05:32:53Z Using the 'sse4(1way),sse41(4way)' SHA256 implementation
6...

Still curious why v22.0 works fine?

laanwj commented at 7:40 am on April 1, 2022: member

Still curious why v22.0 works fine?

Have you checked that it picks the AVX2 code path on v22.0? E.g. that line would show avx2(8way).

hebasto commented at 7:41 am on April 1, 2022: member

Still curious why v22.0 works fine?

Have you checked that it picks the AVX2 code path on v22.0? E.g. that line would show avx2(8way).

Yes, v22.0 does pick AVX2:

0C:\Users\hebasto\Desktop\22.0\bitcoin-22.0>bin\bitcoind.exe -signet
12022-04-01T07:42:37Z Bitcoin Core version v22.0.0 (release build)
22022-04-01T07:42:37Z Signet derived magic (message start): 0a03cf40
32022-04-01T07:42:37Z Assuming ancestors of block 000000187d4440e5bff91488b700a140441e089a8aaea707414982460edbfe54 have valid signatures.
42022-04-01T07:42:37Z Setting nMinimumChainWork=0000000000000000000000000000000000000000000000000000008546553c03
52022-04-01T07:42:37Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
6...

laanwj commented at 7:51 am on April 1, 2022: member

So I see three possible reasons:

It just happens that %esp is always 32-aligned on 22.0 when calling that specific function. Voodoo magic witchcraft. (Edit: I think this one is ruled out: 22.0 bitcoind.exe does not contain any vmovdqa instructions involving the stack, so 32-alignment ought to not matter)
gcc got “smarter” about alignment and started using the 256-bit aligned store vmovdqa instead of split-aligned or non-aligned store in this case. After all, we did change compiler version to 10.3 right?
gcc got dumber and needs to spill, where it could juggle everything in registers before.

To know for sure we would need to compare the disassembly of that function. I can take a look.

laanwj commented at 9:33 am on April 1, 2022: member

I looked at the assembly again and the direct cause seems to be that gcc 10.3 stopped inlining sha256d64_avx2::Write8. This puts the argument on the stack. It’s not spilling as I thought.

Forcing inline of Write might work for a bit but is brittle and unsafe.

Thanks to @fanquake we may have a better alternative solution. Other distributions patch mingw-w64 to not generate AVX code that makes alignment assumptions. E.g. this is debian’s patch: https://salsa.debian.org/mingw-w64-team/gcc-mingw-w64/-/blob/master/debian/patches/vmov-alignment.patch

If we can integrate it in the guix build it may be safe to keep AVX2 enabled for the windows build.

fanquake referenced this in commit d6fae988ef on Apr 1, 2022

laanwj commented at 1:03 pm on April 1, 2022: member

Closing in favor of #24736

laanwj closed this on Apr 1, 2022

MarcoFalke removed the label Needs backport (23.x) on Apr 1, 2022

laanwj referenced this in commit 83b26cb97c on Apr 4, 2022

fanquake referenced this in commit db8a5d6094 on Apr 4, 2022

sidhujag referenced this in commit 66206f4616 on Apr 4, 2022

dekm referenced this in commit dcacc71ca6 on Oct 27, 2022

dekm referenced this in commit 7be91387dd on Nov 7, 2022

dekm referenced this in commit 70b22f0f88 on Nov 12, 2022

DrahtBot locked this on Apr 1, 2023

build: Disable AVX2 code path on mingw builds #24727