Fix: OSX QT compile: use built-in swap if available, or defer #9366

kallewoof · 2016-12-16T12:25:31Z

Uses built-in byte swap if available (Apple) and if bswap_XX is undefined.

Defers to pre-defined version if found (e.g. protobuf). For protobuf case, the definitions are identical and thus include order should not affect results.

Fixes compilation errors on recent Mac OS X with QT client enabled.

Replaces #9361

laanwj · 2016-12-16T16:19:51Z

Yes, this looks better, thanks

laanwj · 2016-12-16T16:20:41Z

src/test/bswap_protobuf_tests.cpp

I'd prefer moving this one to the qt unit tests, instead of adding a qt-specific unit test to the core unit tests.

I moved this into a new "CompatTests" test suite in the QT tests.

theuni · 2016-12-16T19:08:16Z

Hmm, is it really necessary to have the system-specific bswaps anymore? Since we require a modern compiler, these are likely to be detected/optimized automatically anyway.

Test file: https://gist.github.com/theuni/deb0b97c31890bd0a06a65dd3cb51822
compiled with gcc/clang: c++ -std=c++11 -O2 -c -o bswapcomp.o bswap.cpp

Results: https://gist.github.com/theuni/508b69da908be1e52bbdd0b7a135c318

On x86-64, I see no difference for osx+clang, linux+clang, linux+gcc.

doesn't look like it's worth bothering to me, or is there some platform that would be more likely to be a discrepancy?

kallewoof · 2016-12-16T23:17:20Z

@theuni So you think it's better to basically #undef and #define them always for consistency?

Edit: I meant to say that since we want the same thing to happen no matter which include order is taken, we might as well just redefine them each time on our own side.

theuni · 2016-12-17T00:13:20Z

@kallewoof I was suggesting just skipping the platform includes (byteswap.h/OSByteOrder.h), skipping the use of any system implementations, and renaming bswap_foo -> int_bswap_foo to avoid collisions.

Need others to chime in first though.

kallewoof · 2016-12-17T01:15:41Z

FWIW I like the idea personally.

…ined. Defers to pre-defined version if found (e.g. protobuf). For protobuf case, the definitions are identical and thus include order should not affect results.

theuni · 2016-12-17T08:10:02Z

utACK 815f414 for the sake of fixing the build.

Dropping the system includes (or alternatively autoconfing this) can come as a next step if it's agreed upon.

laanwj · 2016-12-19T07:27:59Z

Hmm, is it really necessary to have the system-specific bswaps anymore? Since we require a modern compiler, these are likely to be detected/optimized automatically anyway.

If there are bswap primitives, I think we should use them. This is used internally in the serialization as well as crypto operations, so performance here (if there is a compiler where it makes sense) does matter.
If you want to prove for every compiler, architecture combo in existence that this doesn't matter, fine. But if not, I'd prefer to keep using system primitives if available.

laanwj · 2016-12-19T07:29:30Z

utACK 815f414, let's just fix this to fix the build, I think we've been through enough shed-painting here.

…defer 815f414 Uses built-in byte swap if available (Apple) and if bswap_XX is undefined. (Karl-Johan Alm)

droark · 2017-02-01T00:23:38Z

FYI, I backported this to 0.13. I noticed it when trying to compile the Elements Alpha refresh.

…ined. Defers to pre-defined version if found (e.g. protobuf). For protobuf case, the definitions are identical and thus include order should not affect results. Github-Pull: #9366 Rebased-From: 815f414

…defer 815f414 Uses built-in byte swap if available (Apple) and if bswap_XX is undefined. (Karl-Johan Alm)

Fix macosx build (backport core #9169 and #9366)

This was originally added in bitcoin#9366 to fix the gui build, as Protobuf would also define these macros. Now that we're no-longer using Protobuf, remove the additional check.

-BEGIN VERIFY SCRIPT- sed -i 's/__APPLE__/MAC_OSX/g' src/compat/byteswap.h src/util.cpp -END VERIFY SCRIPT- Zcash: Excludes byteswap.h change as we don't have bitcoin/bitcoin#9366.

This was originally added in bitcoin#9366 to fix the gui build, as Protobuf would also define these macros. Now that we're no-longer using Protobuf, remove the additional check.

These were added as part of bitcoin#9366 to fix issues with Protobuf. Now that we no-longer use Protobuf, there's no reason to maintain a duplicate set of byteswap tests for qt.

9ac86bc test: remove qt byteswap compattests (fanquake) Pull request description: These were added as part of bitcoin#9366 when with fixing issues with Protobuf. Now that we no-longer use Protobuf, there's no reason to maintain a duplicate set of byteswap tests in the qt tests. Our other set of byteswap tests are here: https://github.com/bitcoin/bitcoin/blob/master/src/test/bswap_tests.cpp. ACKs for top commit: laanwj: Code review ACK 9ac86bc Tree-SHA512: 72ba131a5f8fbd9fdbbc4e1f95baa794496c960b12e0271700c632c6511b7e1b331e8db07a201838b4d56b2aeeb43d4de4e10265ea07ab14241307fa14d3342e

This was originally added in bitcoin#9366 to fix the gui build, as Protobuf would also define these macros. Now that we're no-longer using Protobuf, remove the additional check.

fanquake added the macOS label Dec 16, 2016

laanwj reviewed Dec 16, 2016

View reviewed changes

kallewoof force-pushed the macosx-qt-build-fix2 branch 2 times, most recently from 1f8c454 to 609eb65 Compare December 17, 2016 01:29

Uses built-in byte swap if available (Apple) and if bswap_XX is undef…

815f414

…ined. Defers to pre-defined version if found (e.g. protobuf). For protobuf case, the definitions are identical and thus include order should not affect results.

kallewoof force-pushed the macosx-qt-build-fix2 branch from ef059d8 to 815f414 Compare December 17, 2016 03:27

kallewoof mentioned this pull request Dec 17, 2016

Fix: OSX QT compile: move bswap_XX into Bitcoin:: to avoid collision with existing definitions #9372

Closed

laanwj merged commit 815f414 into bitcoin:master Dec 19, 2016

laanwj added a commit that referenced this pull request Dec 19, 2016

Merge #9366: Fix: OSX QT compile: use built-in swap if available, or …

79da397

…defer 815f414 Uses built-in byte swap if available (Apple) and if bswap_XX is undefined. (Karl-Johan Alm)

kallewoof deleted the macosx-qt-build-fix2 branch December 19, 2016 07:30

fanquake mentioned this pull request Jan 4, 2017

[Trivial] [Doc] Install Protobuf v3 on OS X #9467

Merged

fanquake mentioned this pull request Jan 12, 2017

Protocol Buffers v3.0.0 has been released #8430

Closed

droark mentioned this pull request Jan 31, 2017

Add Changes From Liquid, Bitcoin ElementsProject/elements#107

Merged

QuantumExplorer mentioned this pull request Feb 28, 2017

fixed protobuf 3.0 (byteswap) issue on macOS dashpay/dash#1370

Merged

sickpig referenced this pull request in sickpig/BitcoinUnlimited Feb 28, 2017

Merge #9366: Fix: OSX QT compile: use built-in swap if available, or …

cad55d5

…defer 815f414 Uses built-in byte swap if available (Apple) and if bswap_XX is undefined. (Karl-Johan Alm)

This was referenced Feb 28, 2017

Fix macosx build (backport core #9169 and #9366) BitcoinUnlimited/BitcoinUnlimited#323

Merged

Get build working on Mac BitcoinUnlimited/BitcoinUnlimited#322

Closed

gandrewstone referenced this pull request in BitcoinUnlimited/BitcoinUnlimited Mar 1, 2017

Merge pull request #323 from sickpig/fix/mac_build

1d56e4c

Fix macosx build (backport core #9169 and #9366)

kyuupichan mentioned this pull request Apr 22, 2017

[port] Fix classic build on Mac bitcoinclassic/bitcoinclassic#256

Closed

fanquake mentioned this pull request Mar 26, 2021

test: remove qt byteswap compattests #21531

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix: OSX QT compile: use built-in swap if available, or defer #9366

Fix: OSX QT compile: use built-in swap if available, or defer #9366

Uh oh!

kallewoof commented Dec 16, 2016 •

edited

Loading

Uh oh!

laanwj commented Dec 16, 2016

Uh oh!

laanwj Dec 16, 2016

Uh oh!

kallewoof Dec 17, 2016

Uh oh!

theuni commented Dec 16, 2016

Uh oh!

kallewoof commented Dec 16, 2016 •

edited

Loading

Uh oh!

theuni commented Dec 17, 2016

Uh oh!

kallewoof commented Dec 17, 2016

Uh oh!

theuni commented Dec 17, 2016

Uh oh!

laanwj commented Dec 19, 2016 •

edited

Loading

Uh oh!

laanwj commented Dec 19, 2016

Uh oh!

droark commented Feb 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix: OSX QT compile: use built-in swap if available, or defer #9366

Fix: OSX QT compile: use built-in swap if available, or defer #9366

Uh oh!

Conversation

kallewoof commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Dec 16, 2016

Uh oh!

laanwj Dec 16, 2016

Choose a reason for hiding this comment

Uh oh!

kallewoof Dec 17, 2016

Choose a reason for hiding this comment

Uh oh!

theuni commented Dec 16, 2016

Uh oh!

kallewoof commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Dec 17, 2016

Uh oh!

kallewoof commented Dec 17, 2016

Uh oh!

theuni commented Dec 17, 2016

Uh oh!

laanwj commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Dec 19, 2016

Uh oh!

droark commented Feb 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kallewoof commented Dec 16, 2016 •

edited

Loading

kallewoof commented Dec 16, 2016 •

edited

Loading

laanwj commented Dec 19, 2016 •

edited

Loading