guix: replace GCC unaligned VMOV patch with binutils patch

fanquake commented at 12:23 pm on April 10, 2024: member

Rather than invasively patching GCC, given we have binutils 2.38 available, we can patch it to flip the default for -muse-unaligned-vector-move.

A 1 line binutils patch, is much more maintainable than the ~300 line patch into GCC. It’s also a slight inprovement in regards to patching out ualigned instructions in the release binaries. For comparison: Master:

0objdump -D bin/*.exe | rg "vmova|vmovdqa|vmovaps|vmovapd|vmovdqa64|vmovdqa32" 
1141b8be20: c5 f8 28 1a                 	vmovaps	(%rdx), %xmm3
21420564b3: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
31403060f3: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
4140792b13: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
5140cb0693: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
61415ea0f3: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)

This PR:

0objdump -D bin/*.exe | rg "vmova|vmovdqa|vmovaps|vmovapd|vmovdqa64|vmovdqa32"
1141b8be20: c5 f8 28 1a                 	vmovaps	(%rdx), %xmm3
21420564b3: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
31403060f3: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
4140792b13: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
5140cb0693: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)

guix: replace GCC unaligned VMOV patch with binutils patch

Rather than invasively patching GCC. Given we have binutils 2.38
available, we can patch it to flip the default for
`-muse-unaligned-vector-move`.

a0dc2ebcda

DrahtBot commented at 12:23 pm on April 10, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	laanwj
Concept ACK	hebasto, theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Build system on Apr 10, 2024

fanquake added the label DrahtBot Guix build requested on Apr 10, 2024

willcl-ark commented at 12:51 pm on April 10, 2024: contributor

Will this close #28413 ?

fanquake commented at 2:27 pm on April 10, 2024: member

Will this close #28413 ?

Not quite, we don’t yet have a check. I’d still like to add one, but given we’ve still got occurances, we’d currently have to add exceptions in some way.

hebasto commented at 8:58 pm on April 10, 2024: member

Concept ACK.

DrahtBot commented at 0:21 am on April 11, 2024: contributor

Guix builds (on x86_64)

File	commit 3f6a6da3b08da15c10cbf2806f672da4a57254f6(master)	commit 6dfee9588ecc50a247de2d8f31a62ab21333b780(master and this pull)
SHA256SUMS.part	`d5aaa93554d4820d...`	`6ea962be116fefc0...`
*-aarch64-linux-gnu-debug.tar.gz	`08e8bbba94777556...`	`938917e0f7d1d08b...`
*-aarch64-linux-gnu.tar.gz	`749cf5ff444fabe9...`	`f74a5b9c90c52a3e...`
*-arm-linux-gnueabihf-debug.tar.gz	`b93daca0edcd78ad...`	`6b758bc6dd65c9b9...`
*-arm-linux-gnueabihf.tar.gz	`bccd9690ed40b22d...`	`cee37bd5a203cebf...`
*-arm64-apple-darwin-unsigned.tar.gz	`fd02e2548e0f1518...`	`bb0f5c0f8b5f6a8e...`
*-arm64-apple-darwin-unsigned.zip	`f1f7c3e990bedb80...`	`fa9c6f5f86c5eb5e...`
*-arm64-apple-darwin.tar.gz	`44db61fa0d635786...`	`68b5f5da0d168eeb...`
*-powerpc64-linux-gnu-debug.tar.gz	`ad5bb151cedfa33c...`	`1919e8e0dcd95aaf...`
*-powerpc64-linux-gnu.tar.gz	`77ed970ee0d8cc3c...`	`a062cae464528194...`
*-riscv64-linux-gnu-debug.tar.gz	`4f8a96197351c941...`	`4be0e49990652167...`
*-riscv64-linux-gnu.tar.gz	`cf4025104b87c312...`	`7c9a5ff6c6f4e62d...`
*-x86_64-apple-darwin-unsigned.tar.gz	`ac20abdf695deb92...`	`5ff13a8f0bc3dee0...`
*-x86_64-apple-darwin-unsigned.zip	`95fce23c931cd999...`	`52a9e920c8a59704...`
*-x86_64-apple-darwin.tar.gz	`b08e9359b9d83b46...`	`e13ab9d1cb6fdc6e...`
*-x86_64-linux-gnu-debug.tar.gz	`efa0ecb4fc85f75d...`	`0867644507c37a5a...`
*-x86_64-linux-gnu.tar.gz	`a1963261f8540280...`	`5e4ad9b118f54c8a...`
*.tar.gz	`7809f81ee0abf78c...`	`f988dd20d8ac92ef...`
guix_build.log	`919854cf11856935...`	`5cd528cd8927416a...`
guix_build.log.diff		`235af24648504dfa...`

DrahtBot removed the label DrahtBot Guix build requested on Apr 11, 2024

hebasto commented at 6:23 am on April 11, 2024: member

My Guix builds:

 0x86_64
 107f97e7479e5a7e1baae091ed74839e9b7be118d0504a8b334c4add8e13c78cc  guix-build-a0dc2ebcda9e/output/aarch64-linux-gnu/SHA256SUMS.part
 2ee4ab82a42c802dfa1e455e6badb6a49e819c77f03fcaf3568656faa5f69dce4  guix-build-a0dc2ebcda9e/output/aarch64-linux-gnu/bitcoin-a0dc2ebcda9e-aarch64-linux-gnu-debug.tar.gz
 3f405a168ea3aa2b3a230664381babb406b03680404ebb89e1638f07efa872520  guix-build-a0dc2ebcda9e/output/aarch64-linux-gnu/bitcoin-a0dc2ebcda9e-aarch64-linux-gnu.tar.gz
 498c203bb0d8c6a2568160e5fe78982ed7903236ca1d3e4bb795dc20456abeb2c  guix-build-a0dc2ebcda9e/output/arm-linux-gnueabihf/SHA256SUMS.part
 58fb565e3352621c69829d3f79b43b3e8b49f62b25495032286eab379731c891a  guix-build-a0dc2ebcda9e/output/arm-linux-gnueabihf/bitcoin-a0dc2ebcda9e-arm-linux-gnueabihf-debug.tar.gz
 6d04f8d9776afe64fc2981315f1095536c51c7b839be194ee11292cb7f8267854  guix-build-a0dc2ebcda9e/output/arm-linux-gnueabihf/bitcoin-a0dc2ebcda9e-arm-linux-gnueabihf.tar.gz
 70063adcf7b45e0e2f1100259383d3d49cd9d0ba1cacf6ff2fce77138805eac0c  guix-build-a0dc2ebcda9e/output/arm64-apple-darwin/SHA256SUMS.part
 863029d80ace5e99d6b63dce2f769eafc89add6abe655a887cdb97b3af78e5044  guix-build-a0dc2ebcda9e/output/arm64-apple-darwin/bitcoin-a0dc2ebcda9e-arm64-apple-darwin-unsigned.tar.gz
 94f55f46abf8215bff70cff23e9801540e8c2e96244ac897e256bc9561cd4c399  guix-build-a0dc2ebcda9e/output/arm64-apple-darwin/bitcoin-a0dc2ebcda9e-arm64-apple-darwin-unsigned.zip
1079749cabf2a63d1c63adbb234fc1e5c0fc16ab585ab16db9cae44457804e8091  guix-build-a0dc2ebcda9e/output/arm64-apple-darwin/bitcoin-a0dc2ebcda9e-arm64-apple-darwin.tar.gz
1144e8563638e45987d3d907c0edd53b523b6eee14185e4792cd80bef919625ae4  guix-build-a0dc2ebcda9e/output/dist-archive/bitcoin-a0dc2ebcda9e.tar.gz
128bbb9d79772dfc106b93c4a0842aaa6c48a91271373a970feabefcde504dea78  guix-build-a0dc2ebcda9e/output/powerpc64-linux-gnu/SHA256SUMS.part
13c9ea05121871d00764551019429a9e0bef57b4d5a0f46e306f10d905bf50d0c7  guix-build-a0dc2ebcda9e/output/powerpc64-linux-gnu/bitcoin-a0dc2ebcda9e-powerpc64-linux-gnu-debug.tar.gz
1490344694399225e88f0654018044e3cdc50693e7dec892ec149af130aab5fb9c  guix-build-a0dc2ebcda9e/output/powerpc64-linux-gnu/bitcoin-a0dc2ebcda9e-powerpc64-linux-gnu.tar.gz
1546f008134e0972120e82ea940815ff32349fca0b292455dba4379359445a962c  guix-build-a0dc2ebcda9e/output/riscv64-linux-gnu/SHA256SUMS.part
1613c646cea9a8ab44c3942e17ddc6bad348dfb6877092cdb95cc320fc6d02763a  guix-build-a0dc2ebcda9e/output/riscv64-linux-gnu/bitcoin-a0dc2ebcda9e-riscv64-linux-gnu-debug.tar.gz
17257f749cd33f01cd271fdbd8e802c66bda83886c98bc89d4fc6045afe02d55a4  guix-build-a0dc2ebcda9e/output/riscv64-linux-gnu/bitcoin-a0dc2ebcda9e-riscv64-linux-gnu.tar.gz
185c1cef450825cbe691a1207cad686bb5cf742a2195cb5da6cdad981a62ff4ac7  guix-build-a0dc2ebcda9e/output/x86_64-apple-darwin/SHA256SUMS.part
19bed99cb39f148b057daaeb0420691c9fd22c735013ef4adc415755656e48ddc2  guix-build-a0dc2ebcda9e/output/x86_64-apple-darwin/bitcoin-a0dc2ebcda9e-x86_64-apple-darwin-unsigned.tar.gz
20eef9064c217b030651422d0684338e7c920978deed2c3e36da7509d8f5322a8a  guix-build-a0dc2ebcda9e/output/x86_64-apple-darwin/bitcoin-a0dc2ebcda9e-x86_64-apple-darwin-unsigned.zip
212e23e0f4282691a075609ee80022a2262de90a0e5ed88bc3fd7957589643b3f2  guix-build-a0dc2ebcda9e/output/x86_64-apple-darwin/bitcoin-a0dc2ebcda9e-x86_64-apple-darwin.tar.gz
22a6c26a0e1abab26bfbfdb48de72be0245c468b0fb2c25bcd7ec67a3e861acf8c  guix-build-a0dc2ebcda9e/output/x86_64-linux-gnu/SHA256SUMS.part
23971acc92bcfe937a96a27a5129f9382077c8a89320f7506cd147c9fa14937980  guix-build-a0dc2ebcda9e/output/x86_64-linux-gnu/bitcoin-a0dc2ebcda9e-x86_64-linux-gnu-debug.tar.gz
2450c5bbe2a6987cb9bd4f71bb29fdbc1ac593d82a0213c5a198ff7b4d566305f4  guix-build-a0dc2ebcda9e/output/x86_64-linux-gnu/bitcoin-a0dc2ebcda9e-x86_64-linux-gnu.tar.gz
25697e56a1c3a7d3c71eabc7104ed3e8d0241352778c9cda046278824a732c77e0  guix-build-a0dc2ebcda9e/output/x86_64-w64-mingw32/SHA256SUMS.part
264011c8ef9948f871ccf40b9ecfec164c6eb5d4af5d94f23bb71bb30c75ca0841  guix-build-a0dc2ebcda9e/output/x86_64-w64-mingw32/bitcoin-a0dc2ebcda9e-win64-debug.zip
27c7bf3a072f959791ad6b0cfbd600624f60e06d1fe39c8524560ed047b3d923b7  guix-build-a0dc2ebcda9e/output/x86_64-w64-mingw32/bitcoin-a0dc2ebcda9e-win64-setup-unsigned.exe
283b40301fe84b7ae65b357ffd5329b589f25f84a071affeea1f37652c8f636f3d  guix-build-a0dc2ebcda9e/output/x86_64-w64-mingw32/bitcoin-a0dc2ebcda9e-win64-unsigned.tar.gz
29401e8b433f8a5bc6be0bbcc3b237860aad635ae9e2473a7df03bfaebfc365969  guix-build-a0dc2ebcda9e/output/x86_64-w64-mingw32/bitcoin-a0dc2ebcda9e-win64.zip

in contrib/guix/patches/binutils-unaligned-default.patch:5 in a0dc2ebcda

0@@ -0,0 +1,22 @@
1+commit 6537181f59ed186a341db621812a6bc35e22eaf6
2+Author: fanquake <fanquake@gmail.com>
3+Date:   Wed Apr 10 12:15:52 2024 +0200
4+
5+    build: turn on -muse-unaligned-vector-move by default

laanwj commented at 6:40 am on April 11, 2024:

i think i’m missing something: if this is a build flag, is a patch really needed? Or is this more of a defense-in-depth approach to prevent accidentally forgetting it.

fanquake commented at 7:22 am on April 11, 2024:

It’s so we build the whole world / toolchain / depends etc with this option.

laanwj commented at 7:00 am on April 11, 2024: member

Concept ACK, will test

theuni commented at 12:35 pm on April 11, 2024: member

Concept ACK, looks like a strict improvement.

laanwj commented at 9:29 am on April 15, 2024: member

0objdump -D bin/*.exe | rg "vmova|vmovdqa|vmovaps|vmovapd|vmovdqa64|vmovdqa32" 
1141b8be20: c5 f8 28 1a                 	vmovaps	(%rdx), %xmm3
21420564b3: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
31403060f3: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
4140792b13: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
5140cb0693: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)
61415ea0f3: c5 79 29 36                 	vmovapd	%xmm14, (%rsi)>

i’ve re-read about the issue and from what i understand these are actually fine-the stack alignment problem is about 32 byte (>=256 bit) alignment. And xmm registers are 16-byte aligned. It would be a problem if ymm or zmm. If so, our check in #28413 could skip these.

laanwj commented at 8:20 am on April 16, 2024: member

I get the same output (for windows) as @hebasto
I’ve checked the resulting binaries for instructions that enforce aligned memory accesses, and didn’t find any dangerous ones

Code review ACK a0dc2ebcda9e33aa5320221cd4ea371f84d221fd

DrahtBot requested review from hebasto on Apr 16, 2024

DrahtBot requested review from theuni on Apr 16, 2024

fanquake merged this on Apr 17, 2024

fanquake closed this on Apr 17, 2024

fanquake deleted the branch on Apr 17, 2024

guix: replace GCC unaligned VMOV patch with binutils patch #29846

Code Coverage

Reviews

Guix builds (on x86_64)