Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

depends: Patch qt_intersect_spans to avoid non-deterministic behavior in LLVM 8 #20447

Merged
merged 1 commit into from
Nov 24, 2020
Merged

depends: Patch qt_intersect_spans to avoid non-deterministic behavior in LLVM 8 #20447

merged 1 commit into from
Nov 24, 2020

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Nov 21, 2020 •
edited by fanquake
Loading

Applies a patch to Qt that fixes the non-determinism by modifying Qt. The source of the non-determinism is how LLVM 8 optimizes qt_intersect_spans when compiling. The particular optimization that seems to be causing the problems is that a temp variable is being added for spans->y. For some reason, when it does this, it chooses different instructions to use when making that variable. We bypass this problem by patching qt_intersect_spans to always make and use this local variable.

Potential alternative to #20436 and #20440

@laanwj
Copy link
Member

laanwj commented Nov 21, 2020 •
edited
Loading

No opinion on which of the alternatives to choose, but the Qt code change looks fairly trivially correct.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 22, 2020 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Nov 22, 2020
Closed
Merged
hebasto
hebasto reviewed Nov 22, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing 3dd3da6, gitian macOS build (was done 3 times with a clear cache):

8560561d26d7472e3a19d56e968bb5efb5cf18cb0f7164e94c9a1a4e89f3c4ed  bitcoin-3dd3da68dc73-osx-unsigned.dmg
0769b981c93b23812d325c186d4e415ef9bab6cf13e12dc08338233011d01bad  bitcoin-3dd3da68dc73-osx-unsigned.tar.gz
0fb127663568a8a42d077b0bacb6c0e7fe278f3ed2a801960695539e2f6055ec  bitcoin-3dd3da68dc73-osx64.tar.gz
cd73b1f50b9d56e1d68eb1ca8b6102ff6eef6bc3a23fa6734bdfe8b857261f12  src/bitcoin-3dd3da68dc73.tar.gz

@achow101
Copy link
Member Author 

8560561d26d7472e3a19d56e968bb5efb5cf18cb0f7164e94c9a1a4e89f3c4ed  bitcoin-3dd3da68dc73-osx-unsigned.dmg
0769b981c93b23812d325c186d4e415ef9bab6cf13e12dc08338233011d01bad  bitcoin-3dd3da68dc73-osx-unsigned.tar.gz
0fb127663568a8a42d077b0bacb6c0e7fe278f3ed2a801960695539e2f6055ec  bitcoin-3dd3da68dc73-osx64.tar.gz
cd73b1f50b9d56e1d68eb1ca8b6102ff6eef6bc3a23fa6734bdfe8b857261f12  src/bitcoin-3dd3da68dc73.tar.gz
ecc4283a0947af073a56ac92534edd1517e23caf821ea4f75848fb714646d3be  bitcoin-core-osx-22-res.yml

@hebasto hebasto mentioned this pull request Nov 22, 2020
Closed
@sipa
Copy link
Member

sipa commented Nov 23, 2020 •
edited
Loading

utACK 3dd3da6. If this fixes the problem, it seems like the simplest solution that doesn't require messing with the build system or changing compilers at the last minute.

@fanquake fanquake mentioned this pull request Nov 23, 2020
Closed
fanquake
fanquake reviewed Nov 23, 2020
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. I've gitian built on my Linux box, and see matching hashes. Just going to build on macOS as well.

Generating report
8560561d26d7472e3a19d56e968bb5efb5cf18cb0f7164e94c9a1a4e89f3c4ed  bitcoin-3dd3da68dc73-osx-unsigned.dmg
0769b981c93b23812d325c186d4e415ef9bab6cf13e12dc08338233011d01bad  bitcoin-3dd3da68dc73-osx-unsigned.tar.gz
0fb127663568a8a42d077b0bacb6c0e7fe278f3ed2a801960695539e2f6055ec  bitcoin-3dd3da68dc73-osx64.tar.gz
cd73b1f50b9d56e1d68eb1ca8b6102ff6eef6bc3a23fa6734bdfe8b857261f12  src/bitcoin-3dd3da68dc73.tar.gz
f3562e4eccbfa50e16a0394a41336f1c48d88cfb6f0b64416f776a3219a16abd  bitcoin-core-osx-22-res.yml
Done.

Edit - macOS build also match:

Generating report
8560561d26d7472e3a19d56e968bb5efb5cf18cb0f7164e94c9a1a4e89f3c4ed  bitcoin-3dd3da68dc73-osx-unsigned.dmg
0769b981c93b23812d325c186d4e415ef9bab6cf13e12dc08338233011d01bad  bitcoin-3dd3da68dc73-osx-unsigned.tar.gz
0fb127663568a8a42d077b0bacb6c0e7fe278f3ed2a801960695539e2f6055ec  bitcoin-3dd3da68dc73-osx64.tar.gz
cd73b1f50b9d56e1d68eb1ca8b6102ff6eef6bc3a23fa6734bdfe8b857261f12  src/bitcoin-3dd3da68dc73.tar.gz
d0304b68bf17a47328532b0fd3676a3f967f19a494c96f1bd8302dcfe274269f  bitcoin-core-osx-22-res.yml
Done.

depends/patches/qt/fix_qpainter_non_determinism.patch Outdated
Author: Andrew Chow <achow101-github@achow101.com>
Date: Sat Nov 21 01:11:04 2020 -0500

Fix non-determinism?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the relevant explanations to your patch file. Feel free to take any wording from mine. Also mention when the patch can be dropped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch file could be applied for the 0.21 branch only, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@maflcko maflcko added this to the 0.21.0 milestone Nov 23, 2020
hebasto
hebasto approved these changes Nov 23, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3dd3da6, I did not observe any non-determinism during gitian builds, C++ changes are trivial, I agree this PR can be merged into the 0.21 branch for rc2.

For the master branch the #20454 seems more appropriate.

jonatack
jonatack reviewed Nov 23, 2020
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 3dd3da6

@practicalswift
Copy link
Contributor

Concept ACK: this fix seems less intrusive than the alternatives :)

@DrahtBot DrahtBot mentioned this pull request Nov 23, 2020
Closed
@achow101
Fix QPainter non-determinism on macOS
8f7d1b3 
Aplies a patch to Qt that fixes the non-determinism by modifying Qt. The
source of the non-determinism is how LLVM 8 optimizes qt_intersect_spans
when compiling. The particular optimization that seems to be causing the
problems is that a temp variable is being added for spans->y. For some
reason, when it does this, it chooses different instructions to use when
making that variable. We bypass this problem by patching
qt_intersect_spans to always make and use this local variable.
hebasto
hebasto approved these changes Nov 23, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 8f7d1b3 for merging into the 0.21 branch, but not into the master branch.
Only the patch description updated since my previous review.

EDIT: agree with @achow101 on IRC:

i think it's reasonable to merge into master and then revert when clang 9 is merged

fanquake
fanquake approved these changes Nov 24, 2020
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8f7d1b3

@fanquake fanquake merged commit 31c9987 into bitcoin:master Nov 24, 2020
@laanwj
Copy link
Member

laanwj commented Nov 24, 2020 •
edited
Loading

@achow101 I tried to cherry-pick this to 0.21 branch but it's not quite a clean backport. Many conflicts in depends/packages/qt.mk. Mind opening a PR for the 0.21 branch as well?

Edit: no idea if I got the order right but see #20479.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 24, 2020
@fanquake
Merge bitcoin#20447: depends: Patch qt_intersect_spans to avoid non-d…
b5fbf10 
…eterministic behavior in LLVM 8

8f7d1b3 Fix QPainter non-determinism on macOS (Andrew Chow)

Pull request description:

  Aplies a patch to Qt that fixes the non-determinism by modifying Qt. The source of the non-determinism is how LLVM 8 optimizes qt_intersect_spans when compiling. The particular optimization that seems to be causing the problems is that a temp variable is being added for spans->y. For some reason, when it does this, it chooses different instructions to use when making that variable. We bypass this problem by patching qt_intersect_spans to always make and use this local variable.

  Potential alternative to bitcoin#20436 and bitcoin#20440

ACKs for top commit:
  hebasto:
    re-ACK 8f7d1b3 ~for merging into the 0.21 branch, but [not into the master](bitcoin#20454) branch.~
  fanquake:
    ACK 8f7d1b3

Tree-SHA512: b0d00a77643554021736524fb64611462ef2ec849a220543c12d99edb0f52f2e8128d2cc61fa82176b7e13b294574774a92d6b649badf8b7630c6d6a7e70ce10
laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Nov 24, 2020
@achow101 @laanwj
Fix QPainter non-determinism on macOS
ab23a83 
Aplies a patch to Qt that fixes the non-determinism by modifying Qt. The
source of the non-determinism is how LLVM 8 optimizes qt_intersect_spans
when compiling. The particular optimization that seems to be causing the
problems is that a temp variable is being added for spans->y. For some
reason, when it does this, it chooses different instructions to use when
making that variable. We bypass this problem by patching
qt_intersect_spans to always make and use this local variable.

Github-Pull: bitcoin#20447
Rebased-From: 8f7d1b3
Tree-SHA512: 558da5c2bb0373e2a89f2c219170f802036e0e87cc8e808336b23d074152cb893007a440f46ec957156b0921355cd18502710f2d224f27bc26e934c50ebebc41
@laanwj laanwj mentioned this pull request Nov 24, 2020
Merged
fanquake added a commit that referenced this pull request Nov 25, 2020
@fanquake
Merge #20479: Fix QPainter non-determinism on macOS (0.21 backport)
17294c1 
ab23a83 Fix QPainter non-determinism on macOS (Andrew Chow)

Pull request description:

  Aplies a patch to Qt that fixes the non-determinism by modifying Qt. The
  source of the non-determinism is how LLVM 8 optimizes qt_intersect_spans
  when compiling. The particular optimization that seems to be causing the
  problems is that a temp variable is being added for spans->y. For some
  reason, when it does this, it chooses different instructions to use when
  making that variable. We bypass this problem by patching
  qt_intersect_spans to always make and use this local variable.

  Github-Pull: #20447
  Rebased-From: 8f7d1b3
  Tree-SHA512: 558da5c2bb0373e2a89f2c219170f802036e0e87cc8e808336b23d074152cb893007a440f46ec957156b0921355cd18502710f2d224f27bc26e934c50ebebc41

ACKs for top commit:
  jonasschnelli:
    codereview ACK ab23a83
  achow101:
    ACK ab23a83

Tree-SHA512: 10991fe2b5452b1393678c315281cfdca011e9bb2cd8094a002746e690890ace148ac2dbf39c5fbe5e7f4cd39eeebfa0a715c065cff150cf70e9733cb0ff32d6
@maflcko
Copy link
Member

maflcko commented Nov 25, 2020

Backported in #20479

@fanquake fanquake mentioned this pull request Nov 30, 2020
Closed
@Fuzzbawls Fuzzbawls mentioned this pull request Mar 27, 2021
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request May 25, 2021
@furszy
Merge #2272: [Depends] Update macOS cross-compiling toolchain
816f42d 
e1b89ac Fix QPainter non-determinism on macOS (Andrew Chow)
831c317 macOS deploy: use the new plistlib API (Jonas Schnelli)
5857aaf doc: Document ALLOW_HOST_PACKAGES dependency option (skmcontrib)
2329e08 build: Fix behavior when ALLOW_HOST_PACKAGES unset (Hennadii Stepanov)
1768870 depends: native_ds_store 1.3.0 (fanquake)
3f9f3e5 depends: pull upstream libdmg-hfsplus changes (fanquake)
f7606dc depends: latest config.guess & config.sub (fanquake)
cc3ae74 depends: bump native_cctools for fixed lto with external clang (Cory Fields)
b26c648 depends: enable lto support for Apple's ld64 (Cory Fields)
50933d7 depends: Add documentation for FORCE_USE_SYSTEM_CLANG make flag (Carl Dong)
ba3ddf2 depends: Reformat make options as definition list (Carl Dong)
3b855a7 depends: Add justifications for macOS clang flags (Carl Dong)
4104de0 depends: specify libc++ header location for darwin (Cory Fields)
cd4335f depends: force a new host id string if FORCE_USE_SYSTEM_CLANG is in use (Cory Fields)
d30e1af depends: Allow building with system clang (Carl Dong)
234828b depends: Decouple toolchain + binutils (Carl Dong)
1dd3a5a doc: explain why passing -mlinker-version is required (fanquake)
5cc0d0f darwin: pass mlinker-version so that clang enables new features (Cory Fields)
813a552 macos: Bump to xcode 11.3.1 and 10.15 SDK (Cory Fields)
ee7085f depends: bump MacOS toolchain (Cory Fields)
e5b092b contrib: macdeploy: Remove historical extraction notes (Carl Dong)
5893caf contrib: macdeploy: Use apple-sdk-tools instead of xar+pbzx (Carl Dong)
9f2d4ba native_cctools: Don't use libc++ from pinned clang (Carl Dong)
0c8d217 Adapt rest of tooling to new SDK naming scheme (Carl Dong)
bdacfa8 contrib: macdeploy: Correctly generate macOS SDK (Carl Dong)
f7eee2c Fix naming of macOS SDK and clarify version (Andrew Chow)
62f9e23 build: use macOS 10.14 SDK (fanquake)
bc2e1af depends: native_cctools 921, ld64 409.12, libtapi 1000.10.8 (fanquake)
a296d87 depends: clang 6.0.1 (fanquake)
8f6c475 build: Set minimum supported macOS to 10.12 (Fuzzbawls)

Pull request description:

  This backports the following upstream PRs to update the macOS cross-compiling tools:

  bitcoin#17550
  bitcoin#16392
  bitcoin#18589
  bitcoin#19240
  bitcoin#19407
  bitcoin#17919
  bitcoin#19530
  bitcoin#17057
  bitcoin#20333
  bitcoin#18051
  bitcoin#19124
  bitcoin#20298
  bitcoin#20447

  The tools being updated are

  ### Clang
  Upgraded from `3.7.1` to `8.0.0`

  ### cctools

  * cctools `877.8` -> `949.0.1`
  * LD64 `253.9` -> `530`
  * TAPI `1000.10.8`

  ### DSStore
  Upgraded from `1.1.2` to `1.3.0` (this removes the biplist dependency)

  This also effectively bumps our minimum supported macOS version to 10.12 (Sierra).

ACKs for top commit:
  furszy:
    tested ACK e1b89ac
  random-zebra:
    utACK e1b89ac

Tree-SHA512: f5cec8db57e07d8855070646b9e1400d48aac1d01e3c2c3b3e134665c6372d6535f3328888bb9a75087f7b3d5231ecb4b509723bfa51bd40770ffe2810c67f65
@lyricidal lyricidal mentioned this pull request Oct 31, 2021
Merged
kwvg added a commit to kwvg/dash that referenced this pull request Nov 24, 2021
@kwvg
merge bitcoin#20447: Patch qt_intersect_spans to avoid non-determinis…
11d221b 
…tic behavior in LLVM 8
kwvg added a commit to kwvg/dash that referenced this pull request Nov 30, 2021
@kwvg
merge bitcoin#20447: Patch qt_intersect_spans to avoid non-determinis…
ca3f7f7 
…tic behavior in LLVM 8
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Nov 30, 2021
@PastaPastaPasta
Merge pull request #4582 from kittywhiskers/openssl_bump
25a965d 
merge bitcoin#16110, bitcoin#16413, bitcoin#16837, bitcoin#17466, bitcoin#19959, bitcoin#19867, bitcoin#20447, bitcoin#21363, partial bitcoin#17730: bump qt
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonatack jonatack jonatack left review comments

@fanquake fanquake fanquake approved these changes

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.
9 participants
@achow101 @laanwj @DrahtBot @sipa @practicalswift @maflcko @fanquake @jonatack @hebasto