Document FreeBSD quirk. Fix FreeBSD build: Use std::min(…) to allow for compilation under certain FreeBSD versions. #13503

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:document-freebsd-quirk changing 1 files +2 −1
  1. practicalswift commented at 1:48 pm on June 19, 2018: contributor
    • Document FreeBSD quirk.
    • Fix FreeBSD build: Cast to int to allow std::min to work under FreeBSD.

    Context: #9598 (comment)

  2. fanquake added the label Linux/Unix on Jun 19, 2018
  3. fanquake commented at 2:14 pm on June 19, 2018: member

    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:

    0fvisibility=hidden -c -o libbitcoin_server_a-init.o `test -f 'init.cpp' || echo './'`init.cpp
1init.cpp:969:2: error: invalid preprocessing directive #fi
2 #fi
3  ^~
4init.cpp:964:0: error: unterminated #else
5 #if defined(__FreeBSD__) || defined(__DragonFly__)
6 
7make[2]: *** [libbitcoin_server_a-init.o] Error 1
8Makefile:5817: recipe for target 'libbitcoin_server_a-init.o' failed
  4. in src/init.cpp:969 in d0f7f6b52c outdated 
    960@@ -961,7 +961,12 @@ bool AppInitParameterInteraction()
961     nMaxConnections = std::max(nUserMaxConnections, 0);
962 
963     // Trim requested connection counts, to fit into system limitations
964+#if defined(__FreeBSD__) || defined(__DragonFly__)
965+    // See #2695: Explictly cast to int to allow std::min to work under FreeBSD
966+    nMaxConnections = std::max(std::min(nMaxConnections, (int)(FD_SETSIZE - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS)), 0);
967+#else
968     nMaxConnections = std::max(std::min(nMaxConnections, FD_SETSIZE - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS), 0);
969+#fi
    ken2812221 commented at 2:15 pm on June 19, 2018:
    This is not bash script :)
  5. practicalswift force-pushed on Jun 19, 2018
  6. practicalswift commented at 2:20 pm on June 19, 2018: contributor
    @fanquake @ken2812221 Sorry about the bashism :-) Not fixed!
  7. practicalswift commented at 2:23 pm on June 19, 2018: contributor
    @fanquake Good question regarding FreeBSD version. @anton48, what FreeBSD version are you running?
  8. practicalswift commented at 2:25 pm on June 19, 2018: contributor
    If this is fixed in FreeBSD 11.1 I’m not sure we have a problem to solve :-)
  9. practicalswift commented at 2:28 pm on June 19, 2018: contributor

    Closing this temporarily while waiting for input from @anton48.

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

  10. practicalswift closed this on Jun 19, 2018
  11. anton48 commented at 8:42 pm on June 19, 2018: none
    @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.
  12. fanquake reopened this on Jun 21, 2018
  13. fanquake commented at 1:39 am on June 21, 2018: member

    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.

  14. MarcoFalke commented at 1:54 am on June 21, 2018: member

    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)

  15. practicalswift force-pushed on Jun 21, 2018
  16. Document FreeBSD quirk. Fix FreeBSD build. 629a47a154
  17. practicalswift force-pushed on Jun 21, 2018
  18. practicalswift renamed this:
    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.
    on Jun 21, 2018
  19. practicalswift renamed this:
    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.
    on Jun 21, 2018
  20. practicalswift commented at 7:33 am on June 21, 2018: contributor
    @fanquake @MarcoFalke Thanks for reviewing. Feedback addressed. Please re-review :-) @anton48 Can you confirm that the updated version works as expected for you?
  21. anton48 commented at 7:55 am on June 21, 2018: none
    @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).
  22. practicalswift commented at 8:13 am on June 21, 2018: contributor
    @anton48 Yes, that’s the new version. Thanks for confirming.
  23. laanwj commented at 2:05 pm on June 24, 2018: member
    I’m confused. I build bitcoin core on FreeBSD all the time and have never needed this. #2695 is ancient.
  24. MarcoFalke commented at 2:18 pm on June 24, 2018: member
    The cast to int was only recently removed in master and I believe it only fails on FreeBSD 10, not 11.
  25. MarcoFalke commented at 9:18 pm on June 24, 2018: member
    utACK 629a47a1543a6e77cbf9c73917e2e419669b04df
  26. sipa commented at 1:03 am on June 27, 2018: member
    utACK 629a47a1543a6e77cbf9c73917e2e419669b04df
  27. fanquake commented at 8:12 am on June 27, 2018: member
    utACK 629a47a for fixing the FreeBSD 10.x build. As noted above, the issue comes from Clang 3.4
  28. MarcoFalke merged this on Jun 27, 2018
  29. MarcoFalke closed this on Jun 27, 2018
  30. MarcoFalke referenced this in commit c655b2c2df on Jun 27, 2018
  31. codablock referenced this in commit ac02dcf7d5 on Apr 7, 2020
  32. codablock referenced this in commit 9591199629 on Apr 8, 2020
  33. deadalnix referenced this in commit ee323c8b68 on Jun 9, 2020
  34. ftrader referenced this in commit add9abcee0 on Aug 17, 2020
  35. ckti referenced this in commit 136846ae40 on Mar 28, 2021
  36. practicalswift deleted the branch on Apr 10, 2021
  37. gades referenced this in commit 1b227c47ff on Jun 25, 2021
  38. laanwj referenced this in commit 8b523f2e55 on Sep 27, 2021
  39. sidhujag referenced this in commit e84c360598 on Sep 27, 2021
  40. gades referenced this in commit 8c955a9ce0 on Apr 1, 2022
  41. DrahtBot locked this on Aug 18, 2022


practicalswift fanquake ken2812221 anton48 MarcoFalke laanwj sipa

Labels
Linux/Unix

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-22 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me