Skip to content
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.

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

Add compile time verification of assumptions we're currently making implicitly/tacitly #15391

Merged
merged 1 commit into from Feb 15, 2019

Conversation

Projects
None yet
7 participants
@practicalswift
Copy link
Member

commented Feb 12, 2019

Add compile time verification of assumptions we're currently making implicitly/tacitly.

As suggested by @sipa in #14239 (comment) and @MarcoFalke in #14479 (comment).

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

It would be useful for it to get compiled, at least AFAICT adding a false assumption here won't make it fail. :) Concept ACK. Maybe also the #if defined(NDEBUG)? check? Probably every other primitive type we depend on the size of, including the unsigned ones.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

You are only adding a header. Does this need to be included in a cpp file to get compiled?

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

@gmaxwell @MarcoFalke Yes, obviously it needs to be included :-) The inclusion somehow got lost during my latest git commit --amend fixup. Fixing!

@practicalswift practicalswift force-pushed the practicalswift:assumptions branch from f04b48d to 1ea39ed Feb 12, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

Now including from src/util/system.h which also is the most included file FWIW :-)

$ git grep -E "^#include " -- "*.cpp" | cut -f2 -d'<' | cut -f1 -d'>' | sort | uniq -c | \
      sort -n | tail -1
     99 util/system.h

Let me know if you can think of a more appropriate file for the include.

@practicalswift practicalswift force-pushed the practicalswift:assumptions branch 3 times, most recently from a015e45 to 8add86e Feb 12, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

Added a couple of assumptions and listed important "non-assumptions".

Please help me identify further assumptions and corresponding examples of where we are relying on said assumptions :-)

@jb55

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

utACK 8add86e

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@jb55 Thanks for the review! Can you think of any further assumptions and examples of where we rely on them being true? :-)

Show resolved Hide resolved src/assumptions.h Outdated

@practicalswift practicalswift force-pushed the practicalswift:assumptions branch from 8add86e to 7548e6e Feb 14, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Moved to src/compat/ and added an explicit non-assumption regarding size_t :-)

@jb55

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

re-utACK 7548e6e

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK 7548e6e
I'm not 100% sure we make the int=32 bit assumption (more like "int is at least 32 bit" I think? otherwise we use explicitly sized types like int32_t), but I doubt anyone ever tested the code on an architecture with a different integer size so I'm fine with making the assumption.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@laanwj If I'm reading GetSizeOfCompactSize, WriteCompactSize and ReadCompactSize correctly we're assuming that int has a width of exactly 32 bits, no?

Example:

/**
 * Compact Size
 * size <  253        -- 1 byte
 * size <= USHRT_MAX  -- 3 bytes  (253 + 2 bytes)
 * size <= UINT_MAX   -- 5 bytes  (254 + 4 bytes)
 * size >  UINT_MAX   -- 9 bytes  (255 + 8 bytes)
 */
inline unsigned int GetSizeOfCompactSize(uint64_t nSize)
{
    if (nSize < 253)             return sizeof(unsigned char);
    else if (nSize <= std::numeric_limits<unsigned short>::max()) return sizeof(unsigned char) + sizeof(unsigned short);
    else if (nSize <= std::numeric_limits<unsigned int>::max())  return sizeof(unsigned char) + sizeof(unsigned int);
    else                         return sizeof(unsigned char) + sizeof(uint64_t);
}

@practicalswift practicalswift force-pushed the practicalswift:assumptions branch from 7548e6e to 7cee858 Feb 14, 2019

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@jb55 @laanwj Please re-review after s/BITCOIN_ASSUMPTIONS_H/BITCOIN_COMPAT_ASSUMPTIONS_H/g

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@laanwj If I'm reading GetSizeOfCompactSize, WriteCompactSize and ReadCompactSize correctly we're assuming that int has a width of exactly 32 bits?

You're right, thanks for giving an example.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Would it make sense to refer to an example for each assumption. That way, we know of at least one example. An alternative would be to just inline the assumptions where they are needed.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@MarcoFalke I'm not sure I follow: the examples have been there since this PR first was submitted? :-)

In this specific case the following has been in there all along:

// Assumption: We assume integer widths.
// Example(s): GetSizeOfCompactSize and WriteCompactSize in the serialization
//             code.
static_assert(sizeof(short) == 2, "16-bit short assumed");
static_assert(sizeof(int) == 4, "32-bit int assumed");

:-)

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Ok, my bad. I must have missed them when I last looked at it a few days ago.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

ACK 7cee858

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15146 (Solve SmartOS FD_ZERO build issue by Empact)

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.

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK 7cee858

@laanwj laanwj merged commit 7cee858 into bitcoin:master Feb 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 15, 2019

Merge #15391: Add compile time verification of assumptions we're curr…
…ently making implicitly/tacitly

7cee858 Add compile time verification of assumptions we're currently making implicitly/tacitly (practicalswift)

Pull request description:

  Add compile time verification of assumptions we're currently making implicitly/tacitly.

  As suggested by @sipa in #14239 (comment) and @MarcoFalke in #14479 (comment).

Tree-SHA512: e68fe51164dbd3eeb76aa8a7e83dfcd3b4d5a66037c0f1822bbbd189bbe3c280e03b3b10af870880ecc09b612e62fb3d9bcd6cf1e16cb7ba818c257db0712ce4

MarcoFalke added a commit that referenced this pull request Feb 17, 2019

Merge #15431: msvc: scripted-diff: Remove NDEBUG pre-define in projec…
…t file

3ec56be appveyor: Remove unused NDEBUG removal (Chun Kuan Lee)
8a1f0a3 scripted-diff: Remove NDEBUG pre-define (Chun Kuan Lee)

Pull request description:

  Follow #15391

Tree-SHA512: f264418cbc69b5f083469ed9005a6d592d4268f2b7da967e571ce30195de73b09a9e14c8610a5b6b0f056847d82a4bc7c2fbe56498307093aab4dd42903e6137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.