Document FreeBSD quirk. Fix FreeBSD build: Use std::min<int>(...) to allow for compilation under certain FreeBSD versions. by practicalswift · Pull Request #13503 · bitcoin/bitcoin

practicalswift · 2018-06-19T13:48:48Z

Document FreeBSD quirk.
Fix FreeBSD build: Cast to int to allow std::min to work under FreeBSD.

fanquake · 2018-06-19T14:14:14Z

So this was originally applied in #2695:

When compiling on FreeBSD, the calculation here returns an unsigned int. This causes a compile-time error with std::min, which cannot compare signed with unsigned integers.
This pull inserts an explicit cast, treating the calculated value as signed, keeping the compiler happy.

Then reverted in #9598 to:

Improve readability by removing redundant casts to same type (on all platforms)

Now we're re-adding (int) again but only for FreeBSD.

I've just built master (3f398d7) successfully on a FreeBSD 11.1 VM, so I'm curious to know which version of FreeBSD this is broken on?

Regardless of the above, the current patch doesn't compile:

fvisibility=hidden -c -o libbitcoin_server_a-init.o `test -f 'init.cpp' || echo './'`init.cpp
init.cpp:969:2: error: invalid preprocessing directive #fi
 #fi
  ^~
init.cpp:964:0: error: unterminated #else
 #if defined(__FreeBSD__) || defined(__DragonFly__)
 
make[2]: *** [libbitcoin_server_a-init.o] Error 1
Makefile:5817: recipe for target 'libbitcoin_server_a-init.o' failed

src/init.cpp

practicalswift · 2018-06-19T14:20:16Z

@fanquake @ken2812221 Sorry about the bashism :-) Not fixed!

practicalswift · 2018-06-19T14:23:01Z

@fanquake Good question regarding FreeBSD version. @anton48, what FreeBSD version are you running?

practicalswift · 2018-06-19T14:25:16Z

If this is fixed in FreeBSD 11.1 I'm not sure we have a problem to solve :-)

practicalswift · 2018-06-19T14:28:01Z

Closing this temporarily while waiting for input from @anton48.

If we can reproduce under FreeBSD 11.1 I'll re-open.

anton48 · 2018-06-19T20:42:59Z

@practicalswift @fanquake the source of the problem is not a version of FreeBSD itself, but version of clang. currently there are two production versions of FreeBSD: 11.1 and 10.4. former contains clang 4, latter clang 3.4. "new" code in init.cpp can be compiled with clang 4, but not with 3.4.

@practicalswift your patch with "if defined(FreeBSD)" works for me at 10.3, 10.4 and 11.1.

fanquake · 2018-06-21T01:39:55Z

Thanks for following up @anton48.

Not sure if we want to add a FreeBSD specific "work around", or if we should just revert the init.cpp change from #9598.

maflcko · 2018-06-21T01:54:25Z

Agree with @fanquake.

Having different code for each platform makes testing a nightmare. Btw. I believe you can specify the function template you want to call with std::min<type>(a,b)

practicalswift · 2018-06-21T07:33:39Z

@fanquake @MarcoFalke Thanks for reviewing. Feedback addressed. Please re-review :-)

@anton48 Can you confirm that the updated version works as expected for you?

anton48 · 2018-06-21T07:55:22Z

@practicalswift by updated version you mean nMaxConnections = std::max(std::min<int>(nMaxConnections, FD_SETSIZE - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS), 0);? yes, it can be compiled without errors on FreeBSD (tested on 10 and 11 versions).

practicalswift · 2018-06-21T08:13:15Z

@anton48 Yes, that's the new version. Thanks for confirming.

laanwj · 2018-06-24T14:05:16Z

I'm confused. I build bitcoin core on FreeBSD all the time and have never needed this.
#2695 is ancient.

maflcko · 2018-06-24T14:18:59Z

The cast to int was only recently removed in master and I believe it only fails on FreeBSD 10, not 11.

maflcko · 2018-06-24T21:18:23Z

utACK 629a47a

sipa · 2018-06-27T01:03:53Z

utACK 629a47a

fanquake · 2018-06-27T08:12:49Z

utACK 629a47a for fixing the FreeBSD 10.x build.
As noted above, the issue comes from Clang 3.4

…<int>(...) to allow for compilation under certain FreeBSD versions. 629a47a Document FreeBSD quirk. Fix FreeBSD build. (practicalswift) Pull request description: * Document FreeBSD quirk. * Fix FreeBSD build: Cast to `int` to allow `std::min` to work under FreeBSD. Context: #9598 (comment) Tree-SHA512: 5ca7a5fa9e1f3efae241b9be64c9b019ec713c11dcc3edaaed383477ea48ac0dc82549ffebbe9069e8c3f6eff30acd6e4542b4aa31d307f022f4f51e5851a82c

…td::min<int>(...) to allow for compilation under certain FreeBSD versions. 629a47a Document FreeBSD quirk. Fix FreeBSD build. (practicalswift) Pull request description: * Document FreeBSD quirk. * Fix FreeBSD build: Cast to `int` to allow `std::min` to work under FreeBSD. Context: bitcoin#9598 (comment) Tree-SHA512: 5ca7a5fa9e1f3efae241b9be64c9b019ec713c11dcc3edaaed383477ea48ac0dc82549ffebbe9069e8c3f6eff30acd6e4542b4aa31d307f022f4f51e5851a82c

…<int>(...) to allow for compilation under certain FreeBSD versions. Summary: 629a47a Document FreeBSD quirk. Fix FreeBSD build. (practicalswift) Pull request description: * Document FreeBSD quirk. * Fix FreeBSD build: Cast to `int` to allow `std::min` to work under FreeBSD. Context: bitcoin/bitcoin#9598 (comment) Tree-SHA512: 5ca7a5fa9e1f3efae241b9be64c9b019ec713c11dcc3edaaed383477ea48ac0dc82549ffebbe9069e8c3f6eff30acd6e4542b4aa31d307f022f4f51e5851a82c Backport of Core [[bitcoin/bitcoin#13503 | PR13503]] Test Plan: ninja ninja check-all Repeat with FreeBSD build. Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6481

…td::min<int>(...) to allow for compilation under certain FreeBSD versions. 629a47a Document FreeBSD quirk. Fix FreeBSD build. (practicalswift) Pull request description: * Document FreeBSD quirk. * Fix FreeBSD build: Cast to `int` to allow `std::min` to work under FreeBSD. Context: bitcoin#9598 (comment) Tree-SHA512: 5ca7a5fa9e1f3efae241b9be64c9b019ec713c11dcc3edaaed383477ea48ac0dc82549ffebbe9069e8c3f6eff30acd6e4542b4aa31d307f022f4f51e5851a82c

ee7891a doc: Remove outdated comments (Hennadii Stepanov) Pull request description: The first removed comment was introduced in #5288, the second one in #13503. Both are outdated since #14336. ACKs for top commit: duncandean: crACK ee7891a Tree-SHA512: a2d6071919e81c916bfc2178109bbc464417321bcc567ed0644448c5faea8e58cb08a7657afa1b6ffe1fb63e114a2a47b31c893e471839ba9d49a3986e68b2a7

ee7891a doc: Remove outdated comments (Hennadii Stepanov) Pull request description: The first removed comment was introduced in bitcoin#5288, the second one in bitcoin#13503. Both are outdated since bitcoin#14336. ACKs for top commit: duncandean: crACK ee7891a Tree-SHA512: a2d6071919e81c916bfc2178109bbc464417321bcc567ed0644448c5faea8e58cb08a7657afa1b6ffe1fb63e114a2a47b31c893e471839ba9d49a3986e68b2a7

…td::min<int>(...) to allow for compilation under certain FreeBSD versions. 629a47a Document FreeBSD quirk. Fix FreeBSD build. (practicalswift) Pull request description: * Document FreeBSD quirk. * Fix FreeBSD build: Cast to `int` to allow `std::min` to work under FreeBSD. Context: bitcoin#9598 (comment) Tree-SHA512: 5ca7a5fa9e1f3efae241b9be64c9b019ec713c11dcc3edaaed383477ea48ac0dc82549ffebbe9069e8c3f6eff30acd6e4542b4aa31d307f022f4f51e5851a82c

fanquake added the Linux/Unix label Jun 19, 2018

practicalswift mentioned this pull request Jun 19, 2018

Improve readability by removing redundant casts to same type (on all platforms) #9598

Merged

ken2812221 reviewed Jun 19, 2018

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the document-freebsd-quirk branch from d0f7f6b to 2944db6 Compare June 19, 2018 14:19

practicalswift closed this Jun 19, 2018

fanquake reopened this Jun 21, 2018

practicalswift force-pushed the document-freebsd-quirk branch from 2944db6 to 74a6179 Compare June 21, 2018 07:26

Document FreeBSD quirk. Fix FreeBSD build.

629a47a

practicalswift force-pushed the document-freebsd-quirk branch from 74a6179 to 629a47a Compare June 21, 2018 07:27

practicalswift changed the title ~~Document FreeBSD quirk. Fix FreeBSD build: Cast to int to allow std::min to work under FreeBSD.~~ Document FreeBSD quirk. Fix FreeBSD build: Use std::min<int>(...) allow compilation under certain FreeBSD versions. Jun 21, 2018

practicalswift changed the title ~~Document FreeBSD quirk. Fix FreeBSD build: Use std::min<int>(...) allow compilation under certain FreeBSD versions.~~ Document FreeBSD quirk. Fix FreeBSD build: Use std::min<int>(...) to allow for compilation under certain FreeBSD versions. Jun 21, 2018

maflcko merged commit 629a47a into bitcoin:master Jun 27, 2018

Bushstar mentioned this pull request Jun 28, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#350

Merged

practicalswift mentioned this pull request Feb 4, 2019

ci: Build and run tests once on freebsd #15338

Merged

codablock mentioned this pull request Apr 7, 2020

Backport poll() code from Bitcoin dashpay/dash#3396

Merged

practicalswift deleted the document-freebsd-quirk branch April 10, 2021 19:35

hebasto mentioned this pull request Sep 25, 2021

doc: Remove outdated comments #23094

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

practicalswift commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jun 19, 2018

Uh oh!

practicalswift commented Jun 19, 2018

Uh oh!

practicalswift commented Jun 19, 2018

Uh oh!

practicalswift commented Jun 19, 2018

Uh oh!

anton48 commented Jun 19, 2018

Uh oh!

fanquake commented Jun 21, 2018

Uh oh!

maflcko commented Jun 21, 2018

Uh oh!

practicalswift commented Jun 21, 2018

Uh oh!

anton48 commented Jun 21, 2018

Uh oh!

practicalswift commented Jun 21, 2018

Uh oh!

laanwj commented Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 24, 2018

Uh oh!

maflcko commented Jun 24, 2018

Uh oh!

sipa commented Jun 27, 2018

Uh oh!

fanquake commented Jun 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

practicalswift commented Jun 19, 2018 •

edited

Loading

fanquake commented Jun 19, 2018 •

edited

Loading

laanwj commented Jun 24, 2018 •

edited

Loading