p2p: declare Announcement::m_state as uint8_t, add getter/setter #20162

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:bitfield-too-small-warning changing 1 files +54 −46

jonatack commented at 11:34 PM on October 15, 2020: contributor
Change Announcement::m_state in tx_request.cpp from type State to uint8_t and add a getter and setter for the conversion to/from State. This should silence these travis ci gcc compiler warnings:
```
txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is
too small to hold all values of ‘enum class {anonymous}::State’
     State m_state : 3;
                     ^
```
The gcc warnings are based on the maximum value held by the underlying uint8_t enumerator type, even though the intention of the bitfield declaration is the maximum declared enumerator value. They have apparently been silenced in gcc 8.4+ and 9.3+ according to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414.
sipa commented at 11:38 PM on October 15, 2020: member

If you increase the size of m_state, please also decrease the size of m_sequence (54 bits is still plenty!), otherwise the memory usage of an Announcement object will increase by 16 bytes.

An alternative is changing the type to uint8_t, and having a getter/setter on Announcement to convert/from to State.
ajtowns commented at 11:43 PM on October 15, 2020: contributor

Needs to shrink m_sequence by 5 bits to keep the overall data structure size the same.

The original solution was making m_state be a uint8_t : 3 and having a getter and setter that did the conversion to/from State rather than accessing it directly. I think that's better than shrinking the sequence by 5 bits, but don't have an opinion between doing the getter/setter and spurious warning in older versions of gcc. (EDIT: sequence not priority, duh)
sipa commented at 11:45 PM on October 15, 2020: member

@ajtowns Priority isn't stored anywhere, it's computed on the fly. But sequence can be shrunk with no downside really.
jonatack force-pushed on Oct 15, 2020
jonatack commented at 11:51 PM on October 15, 2020: contributor

Thanks -- adjusted m_sequence accordingly. It's late so will re-look at this tomorrow.
jonatack force-pushed on Oct 15, 2020
jonatack force-pushed on Oct 16, 2020
jonatack commented at 9:06 PM on October 16, 2020: contributor

Previous commit 0bd2602 added 5 bits to m_state and removed 5 from m_sequence.

Current commit 39cb2ba follows your suggestions to do uint8_t m_state : 3 with a getter and setter.

p2p: declare Announcement::m_state as uint8_t, add getter/setter

to silence these Travis CI GCC compiler warnings:

txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is
too small to hold all values of ‘enum class {anonymous}::State’
     State m_state : 3;
                     ^
The warnings are based on the maximum value held by the underlying uint8_t
enumerator type, though the intention of the bitfield declaration is the
maximum declared enumerator value.

The warning been silenced in GCC 8.4+ and 9.3+ according to
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414

c8abbc9d1f

in src/txrequest.cpp:73 in 39cb2ba486 outdated
```
  69 | @@ -70,31 +70,38 @@ struct Announcement {
  70 |      const bool m_is_wtxid : 1;
  71 |  
  72 |      /** What state this announcement is in. */
  73 | -    State m_state : 3;
  74 | +    /** This is a uint8_t instead of a State to silence a GCC warning. */
```
sipa commented at 9:12 PM on October 16, 2020:

Perhaps document which GCC version(s) and a link to the bug, so that we can decide in the future if it's worth maintaining this workaround.

(I initially had this approach in my PR, but removed it after I couldn't reproduce it anymore).

jonatack commented at 9:17 PM on October 16, 2020:

Good idea. Yes, this is the approach in your commit daa4f59b, the first one I reviewed.

jonatack commented at 9:36 PM on October 16, 2020:

Done
jonatack force-pushed on Oct 16, 2020
jonatack renamed this:
~~p2p, compiler warnings: specify Announcement::m_state bitfield to be 8 bits~~
p2p: declare Announcement::m_state as uint8_t, add getter/setter
on Oct 16, 2020
sipa commented at 10:30 PM on October 16, 2020: member

utACK c8abbc9d1fd8f77b5dcf31cf7a6dd6aac5d1c75c
DrahtBot added the label P2P on Oct 16, 2020
ajtowns commented at 1:37 AM on October 17, 2020: contributor

ACK c8abbc9d1fd8f77b5dcf31cf7a6dd6aac5d1c75c -- quick code review
fanquake cross-referenced this on Oct 17, 2020 from issue txrequest announcement warning by sidhujag
hebasto cross-referenced this on Oct 19, 2020 from issue ci: Build with --enable-werror by default, and document exceptions by hebasto
hebasto approved
hebasto commented at 11:03 AM on October 19, 2020: member
ACK c8abbc9d1fd8f77b5dcf31cf7a6dd6aac5d1c75c, tested on Bionic (x86_64, gcc 7.5.0):
- master (4769942d901a6095dc8715b6008d608e62d10b3c):
```
$ make clean && make > /dev/null
txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is too small to hold all values of ‘enum class {anonymous}::State’
     State m_state : 3;
                     ^
```
- this PR (c8abbc9d1fd8f77b5dcf31cf7a6dd6aac5d1c75c):
```
$ make clean && make > /dev/null
# no warnings :)
```
I verified that the following statements did not changed:
- sizeof(Announcement) == size_t(56)
- alignof(Announcement) == size_t(8)
MarcoFalke added the label Refactoring on Oct 19, 2020
MarcoFalke merged this on Oct 19, 2020
MarcoFalke closed this on Oct 19, 2020
jonatack deleted the branch on Oct 19, 2020
sidhujag referenced this in commit 199be16fdd on Oct 19, 2020
MarcoFalke referenced this in commit e2ae6a2bef on Dec 4, 2020
deadalnix referenced this in commit 07731aaf05 on May 20, 2021
Fabcien referenced this in commit 47c2219049 on May 20, 2021
JaredTate cross-referenced this on Oct 30, 2021 from issue WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code by JaredTate
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#20173 txrequest announcement warning #20182 ci: Build with --enable-werror by default, and document exceptions