Skip to content

Avoid >> above type width in BitWriter #74

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
Merged
merged 2 commits into from
Apr 11, 2024
Merged

Avoid >> above type width in BitWriter #74

merged 2 commits into from
Apr 11, 2024

Conversation

sipa
Copy link
Collaborator

@sipa sipa commented Oct 19, 2022 •
edited
Loading

For tiny fields (bits <= 8), the table type is uint8_t, which results in BitWriter using uint8_t logic as well. This is technically UB, as it performs right shifts of up to 8 positions.

Avoid this by performing the BitWriter logic in either the table type, or unsigned, whichever is larger.

This is an alternative to #71.

@sipa sipa mentioned this pull request Oct 19, 2022
Closed
hebasto
hebasto reviewed Oct 19, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Concept ACK.

src/int_utils.h
Comment on lines +83 to +85
template<int BITS, typename I>
inline void Write(I val) {
Copy link
Member

Choose a reason for hiding this comment

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

Whether inline makes anything for templates?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

inline has two meanings in C++:

  • A semantic one, permitting multiple identical definitions in separate translation units.
  • A hint for the compiler that inlining the function is desired.

The first one is always true for templated things, but the second one isn't.

Copy link
Member

Choose a reason for hiding this comment

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

  • A hint for the compiler that inlining the function is desired.

I thought that C++ standard abandoned this meaning of the inline keyword, especially since C++17.

https://en.cppreference.com/w/cpp/language/inline

Of course, compilers could have different opinions :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you're misreading that page. The C++17 specific boxes explain that variables can be inline too now, justified by the fact that inline has ended up meaning "multiple definitions are allowed". It doesn't say anything about whether compilers do or don't also take inline into account for making inlining decisions (which isn't possible anyway, as there is no observable effect from the perspective of language definition).

hebasto
hebasto approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 32a2c64, I have reviewed the code and it looks OK. No -Wshift-count-overflow warnings verified with clang 14.0 on Ubuntu 22.04.

@sipa sipa force-pushed the 202210_noshiftoff branch from 32a2c64 to fe1040f Compare April 10, 2024 07:47
@sipa
Copy link
Collaborator Author

sipa commented Apr 10, 2024

Rebased to retrigger CI.

@sipa
Copy link
Collaborator Author

sipa commented Apr 10, 2024

@hebasto Re-review?

hebasto
hebasto approved these changes Apr 11, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK fe1040f. Only rebased since my recent review.

It still compiles with no -Wshift-count-overflow warnings.

@sipa sipa merged commit 9e6127f into master Apr 11, 2024
fanquake added a commit to fanquake/bitcoin that referenced this pull request Apr 11, 2024
@fanquake
Squashed 'src/minisketch/' changes from a571ba20f9..4a48f31a37
32937e9 
4a48f31a37 Merge bitcoin-core/minisketch#83: ci: Fix "s390x (big-endian)" task
0498084d31 ci: Fix "s390x (big-endian)" task
71709dca9e Merge bitcoin-core/minisketch#82: ci: Fix `x86_64-w64-mingw32` task
9e6127fa98 Merge bitcoin-core/minisketch#74: Avoid >> above type width in BitWriter
ed420bc170 ci: Fix `x86_64-w64-mingw32` task
fe1040f227 Drop -Wno-shift-count-overflow compile flag
154bcd43bd Avoid >> above type width in BitWriter
67b87acdb6 Merge bitcoin-core/minisketch#78: ci: Update macOS image for CI
7de7250416 ci: Update macOS image for CI
83d812ea9f Merge bitcoin-core/minisketch#73: ci: Use correct variable to designate C++ compiler
e051a7d690 ci: Install wine32 package for Windows tests
2d2c695d78 build: Drop unused `CC` variable
1810fcbd11 ci: Use correct variable to designate C++ compiler
022b959049 Merge bitcoin-core/minisketch#77: Add missing include
08443c4892 Add missing include

git-subtree-dir: src/minisketch
git-subtree-split: 4a48f31a3753534e26daabfdb42768c6236395ea
fanquake added a commit to fanquake/bitcoin that referenced this pull request Apr 11, 2024
@fanquake
Squashed 'src/minisketch/' changes from a571ba20f9..33b7c200b9
b1f01e1 
33b7c200b9 Merge bitcoin-core/minisketch#80: Add c++20 version of CountBits
4a48f31a37 Merge bitcoin-core/minisketch#83: ci: Fix "s390x (big-endian)" task
82b6488acb Add c++20 version of CountBits
0498084d31 ci: Fix "s390x (big-endian)" task
71709dca9e Merge bitcoin-core/minisketch#82: ci: Fix `x86_64-w64-mingw32` task
9e6127fa98 Merge bitcoin-core/minisketch#74: Avoid >> above type width in BitWriter
ed420bc170 ci: Fix `x86_64-w64-mingw32` task
fe1040f227 Drop -Wno-shift-count-overflow compile flag
154bcd43bd Avoid >> above type width in BitWriter
67b87acdb6 Merge bitcoin-core/minisketch#78: ci: Update macOS image for CI
7de7250416 ci: Update macOS image for CI
83d812ea9f Merge bitcoin-core/minisketch#73: ci: Use correct variable to designate C++ compiler
e051a7d690 ci: Install wine32 package for Windows tests
2d2c695d78 build: Drop unused `CC` variable
1810fcbd11 ci: Use correct variable to designate C++ compiler
022b959049 Merge bitcoin-core/minisketch#77: Add missing include
08443c4892 Add missing include

git-subtree-dir: src/minisketch
git-subtree-split: 33b7c200b97498139aa1462a0dd0d6eba49d98b7
fanquake added a commit to fanquake/bitcoin that referenced this pull request Apr 12, 2024
@fanquake
Squashed 'src/minisketch/' changes from a571ba20f9..3472e2f5ec
1eea10a 
3472e2f5ec Merge bitcoin-core/minisketch#81: Avoid overflowing shift by special casing inverse of 1
653d8b2e26 Avoid overflowing shift by special casing inverse of 1
33b7c200b9 Merge bitcoin-core/minisketch#80: Add c++20 version of CountBits
4a48f31a37 Merge bitcoin-core/minisketch#83: ci: Fix "s390x (big-endian)" task
82b6488acb Add c++20 version of CountBits
0498084d31 ci: Fix "s390x (big-endian)" task
71709dca9e Merge bitcoin-core/minisketch#82: ci: Fix `x86_64-w64-mingw32` task
9e6127fa98 Merge bitcoin-core/minisketch#74: Avoid >> above type width in BitWriter
ed420bc170 ci: Fix `x86_64-w64-mingw32` task
fe1040f227 Drop -Wno-shift-count-overflow compile flag
154bcd43bd Avoid >> above type width in BitWriter
67b87acdb6 Merge bitcoin-core/minisketch#78: ci: Update macOS image for CI
7de7250416 ci: Update macOS image for CI
83d812ea9f Merge bitcoin-core/minisketch#73: ci: Use correct variable to designate C++ compiler
e051a7d690 ci: Install wine32 package for Windows tests
2d2c695d78 build: Drop unused `CC` variable
1810fcbd11 ci: Use correct variable to designate C++ compiler
022b959049 Merge bitcoin-core/minisketch#77: Add missing include
08443c4892 Add missing include

git-subtree-dir: src/minisketch
git-subtree-split: 3472e2f5ec75ace39ce9243af6b3fee233a67492
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Jun 6, 2024
@fanquake @janus
Squashed 'src/minisketch/' changes from a571ba20f9..3472e2f5ec
b47db96 
3472e2f5ec Merge bitcoin-core/minisketch#81: Avoid overflowing shift by special casing inverse of 1
653d8b2e26 Avoid overflowing shift by special casing inverse of 1
33b7c200b9 Merge bitcoin-core/minisketch#80: Add c++20 version of CountBits
4a48f31a37 Merge bitcoin-core/minisketch#83: ci: Fix "s390x (big-endian)" task
82b6488acb Add c++20 version of CountBits
0498084d31 ci: Fix "s390x (big-endian)" task
71709dca9e Merge bitcoin-core/minisketch#82: ci: Fix `x86_64-w64-mingw32` task
9e6127fa98 Merge bitcoin-core/minisketch#74: Avoid >> above type width in BitWriter
ed420bc170 ci: Fix `x86_64-w64-mingw32` task
fe1040f227 Drop -Wno-shift-count-overflow compile flag
154bcd43bd Avoid >> above type width in BitWriter
67b87acdb6 Merge bitcoin-core/minisketch#78: ci: Update macOS image for CI
7de7250416 ci: Update macOS image for CI
83d812ea9f Merge bitcoin-core/minisketch#73: ci: Use correct variable to designate C++ compiler
e051a7d690 ci: Install wine32 package for Windows tests
2d2c695d78 build: Drop unused `CC` variable
1810fcbd11 ci: Use correct variable to designate C++ compiler
022b959049 Merge bitcoin-core/minisketch#77: Add missing include
08443c4892 Add missing include

git-subtree-dir: src/minisketch
git-subtree-split: 3472e2f5ec75ace39ce9243af6b3fee233a67492
vmta added a commit to umkoin/umkoin that referenced this pull request Oct 6, 2024
@vmta
Squashed 'src/minisketch/' changes from a223557ce..eb37a9b8e
21eb7a8 
eb37a9b8e Merge bitcoin-core/minisketch#87: Avoid copy in self-assign
fe6557642 Merge bitcoin-core/minisketch#88: build: Add `-Wundef`
8ea298bfa Avoid copy in self-assign
978a3d886 build: Add `-Wundef`
338704417 Merge bitcoin-core/minisketch#86: doc: fix typo in sketch_impl.h
15c2d13b6 doc: fix typo in sketch_impl.h
7be08b8a4 Merge bitcoin-core/minisketch#85: Fixes for integer precision loss
00fb4a4d8 Avoid or make integer precision conversion explicit
9d62a4d27 Avoid the need to cast/convert to size_t for vector operations
19e06cc7a Prevent overflows from large capacity/max_elements
3472e2f5e Merge bitcoin-core/minisketch#81: Avoid overflowing shift by special casing inverse of 1
653d8b2e2 Avoid overflowing shift by special casing inverse of 1
33b7c200b Merge bitcoin-core/minisketch#80: Add c++20 version of CountBits
4a48f31a3 Merge bitcoin-core/minisketch#83: ci: Fix "s390x (big-endian)" task
82b6488ac Add c++20 version of CountBits
0498084d3 ci: Fix "s390x (big-endian)" task
71709dca9 Merge bitcoin-core/minisketch#82: ci: Fix `x86_64-w64-mingw32` task
9e6127fa9 Merge bitcoin-core/minisketch#74: Avoid >> above type width in BitWriter
ed420bc17 ci: Fix `x86_64-w64-mingw32` task
fe1040f22 Drop -Wno-shift-count-overflow compile flag
154bcd43b Avoid >> above type width in BitWriter
67b87acdb Merge bitcoin-core/minisketch#78: ci: Update macOS image for CI
7de725041 ci: Update macOS image for CI
83d812ea9 Merge bitcoin-core/minisketch#73: ci: Use correct variable to designate C++ compiler
e051a7d69 ci: Install wine32 package for Windows tests
2d2c695d7 build: Drop unused `CC` variable
1810fcbd1 ci: Use correct variable to designate C++ compiler
022b95904 Merge bitcoin-core/minisketch#77: Add missing include
08443c489 Add missing include
a571ba20f Merge bitcoin-core/minisketch#68: Add missed `#include <string>`
b9a7f7e2b Merge bitcoin-core/minisketch#69: refactor: Drop unused `total` local variables
8a5af94ed Merge bitcoin-core/minisketch#70: build: Remove `-Qunused-arguments` workaround for clang + ccache
c36f1f03a Merge bitcoin-core/minisketch#72: Fix MSVC implementation of `CountBits()` function
0078bedda Ignore `HAVE_CLZ` macro when building with MSVC
1c772918c Fix MSVC implementation of `CountBits()` function
98f87c55f build: Remove `-Qunused-arguments` workaround for clang + ccache
11a1e25c8 refactor: Drop unused `total` local variables
ed6c8fcfd Add missed `#include <string>`
47f0a2d26 Merge bitcoin-core/minisketch#66: msvc: remove direct Bitcoin Core `compat.h` include
64f17584c msvc: remove Core compat.h include

git-subtree-dir: src/minisketch
git-subtree-split: eb37a9b8e79f9e49d73b96a49bf97a96d9eb676c
kwvg added a commit to kwvg/dash that referenced this pull request Oct 29, 2024
@kwvg
Squashed 'src/minisketch/' changes from a571ba20f9..3472e2f5ec
9540ecb 
3472e2f5ec Merge bitcoin-core/minisketch#81: Avoid overflowing shift by special casing inverse of 1
653d8b2e26 Avoid overflowing shift by special casing inverse of 1
33b7c200b9 Merge bitcoin-core/minisketch#80: Add c++20 version of CountBits
4a48f31a37 Merge bitcoin-core/minisketch#83: ci: Fix "s390x (big-endian)" task
82b6488acb Add c++20 version of CountBits
0498084d31 ci: Fix "s390x (big-endian)" task
71709dca9e Merge bitcoin-core/minisketch#82: ci: Fix `x86_64-w64-mingw32` task
9e6127fa98 Merge bitcoin-core/minisketch#74: Avoid >> above type width in BitWriter
ed420bc170 ci: Fix `x86_64-w64-mingw32` task
fe1040f227 Drop -Wno-shift-count-overflow compile flag
154bcd43bd Avoid >> above type width in BitWriter
67b87acdb6 Merge bitcoin-core/minisketch#78: ci: Update macOS image for CI
7de7250416 ci: Update macOS image for CI
83d812ea9f Merge bitcoin-core/minisketch#73: ci: Use correct variable to designate C++ compiler
e051a7d690 ci: Install wine32 package for Windows tests
2d2c695d78 build: Drop unused `CC` variable
1810fcbd11 ci: Use correct variable to designate C++ compiler
022b959049 Merge bitcoin-core/minisketch#77: Add missing include
08443c4892 Add missing include

git-subtree-dir: src/minisketch
git-subtree-split: 3472e2f5ec75ace39ce9243af6b3fee233a67492
tomt1664 pushed a commit to tomt1664/elements that referenced this pull request Apr 24, 2025
Squashed 'src/minisketch/' changes from a571ba20f9..3472e2f5ec
6e0b07d 
3472e2f5ec Merge bitcoin-core/minisketch#81: Avoid overflowing shift by special casing inverse of 1
653d8b2e26 Avoid overflowing shift by special casing inverse of 1
33b7c200b9 Merge bitcoin-core/minisketch#80: Add c++20 version of CountBits
4a48f31a37 Merge bitcoin-core/minisketch#83: ci: Fix "s390x (big-endian)" task
82b6488acb Add c++20 version of CountBits
0498084d31 ci: Fix "s390x (big-endian)" task
71709dca9e Merge bitcoin-core/minisketch#82: ci: Fix `x86_64-w64-mingw32` task
9e6127fa98 Merge bitcoin-core/minisketch#74: Avoid >> above type width in BitWriter
ed420bc170 ci: Fix `x86_64-w64-mingw32` task
fe1040f227 Drop -Wno-shift-count-overflow compile flag
154bcd43bd Avoid >> above type width in BitWriter
67b87acdb6 Merge bitcoin-core/minisketch#78: ci: Update macOS image for CI
7de7250416 ci: Update macOS image for CI
83d812ea9f Merge bitcoin-core/minisketch#73: ci: Use correct variable to designate C++ compiler
e051a7d690 ci: Install wine32 package for Windows tests
2d2c695d78 build: Drop unused `CC` variable
1810fcbd11 ci: Use correct variable to designate C++ compiler
022b959049 Merge bitcoin-core/minisketch#77: Add missing include
08443c4892 Add missing include

git-subtree-dir: src/minisketch
git-subtree-split: 3472e2f5ec75ace39ce9243af6b3fee233a67492
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@hebasto hebasto hebasto approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@sipa @hebasto