backport: bitcoin/bitcoin#28461: build: Windows SSP roundup #6889
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6889.32ad4a7b. A new comment will be made when the image is pushed. |
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contrib/guix/manifest.scm (1)
455-461: Verify the rpath to rpath-link substitution.The pre-configure phase replaces
-rpath=with-rpath-link=in GCC config files. This changes linker behavior:
-rpathembeds runtime library search paths into the binary-rpath-linkonly affects link-time library resolutionVerify that this substitution doesn't cause runtime library resolution issues for binaries built with this toolchain.
Consider documenting the rationale for this substitution in a comment, as it's a subtle but important change to the linker behavior:
(add-after 'pre-configure 'replace-rpath-with-rpath-link (lambda _ + ;; Replace -rpath with -rpath-link to avoid embedding Guix store paths + ;; in the runtime library search path, while still allowing link-time resolution (substitute* (cons "gcc/config/rs6000/sysv4.h" (find-files "gcc/config" "^gnu-user.*\\.h$")) (("-rpath=") "-rpath-link=")) #t))))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
configure.ac(0 hunks)contrib/guix/manifest.scm(1 hunks)
💤 Files with no reviewable changes (1)
- configure.ac
🧰 Additional context used
📓 Path-based instructions (1)
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/guix/manifest.scm
🔇 Additional comments (2)
contrib/guix/manifest.scm (2)
434-435: LGTM: Windows SSP enabled by default in GCC.The addition of
--enable-default-ssp=yesaligns with the PR objective to enable stack smashing protection by default in the Windows GCC configuration, eliminating the need for explicit libssp linking.
445-450: Verify architecture-specific hardening flags.The configure flags
--enable-standard-branch-protection(ARM-specific, requires ARMv8.5-A+) and--enable-cet(x86-specific, Intel CET) are architecture-dependent. Ensure these flags are compatible with all target architectures that use this GCC configuration, or that the GCC configure script gracefully handles them on unsupported platforms.Run the following script to check which cross-compilation targets use linux-base-gcc:
|
wait guix build! maybe it won't succeed |
|
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6889.32ad4a7b. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6889.85d2f1f3. A new comment will be made when the image is pushed. |
|
Builds successfully for me 👍 |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6889.85d2f1f3. The image should be on dockerhub soon. |
…d683653 as e686744 b31fd98 build: stop tracking cmake dependency relic_conf.h.in (Kittywhiskers Van Gogh) e686744 Squashed 'src/dashbls/' changes from 0bb5c5b032..dd683653c6 (Kittywhiskers Van Gogh) b820266 revert: stop tracking cmake dependency relic_conf.h.in (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6889 * Expected subtree hash `bcd9c069752f3bf10e3812200ad990671dae230a34c2c0aa92e4c15032613743` (see [instructions](#6323 (review)) to calculate) ## Breaking Changes None expected. ## Checklist: - [x] I have performed a self-review of my own code **(note: N/A)** - [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 b31fd98 PastaPastaPasta: utACK b31fd98 Tree-SHA512: d9aa0ede2b1e1851456d80153fd39476a0ff53eddba603227f2ab30606ba9f07dbae578860531ec2a706d13499ea82852e2ec3da3905f09d2f9fdc46cf96246f
f95af98 guix: default ssp for Windows GCC (fanquake) 95d55b9 guix: remove ssp workaround from Windows GCC (fanquake) 8f43302 build: remove explicit libssp linking from Windows build (fanquake) Pull request description: I was expecting this to fail to compile somewhere, maybe in the CI, but that doesn't seem to be the case? Seems workable given the SSP related changes in the newer mingw-w64 headers (which are in Guix): > Implement some of the stack protector functions/variables so -lssp is now optional when _FORTIFY_SOURCE or -fstack-protector-strong is used. However I think this would still be broken in some older environments, so we might have to wait for a compiler bump, or similar. The optional -lssp also seems to work when using older headers, which doesn't make sense. Would fix bitcoin#28104. ACKs for top commit: hebasto: ACK f95af98, I've verified binaries from `bitcoin-f95af98128f1-win64.zip` on Windows 11 Pro 23H2. TheCharlatan: ACK f95af98 Tree-SHA512: 71169ec513cfe692dfa7741d2bf37b45da05627c0af1cbd50cf8c3c04cc21c4bf88f3284532bddc1e3e648391ec78dbaca5170987a13c21ac204a7bcaf27f349
knst
changed the title
thephez
left a comment
thephez
left a comment
There was a problem hiding this comment.
utACK f8d4a48
5946705118d0ac0ddca9e34bac01da04c436672141d097d8606362cdf116e021 x86_64-w64-mingw32/dashcore-23.0.0-rc.1-20-gf8d4a485bf9a-win64-debug.zip
7f0fa91b5febe0c2fa998123b3c8e2e483975e1001529b27e822463483451a18 x86_64-w64-mingw32/dashcore-23.0.0-rc.1-20-gf8d4a485bf9a-win64-setup-unsigned.exe
bdb2b7534cfe3182274cd6a34be3aea7c27e56132eaadef29b16f11f2bd934ec x86_64-w64-mingw32/dashcore-23.0.0-rc.1-20-gf8d4a485bf9a-win64-unsigned.tar.gz
65b82e22eeafde63d917330d98558b2a4372cb0d5be523dcf9889447fb98c8ec x86_64-w64-mingw32/dashcore-23.0.0-rc.1-20-gf8d4a485bf9a-win64.zip```
7 participants
Issue being fixed or feature implemented
Fixes undeterminism for windows build
What was done?
Original backport description:
f95af98 guix: default ssp for Windows GCC (fanquake)
95d55b9 guix: remove ssp workaround from Windows GCC (fanquake)
8f43302 build: remove explicit libssp linking from Windows build (fanquake)
I was expecting this to fail to compile somewhere, maybe in the CI, but that doesn't seem to be the case?
Seems workable given the SSP related changes in the newer mingw-w64 headers (which are in Guix):
However I think this would still be broken in some older environments, so we might have to wait for a compiler bump, or similar. The optional -lssp also seems to work when using older headers, which doesn't make sense.
How Has This Been Tested?
Run multiple guix build - hashes matched
Breaking Changes
n/a
Checklist: