build: fix sysctl() detection on macOS #18359

1. 
   fanquake commented at 9:43 AM on March 16, 2020: member

   sysctl() on *BSD takes a "const int *name", whereas sysctl() on macOS it takes an "int *name". So our configure check and sysctl() detection on macOS currently fails:

   /usr/include/sys/sysctl.h:759:9: note: candidate function not viable:
   	no known conversion from 'const int [2]' to 'int *' for 1st argument
   int     sysctl(int *, u_int, void *, size_t *, void *, size_t);
   

   The simplest change seems to be to change the param to a "int *name", which will work during configure on macOS and *BSD systems.

   For consistency I've changed both calls, but note that macOS doesn't have KERN_ARND, so that check will always fail regardless. We can revert/add documentation if preferred.

2. fanquake added the label Build system on Mar 16, 2020
3. 
   laanwj commented at 10:44 AM on March 16, 2020: member

   ACK 155ce7f7b30a800b7b21f1965ee21967ffc18422 Changing both is good for consistency IMO.

4. laanwj added this to the milestone 0.20.0 on Mar 16, 2020
5. 
   practicalswift commented at 11:22 AM on March 16, 2020: contributor

   Concept ACK

6. 
   laanwj commented at 4:07 PM on March 17, 2020: member

   Can someone on MacOS test this please and post results? (e.g. relevant configure output)

7. 
   sipa commented at 6:32 PM on March 17, 2020: member

   ACK after verifying that this is a no-op for MacOS.

8. 
   dongcarl commented at 7:11 PM on March 17, 2020: member

   Ran ./configure on OSX 10.14.6 before and after this change.

   Before:

   configure:24666: checking for sysctl
   configure:24684: g++ -std=c++11 -c -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -I/usr/local/opt/berkeley-db@4/include -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 conftest.cpp >&5
   conftest.cpp:73:5: error: no matching function for call to 'sysctl'
       sysctl(name, 2, nullptr, nullptr, nullptr, 0);
       ^~~~~~
   /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/sysctl.h:759:9: note: candidate function not viable: no known conversion from 'const int [2]' to 'int *' for 1st argument
   int     sysctl(int *, u_int, void *, size_t *, void *, size_t);
           ^
   1 error generated.
   

   After:

   configure:24666: checking for sysctl
   configure:24684: g++ -std=c++11 -c -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -I/usr/local/opt/berkeley-db@4
   /include -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 conftest.cpp >&5
   configure:24684: $? = 0
   configure:24685: result: yes
   configure:24696: checking for sysctl KERN_ARND
   configure:24711: g++ -std=c++11 -c -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -I/usr/local/opt/berkeley-db@4/include -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 conftest.cpp >&5
   conftest.cpp:70:34: error: use of undeclared identifier 'KERN_ARND'
    static int name[2] = {CTL_KERN, KERN_ARND};
                                    ^
   1 error generated.
   

9. 
   theuni commented at 7:56 PM on March 18, 2020: member

   Concept ACK.

   Shouldn't the actual usage in random.cpp be switched the more generic non-const version as well?

10. 
    theuni commented at 8:07 PM on March 18, 2020: member

    Actually... for the test, how about bypassing the issue:

    sysctl(nullptr, 2, nullptr, nullptr, nullptr, 0);
    

    ?

11. build: fix sysctl() detection on macOS

    sysctl() on *BSD takes a "const int *name", whereas sysctl() on macOS
    it takes an "int *name". So our configure check and sysctl() detection on
    macOS currently fails:
    
    ```bash
    /usr/include/sys/sysctl.h:759:9: note: candidate function not viable:
    	no known conversion from 'const int [2]' to 'int *' for 1st argument
    int     sysctl(int *, u_int, void *, size_t *, void *, size_t);
    ```
    
    This change removes the name argument from the sysctl() detection check,
    meaning we will detect correctly on macOS and *BSD.
    
    For consistency we also switch to using the more generic, non-const
    version of the name parameter in the rest of our usage.

    e90e3e684f
12. fanquake force-pushed on Mar 19, 2020
13. 
    fanquake commented at 4:02 AM on March 19, 2020: member

    @theuni Thanks for the comments / suggestions. I've pushed some changes.

    Actually... for the test, how about bypassing the issue:

    I've tested sysctl(nullptr, 2, nullptr, nullptr, nullptr, 0); for sysctl() detection in configure. It works on macOS and *BSDs. This seems like a good simplification.

    Shouldn't the actual usage in random.cpp be switched the more generic non-const version as well?

    There are two places we are using sysctl:

    In randomenv.cpp we are already using the non-const version:

    https://github.com/bitcoin/bitcoin/blob/a421e0a22f1230abd69b4661a019bed39b72205f/src/randomenv.cpp#L166-L171

    In random.cpp we are currently using the const int version:

    https://github.com/bitcoin/bitcoin/blob/a421e0a22f1230abd69b4661a019bed39b72205f/src/random.cpp#L323-L331

    Note that only FreeBSD and NetBSD should fall in here. We already special case macOS and OpenBSD for calls to getentropy(), and neither of them support KERN_ARND anyways. OpenBSD used to, but removed support in 6.1. I don't think macOS ever has.

    Saying that, to be consistent in our sysctl() usage I've also changed this call to the non-const version.

    I've tested e90e3e684ffa7b25f0dfb5b45e70bb0c358261fb on the following platforms:

    sysctl() detection: FreeBSD 12.1 NetBSD 8.1 OpenBSD 6.6 macOS 10.14

    KERN_ARND detection: FreeBSD 12.1 NetBSD 8.1

14. 
    DrahtBot commented at 5:49 AM on March 19, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17563 (lib: fix a compiler warning: unused GetDevURandom() by vasild)

    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.

15. 
    laanwj commented at 2:19 PM on March 19, 2020: member

    That simplification is good though it does change the functionality of the test. Do we care whether CTL_KERN, KERN_VERSION and KERN_ARND are defined?

    Edit: the check for KERN_VERSION is done separately in src/randomenv.cpp. So looks good to me. Re-ACK e90e3e684ffa7b25f0dfb5b45e70bb0c358261fb

16. laanwj merged this on Mar 19, 2020
17. laanwj closed this on Mar 19, 2020

    ---

18. fanquake deleted the branch on Mar 20, 2020
19. fanquake referenced this in commit 8ebb6562f2 on Mar 6, 2021
20. fanquake referenced this in commit 256a71027a on Apr 10, 2022
21. fanquake referenced this in commit d9345e04eb on Apr 10, 2022
22. PastaPastaPasta referenced this in commit b582fbb9a6 on Apr 25, 2022
23. DrahtBot locked this on Aug 16, 2022

Milestone
0.20.0