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

achow101 commented at 4:46 pm on November 21, 2020: member

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

DrahtBot added the label Build system on Nov 21, 2020

laanwj commented at 7:51 pm on November 21, 2020: member

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

DrahtBot commented at 2:16 am on November 22, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19716 (build: Qt 5.15.x by fanquake)

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.

DrahtBot added the label Needs rebase on Nov 22, 2020

achow101 force-pushed on Nov 22, 2020

DrahtBot removed the label Needs rebase on Nov 22, 2020

hebasto commented at 9:21 pm on November 22, 2020: member

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

08560561d26d7472e3a19d56e968bb5efb5cf18cb0f7164e94c9a1a4e89f3c4ed  bitcoin-3dd3da68dc73-osx-unsigned.dmg
10769b981c93b23812d325c186d4e415ef9bab6cf13e12dc08338233011d01bad  bitcoin-3dd3da68dc73-osx-unsigned.tar.gz
20fb127663568a8a42d077b0bacb6c0e7fe278f3ed2a801960695539e2f6055ec  bitcoin-3dd3da68dc73-osx64.tar.gz
3cd73b1f50b9d56e1d68eb1ca8b6102ff6eef6bc3a23fa6734bdfe8b857261f12  src/bitcoin-3dd3da68dc73.tar.gz

achow101 commented at 10:36 pm on November 22, 2020: member

08560561d26d7472e3a19d56e968bb5efb5cf18cb0f7164e94c9a1a4e89f3c4ed  bitcoin-3dd3da68dc73-osx-unsigned.dmg
10769b981c93b23812d325c186d4e415ef9bab6cf13e12dc08338233011d01bad  bitcoin-3dd3da68dc73-osx-unsigned.tar.gz
20fb127663568a8a42d077b0bacb6c0e7fe278f3ed2a801960695539e2f6055ec  bitcoin-3dd3da68dc73-osx64.tar.gz
3cd73b1f50b9d56e1d68eb1ca8b6102ff6eef6bc3a23fa6734bdfe8b857261f12  src/bitcoin-3dd3da68dc73.tar.gz
4ecc4283a0947af073a56ac92534edd1517e23caf821ea4f75848fb714646d3be  bitcoin-core-osx-22-res.yml

in depends/patches/qt/fix_qpainter_non_determinism.patch:5 in 3dd3da68dc outdated

0@@ -0,0 +1,49 @@
1+commit c12e7cfb36b1578f37f478681533f041fb0617df
2+Author: Andrew Chow <achow101-github@achow101.com>
3+Date:   Sat Nov 21 01:11:04 2020 -0500
4+
5+    Fix non-determinism?

fanquake commented at 6:31 am on November 23, 2020:

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.

hebasto commented at 6:58 am on November 23, 2020:

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

achow101 commented at 5:08 pm on November 23, 2020:

Done.

sipa commented at 6:42 am on November 23, 2020: member

utACK 3dd3da68dc738970a3d1276a738b3d59d88510a2. 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 commented at 6:45 am on November 23, 2020: member

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

0Generating report
18560561d26d7472e3a19d56e968bb5efb5cf18cb0f7164e94c9a1a4e89f3c4ed  bitcoin-3dd3da68dc73-osx-unsigned.dmg
20769b981c93b23812d325c186d4e415ef9bab6cf13e12dc08338233011d01bad  bitcoin-3dd3da68dc73-osx-unsigned.tar.gz
30fb127663568a8a42d077b0bacb6c0e7fe278f3ed2a801960695539e2f6055ec  bitcoin-3dd3da68dc73-osx64.tar.gz
4cd73b1f50b9d56e1d68eb1ca8b6102ff6eef6bc3a23fa6734bdfe8b857261f12  src/bitcoin-3dd3da68dc73.tar.gz
5f3562e4eccbfa50e16a0394a41336f1c48d88cfb6f0b64416f776a3219a16abd  bitcoin-core-osx-22-res.yml
6Done.

Edit - macOS build also match:

0Generating report
18560561d26d7472e3a19d56e968bb5efb5cf18cb0f7164e94c9a1a4e89f3c4ed  bitcoin-3dd3da68dc73-osx-unsigned.dmg
20769b981c93b23812d325c186d4e415ef9bab6cf13e12dc08338233011d01bad  bitcoin-3dd3da68dc73-osx-unsigned.tar.gz
30fb127663568a8a42d077b0bacb6c0e7fe278f3ed2a801960695539e2f6055ec  bitcoin-3dd3da68dc73-osx64.tar.gz
4cd73b1f50b9d56e1d68eb1ca8b6102ff6eef6bc3a23fa6734bdfe8b857261f12  src/bitcoin-3dd3da68dc73.tar.gz
5d0304b68bf17a47328532b0fd3676a3f967f19a494c96f1bd8302dcfe274269f  bitcoin-core-osx-22-res.yml
6Done.

fanquake added the label Needs backport (0.21) on Nov 23, 2020

MarcoFalke added this to the milestone 0.21.0 on Nov 23, 2020

hebasto approved

hebasto commented at 6:53 am on November 23, 2020: member

ACK 3dd3da68dc738970a3d1276a738b3d59d88510a2, 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 commented at 7:33 am on November 23, 2020: member

Code review ACK 3dd3da68dc738970a3d1276a738b3d59d88510a2

practicalswift commented at 10:42 am on November 23, 2020: contributor

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

Fix QPainter non-determinism on macOS

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.

8f7d1b39ef

achow101 force-pushed on Nov 23, 2020

hebasto approved

hebasto commented at 5:22 pm on November 23, 2020: member

re-ACK 8f7d1b39efbe65ab2747c593cc3560d4a449a333 ~~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

MarcoFalke added the label Needs gitian build on Nov 23, 2020

MarcoFalke added the label Needs Guix build on Nov 23, 2020

MarcoFalke removed the label Needs Guix build on Nov 23, 2020

MarcoFalke added the label Needs Guix build on Nov 23, 2020

fanquake approved

fanquake commented at 1:11 pm on November 24, 2020: member

ACK 8f7d1b39efbe65ab2747c593cc3560d4a449a333

fanquake merged this on Nov 24, 2020

fanquake closed this on Nov 24, 2020

laanwj commented at 2:15 pm on November 24, 2020: member

~~@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 referenced this in commit b5fbf10b7d on Nov 24, 2020

laanwj referenced this in commit ab23a83400 on Nov 24, 2020

fanquake referenced this in commit 17294c1820 on Nov 25, 2020

MarcoFalke commented at 12:44 pm on November 25, 2020: member

Backported in #20479

MarcoFalke removed the label Needs Guix build on Nov 25, 2020

MarcoFalke removed the label Needs backport (0.21) on Nov 25, 2020

MarcoFalke removed the label Needs gitian build on Nov 25, 2020

furszy referenced this in commit 816f42d7ac on May 25, 2021

kittywhiskers referenced this in commit 11d221b776 on Nov 24, 2021

kittywhiskers referenced this in commit ca3f7f7b84 on Nov 30, 2021

PastaPastaPasta referenced this in commit 25a965d691 on Nov 30, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts