Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

alko89
Copy link

@alko89 alko89 commented Aug 5, 2019

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.

@alko89 alko89 changed the title Check usages of #if defined(...) build: Check usages of #if defined(...) Aug 5, 2019
@dongcarl
Copy link
Contributor

dongcarl commented Aug 5, 2019

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
Copy link
Author

alko89 commented Aug 5, 2019 •
edited
Loading

@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
Copy link
Contributor

dongcarl commented Aug 5, 2019

@fanquake Is this how build-for-compare.py is supposed to be used? (haven't used before)

@fanquake
Copy link
Member

fanquake commented Aug 6, 2019

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

sha256sum /tmp/compare/*.stripped
7eb28b0d46d4fc833b6f6b026f8d4ecd20f733f096ded4c0d0f8bada3e6a7944  /tmp/compare/bitcoin-qt.7f3c669ccf4d97e7aa52722b8549bd474791eb33.stripped
7eb28b0d46d4fc833b6f6b026f8d4ecd20f733f096ded4c0d0f8bada3e6a7944  /tmp/compare/bitcoin-qt.e55444a2a8a2b4a378e6d42b7bf73d13492a5716.stripped
8e5d3e83e125a387170ace209d9fe851ec022e9a5ffb529c92bb0e49a4019569  /tmp/compare/bitcoind.7f3c669ccf4d97e7aa52722b8549bd474791eb33.stripped
8e5d3e83e125a387170ace209d9fe851ec022e9a5ffb529c92bb0e49a4019569  /tmp/compare/bitcoind.e55444a2a8a2b4a378e6d42b7bf73d13492a5716.stripped

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

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

@practicalswift
Copy link
Contributor

Concept ACK

Thanks for tackling this!

maflcko
maflcko reviewed Aug 6, 2019
View reviewed changes
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dongcarl
Copy link
Contributor

dongcarl commented Aug 6, 2019 via email

@maflcko
Copy link
Member

maflcko commented Aug 6, 2019

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

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh2fgv6A7rYhzadAGdG6t/obAjx4vQKShlg/aJ+QUvkmv8MYFzeQ8R+jHrNr6qQ
v9rfPZhgz7yG7W+ir8+ocs5JivkkRVOlm8BAf6g4sopujI866I7+ZHJyG/kEWJH8
izOfCinBmgkHLgGC2lNrR+qXZJim//7cySdXlNNKg5wwsec8xBUvyHzNosOTuMXF
qK5g3sh6RHKWcZ4Qf4/xaW80kmMtTI9flydo3O9v0AHVZw1rC2D0pDILadcG45cM
SydDqSLPx124np/hUvmjxHzK4dezLMfA8spbNRaoKJZzrSfnirxdm77FG+GSQCN7
5/1F/bxIIy1AHx7hKQI+OslR5ZL5oebiH56Un2pdVqjbo4hGwlPf43PLQV8bDqZw
t6yXX7W62qFtEahZQ/m2oMFdWPsg7XI0s7XyGSppUn4XwhCkf89wsKA3MX3hr2Vh
gSzzMupNyfaM9+/qITnUzeJwsCaW1d2zWg138FHcDwXBwUz3jGjceinXGP4nAtJZ
zmBevb/y
=DnP/
-----END PGP SIGNATURE-----

Timestamp of file with hash c37fb0c17180f55491e8dba0f43835dec75e97e5ad9f2c413e542c5681999e94 -

@maflcko
Copy link
Member

maflcko commented Aug 6, 2019

What about stuff like this:

src/crypto/sha256_sse41.cpp:#ifdef ENABLE_SSE41

@dongcarl
Copy link
Contributor

dongcarl commented Aug 6, 2019

What about stuff like this:

src/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?

@maflcko
Copy link
Member

maflcko commented Aug 6, 2019 •
edited
Loading

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

@alko89
Copy link
Author

alko89 commented Aug 6, 2019

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
Copy link
Author

alko89 commented Aug 6, 2019

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
Copy link
Author

alko89 commented Aug 6, 2019 •
edited
Loading

What about stuff like this:

src/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
Copy link
Contributor

DrahtBot commented Aug 6, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@promag
Copy link
Contributor

promag commented Aug 6, 2019

Could this be a scripted diff?

@fanquake
Copy link
Member

@alko89 Is this now ready for review?

@alko89
Copy link
Author

alko89 commented Aug 19, 2019

you can close this PR for now. I'll create a new one when I'm done.

@maflcko maflcko closed this Aug 19, 2019
@bitcoin bitcoin deleted a comment from DrahtBot May 31, 2020
@maflcko maflcko mentioned this pull request May 31, 2020
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@alko89 @dongcarl @fanquake @practicalswift @maflcko @DrahtBot @promag @laanwj @hebasto