build: Check usages of #if defined(...)

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

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.

Fix #if defined usages 7f3c669ccf

alko89 renamed this:
~~Check usages of #if defined(...)~~
build: Check usages of #if defined(...)
on Aug 5, 2019

MarcoFalke added the label Build system on Aug 5, 2019

MarcoFalke added the label Needs gitian build on Aug 5, 2019

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?

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.

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)

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

DrahtBot removed the label Needs gitian build on Aug 6, 2019

practicalswift commented at 9:25 am on August 6, 2019: contributor

Concept ACK

Thanks for tackling this!

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?

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.

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 -

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

What about stuff like this:

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

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?

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

Yeah, if this is changed, it should be done once and for all.

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.

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.

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.

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.

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

Could this be a scripted diff?

fanquake commented at 4:13 am on August 12, 2019: member

@Alko89 Is this now ready for review?

MarcoFalke added the label Waiting for author on Aug 12, 2019

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.

MarcoFalke closed this on Aug 19, 2019

laanwj removed the label Waiting for author on Oct 24, 2019

MarcoFalke deleted a comment on May 31, 2020

DrahtBot locked this on Feb 15, 2022

hebasto added the label Up for grabs on Feb 12, 2023

fanquake removed the label Up for grabs on Feb 13, 2023

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

Conflicts