build: Check usages of #if defined(…) #16547

1. 
   alko89 commented at 3:54 pm on August 5, 2019: none

   PR for #16419.

   I have gone through bitcoin_config.h constants and removed defined where not needed.

   The rationale was to remove the ones where the constant is defined in both config/bitcoin_config.h and build_msvc/bitcoin_conf.h and is declared as 1 or 0. If the constant is commented out or not declared at all in any of those config files, I left the check as #if defined(...), otherwise I changed the definition to #if ...

   If there are any other cases where #if defined is not needed, let me know and I’ll update the PR.

2. Fix #if defined usages 7f3c669ccf
3. alko89 renamed this:
   Check usages of #if defined(...)
   build: Check usages of #if defined(...)
   on Aug 5, 2019
4. MarcoFalke added the label Build system on Aug 5, 2019
5. MarcoFalke added the label Needs gitian build on Aug 5, 2019
6. 
   dongcarl commented at 5:44 pm on August 5, 2019: member

   grepped for all usages, doesn’t seem to be any value-less #defines.

   Some notes while exploring: gcc’s -DFOOBAR flag actually defines FOOBAR as 1, meaning that #if FOOBAR will be true, good default, and we should probably eliminate value-less #defines over time (not urgent) @Alko89 Would you be able to use https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/master/build-for-compare.py to show that no differences arise in the binary after this change?

7. 
   alko89 commented at 6:43 pm on August 5, 2019: none

   @dongcarl I have executed the script you provided with this and previous commit, then compared them with:

   % git diff -W --word-diff /tmp/compare/7f3c669ccf4d97e7aa52722b8549bd474791eb33 /tmp/compare/e55444a2a8a2b4a378e6d42b7bf73d13492a5716

   and get an empty output.

8. 
   dongcarl commented at 9:45 pm on August 5, 2019: member

   @fanquake Is this how build-for-compare.py is supposed to be used? (haven’t used before)
9. 
   fanquake commented at 2:42 am on August 6, 2019: member

   @dongcarl Yes. I’ve also run it against this PR (7f3c669ccf4d97e7aa52722b8549bd474791eb33), and it’s previous commit (e55444a2a8a2b4a378e6d42b7bf73d13492a5716), and didn’t see any difference in the binaries:

   0sha256sum /tmp/compare/*.stripped
   17eb28b0d46d4fc833b6f6b026f8d4ecd20f733f096ded4c0d0f8bada3e6a7944  /tmp/compare/bitcoin-qt.7f3c669ccf4d97e7aa52722b8549bd474791eb33.stripped
   27eb28b0d46d4fc833b6f6b026f8d4ecd20f733f096ded4c0d0f8bada3e6a7944  /tmp/compare/bitcoin-qt.e55444a2a8a2b4a378e6d42b7bf73d13492a5716.stripped
   38e5d3e83e125a387170ace209d9fe851ec022e9a5ffb529c92bb0e49a4019569  /tmp/compare/bitcoind.7f3c669ccf4d97e7aa52722b8549bd474791eb33.stripped
   48e5d3e83e125a387170ace209d9fe851ec022e9a5ffb529c92bb0e49a4019569  /tmp/compare/bitcoind.e55444a2a8a2b4a378e6d42b7bf73d13492a5716.stripped
   

   You obviously also use diffoscope to compare the outputs. i.e:

   0diffoscope /tmp/compare/bitcoind.7f3c669ccf4d97e7aa52722b8549bd474791eb33.stripped \
   1 /tmp/compare/bitcoind.e55444a2a8a2b4a378e6d42b7bf73d13492a5716.stripped
   

10. DrahtBot removed the label Needs gitian build on Aug 6, 2019
11. 
    practicalswift commented at 9:25 am on August 6, 2019: contributor

    Concept ACK

    Thanks for tackling this!

12. in src/bench/verify_script.cpp:7 in 7f3c669ccf

    3@@ -4,7 +4,7 @@
    4 
    5 #include <bench/bench.h>
    6 #include <key.h>
    7-#if defined(HAVE_CONSENSUS_LIB)
    8+#if HAVE_CONSENSUS_LIB
    

    ---

    ---

    

    MarcoFalke commented at 2:52 pm on August 6, 2019:

    This is not defined when ./configure --with-libs=no. So it will default to 0 or something?
13. 
    dongcarl commented at 2:59 pm on August 6, 2019: member

    Yup! See the BUFFSIZE example here: http://gcc.gnu.org/onlinedocs/gcc-3.0.1/cpp_4.html#SEC39

    On Tue, Aug 6, 2019 at 10:56 AM, MarcoFalke notifications@github.com wrote:

    @MarcoFalke commented on this pull request.

    ---

    In src/bench/verify_script.cpp:

    @@ -4,7 +4,7 @@

    #include <bench/bench.h> #include <key.h> -#if defined(HAVE_CONSENSUS_LIB) +#if HAVE_CONSENSUS_LIB

    This is not defined when ./configure –with-libs=no. So it will default to 0 or something?

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

14. 
    MarcoFalke commented at 3:05 pm on August 6, 2019: member

    ACK 7f3c669ccf4d97e7aa52722b8549bd474791eb33 (also checked that gcc and clang produce the same bitcoind before and after

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 7f3c669ccf4d97e7aa52722b8549bd474791eb33 (also checked that gcc and clang produce the same bitcoind before and after
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUh2fgv6A7rYhzadAGdG6t/obAjx4vQKShlg/aJ+QUvkmv8MYFzeQ8R+jHrNr6qQ
     8v9rfPZhgz7yG7W+ir8+ocs5JivkkRVOlm8BAf6g4sopujI866I7+ZHJyG/kEWJH8
     9izOfCinBmgkHLgGC2lNrR+qXZJim//7cySdXlNNKg5wwsec8xBUvyHzNosOTuMXF
    10qK5g3sh6RHKWcZ4Qf4/xaW80kmMtTI9flydo3O9v0AHVZw1rC2D0pDILadcG45cM
    11SydDqSLPx124np/hUvmjxHzK4dezLMfA8spbNRaoKJZzrSfnirxdm77FG+GSQCN7
    125/1F/bxIIy1AHx7hKQI+OslR5ZL5oebiH56Un2pdVqjbo4hGwlPf43PLQV8bDqZw
    13t6yXX7W62qFtEahZQ/m2oMFdWPsg7XI0s7XyGSppUn4XwhCkf89wsKA3MX3hr2Vh
    14gSzzMupNyfaM9+/qITnUzeJwsCaW1d2zWg138FHcDwXBwUz3jGjceinXGP4nAtJZ
    15zmBevb/y
    16=DnP/
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash c37fb0c17180f55491e8dba0f43835dec75e97e5ad9f2c413e542c5681999e94 -

15. 
    MarcoFalke commented at 3:07 pm on August 6, 2019: member

    What about stuff like this:

    0src/crypto/sha256_sse41.cpp:#ifdef ENABLE_SSE41
    

16. 
    dongcarl commented at 3:26 pm on August 6, 2019: member

    What about stuff like this:

    0src/crypto/sha256_sse41.cpp:#ifdef ENABLE_SSE41
    

    Not sure what you’re asking, but #if defined MACRO is precisely equivalent to #ifdef MACRO, so it seems like this should also be changed to #if. @Alko89 could you make sure that’s the case?

17. 
    MarcoFalke commented at 3:38 pm on August 6, 2019: member

    Yeah, if this is changed, it should be done once and for all.
18. 
    alko89 commented at 4:18 pm on August 6, 2019: none

    I have bean reading about if defines in the gcc doc @dongcarl provided. In if chapter it is also stated that undefined macros are interpreted as having the value of 0. I guess in that case most of #if defines (and also #ifdef) are not required.

    https://gcc.gnu.org/onlinedocs/gcc-3.0.1/cpp_4.html#SEC38

    Also the -Wundef option causes gcc to trigger warnings if it encounters an identifier which is not a macro in an #if.

    I will replace all #ifdef and #if defined(...) for numeric macros and try it out.

19. 
    alko89 commented at 6:23 pm on August 6, 2019: none

    I will need more time investigating this.

    What about OS and architecture specific macros like WIN32 or MACOS and __amd64__? The latter is defined as 1 for specific architecture, but might be good to leave as is for better readability?

    As for OS specific macros I’m testing on Linux and would need someone to check how these macros are defined on other platforms.

20. 
    alko89 commented at 6:56 pm on August 6, 2019: none

    What about stuff like this:

    0src/crypto/sha256_sse41.cpp:#ifdef ENABLE_SSE41
    

    Not sure what you’re asking, but #if defined MACRO is precisely equivalent to #ifdef MACRO, so it seems like this should also be changed to #if. @Alko89 could you make sure that’s the case?

    Can confirm that #ifdef MACRO and #if defined MACRO makes no difference. For case like ENABLE_SSE41 it also makes no issue if defined or undefined since it resolves to 1 or 0 respectively.

    Macros that are problematic with #if are value-less macros or macros that are defined as a string or other non-numeric types.

21. 
    DrahtBot commented at 10:20 pm on August 6, 2019: member

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

    Conflicts

    No conflicts as of last run.

22. 
    promag commented at 10:25 pm on August 6, 2019: member

    Could this be a scripted diff?
23. 
    fanquake commented at 4:13 am on August 12, 2019: member

    @Alko89 Is this now ready for review?
24. MarcoFalke added the label Waiting for author on Aug 12, 2019
25. 
    alko89 commented at 8:13 pm on August 19, 2019: none

    you can close this PR for now. I’ll create a new one when I’m done.
26. MarcoFalke closed this on Aug 19, 2019

    ---

27. laanwj removed the label Waiting for author on Oct 24, 2019
28. MarcoFalke deleted a comment on May 31, 2020
29. DrahtBot locked this on Feb 15, 2022
30. hebasto added the label Up for grabs on Feb 12, 2023
31. fanquake removed the label Up for grabs on Feb 13, 2023

Contributors
alko89 dongcarl fanquake practicalswift MarcoFalke DrahtBot promag

Labels
Build system