guix: replace GCC unaligned VMOV patch with binutils patch #29846
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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`.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
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. |
|
Concept ACK. |
|
My Guix builds: |
laanwj
reviewed
Apr 11, 2024
| Author: fanquake <fanquake@gmail.com> | ||
| Date: Wed Apr 10 12:15:52 2024 +0200 | ||
|
|
||
| build: turn on -muse-unaligned-vector-move by default |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's so we build the whole world / toolchain / depends etc with this option.
|
Concept ACK, will test |
|
Concept ACK, looks like a strict improvement. |
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 |
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Nov 4, 2024
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Nov 5, 2024
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Nov 10, 2024
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
Nov 12, 2024
, bitcoin#28786, bitcoin#29078, bitcoin#27897, bitcoin#29651, bitcoin#29695, bitcoin#29673, bitcoin#29828, bitcoin#29846, bitcoin#30231, bitcoin#30438, partial bitcoin#30511 (guix backports: part 5) 91b7ef8 merge bitcoin#30438: build Linux GCC with --enable-cet (Kittywhiskers Van Gogh) cfc6cba partial bitcoin#30511: GCC 12 consolidation (Kittywhiskers Van Gogh) 06f5431 merge bitcoin#30231: bump time-machine to f0bb724211872cd6158fce6162e0b8c73efed126 (Kittywhiskers Van Gogh) 5b292ee merge bitcoin#29846: replace GCC unaligned VMOV patch with binutils patch (Kittywhiskers Van Gogh) 4d1f7dc merge bitcoin#29828: remove `gcc-toolchain static` from Windows build (Kittywhiskers Van Gogh) f321d3d merge bitcoin#29673: use GCC 11 in macOS build env (Kittywhiskers Van Gogh) d570e2d merge bitcoin#29695: build GCC with --enable-standard-branch-protection (Kittywhiskers Van Gogh) c965943 merge bitcoin#29651: bump time-machine to dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a (Kittywhiskers Van Gogh) 59a125a merge bitcoin#27897: use GCC 12.3.0 to build releases (Kittywhiskers Van Gogh) a701b06 merge bitcoin#29078: Bump guix time-machine to unlock riscv64 metal (Kittywhiskers Van Gogh) d4b10a3 merge bitcoin#28786: switch to 6.1 kernel headers over 5.15 (Kittywhiskers Van Gogh) c371870 merge bitcoin#28580: update time-machine (Kittywhiskers Van Gogh) d36c9b6 merge bitcoin#28759: update signapple to latest master (Kittywhiskers Van Gogh) 38c71d8 merge bitcoin#28370: remove GCC 10 workaround from NSIS (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6382 * Dependency for #6384 ## Breaking Changes None expected ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 91b7ef8 Tree-SHA512: 0cfb436a430cf4b624a48a9928ecac9cd5c50e88e51ed04e7d1d0100968af8be1183364f035ac75153781a5e1616aa2f6fadabf0a1c03ec4b66dedea544b77ad
Fabcien
added a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 29, 2024
Summary: ``` 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`. ``` Backport of [[bitcoin/bitcoin#29846 | core#29846]]. Depends on D17235. Test Plan: Run the guix windows build Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D17236
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Dec 2, 2024
Summary: ``` 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`. ``` Backport of [[bitcoin/bitcoin#29846 | core#29846]]. Depends on D17235. Test Plan: Run the guix windows build Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D17236
knst
added a commit
to knst/dash
that referenced
this pull request
Dec 26, 2024
, bitcoin#28580, bitcoin#28786, bitcoin#29078, bitcoin#27897, bitcoin#29651, bitcoin#29695, bitcoin#29673, bitcoin#29828, bitcoin#29846, bitcoin#30231, bitcoin#30438, partial bitcoin#30511 (guix backports: part 5)" This reverts commit be97bfe, reversing changes made to f155ecf.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
This PR: