Add native support for serializing char arrays without FLATDATA #12740

sipa · 2018-03-21T00:40:53Z

Support is added to serialize arrays of type char or unsigned char directly, without any wrappers. All invocations of the FLATDATA wrappers that are obsoleted by this are removed.

This includes a patch by @ryanofsky to make char casting type safe.

The serialization of CSubNet is changed to serialize a bool directly rather than though FLATDATA. This makes the serialization independent of the size of the bool type (and will use 1 byte everywhere).

This is a small change taken from #10785.

eklitzke

utACK 7d29fa4d98ecce780f1b0a2e3004c22c8f4d2395

eklitzke · 2018-03-21T01:02:01Z

src/serialize.h

nit: since these are inline, the static specifier is ignored

Really?

My understanding was that a non-static inline function will still be emitted in non-inlined form in the object file, as the compiler can't know there are no non-inlinable calls from other translation units.

Making it static guarantees no calls from other translation units are possible, and thus permits the compliler to not emit any code as long as all calls within the current translation units are inlinable.

That's my understanding too.

It seems my understanding is correct for C, but not for modern C++. inline in C++ is not just a hint that the function is preferably inlined, but also automatically implies the function may violate ODR.

Thanks, this is very interesting.

It seems my understanding is correct for C, but not for modern C++. inline in C++ is not just a hint that the function is preferably inlined, but also automatically implies the function may violate ODR.

Really confused. What is the reason for removing static and thinking it is ignored? What version of modern c++ is this assuming? I can't follow the logic here.

Reading more on: http://en.cppreference.com/w/cpp/language/inline

Semantically, static (or anonymous namespace) in combination with inline is not meaningless. It still indicates that different translation units may have different definitions. Without static it is (in theory) undefined to have multiple translation units with a different definition.

However, non-static inline functions must be defined inline in every translation unit, and the compiler guarantees it will have the same address in all of them (if any) (these properties are not true in C). This means that the reason why I was using static here (being worried about having multiple emitted copies of a non-inlined version of the function) doesn't apply.

I also tested this, by removing the static and observing that no symbol is emitted in any of the object files produced by gcc.

By modern I mean C++03 and up.

This means that the reason why I was using static here (being worried about having multiple emitted copies of a non-inlined version of the function) doesn't apply.

I'd still think you'd want to specify static to tell the compiler that the function won't be accessed from a different translation unit and that exporting a symbol isn't necessary. It doesn't seem like it can be true that no symbol is emitted without static because according to your link, "An inline function with external linkage (e.g. not declared static)" "has the same address in every translation unit," which would require a symbol.

Testing locally, I see both inline and static inline functions emitting symbols, but inline functions have global symbols (W in nm output) while static inline functions only have local symbols (t in nm output).

@ryanofsky That's what I'm seeing at -O0 (when inlining is disabled). With -O1 or higher, no symbol is emitted with either inline or static inline when all call sites are inlinable.

Also, when compiling with g++, with multiple object files where the call sites are not inlinable, each object gets indeed a symbol, but they're turned into a single symbol in the final executable. When compiling with gcc, the final executable has one symbol per original object.

I think that's the explanation: all translation units with a non-inlinable call site of an inline function indeed get an emitted symbol, but those are combined into one by the linker.

Given that C++ requires that all translation units in which a non-static inline function appears mark it as inline, it means there can't be any actual external calls from another unit to an inline function symbol; they're required to have their own definition, so no symbol is required to be emitted. If it is emitted however, it will be combined across all units by the linked.

dcousens · 2018-03-21T03:13:34Z

src/serialize.h

why not reinterpret_cast?

For primitive types there is no distinction between c-style-cast, reinterpret_cast, static_cast.

dcousens

once over utACK

ryanofsky

utACK 7d29fa4d98ecce780f1b0a2e3004c22c8f4d2395

ryanofsky · 2018-03-21T19:14:47Z

src/serialize.h

That's my understanding too.

Support is added to serialize arrays of type char or unsigned char directly, without any wrappers. All invocations of the FLATDATA wrappers that are obsoleted by this are removed. This includes a patch by Russell Yanofsky to make char casting type safe. The serialization of CSubNet is changed to serialize a bool directly rather than though FLATDATA. This makes the serialization independent of the size of the bool type (and will use 1 byte everywhere).

laanwj · 2018-03-29T20:52:20Z

utACK a7c45bc , good to finally get rid of the FLATDATA eyesore

@ryanofsky

…FLATDATA a7c45bc Add native support for serializing char arrays without FLATDATA (Pieter Wuille) Pull request description: Support is added to serialize arrays of type `char` or `unsigned char` directly, without any wrappers. All invocations of the `FLATDATA` wrappers that are obsoleted by this are removed. This includes a patch by @ryanofsky to make `char` casting type safe. The serialization of `CSubNet` is changed to serialize a `bool` directly rather than though `FLATDATA`. This makes the serialization independent of the size of the bool type (and will use 1 byte everywhere). This is a small change taken from #10785. Tree-SHA512: a41f61ca5fdc2fadb2d0e1702351a58a23841d551f505292a9542602cdb19f90d8944b8df14b872810a56bd201648fa4c0e958f3e9427fe829886284e85b9bfd

@ryanofsky

…ithout FLATDATA a7c45bc Add native support for serializing char arrays without FLATDATA (Pieter Wuille) Pull request description: Support is added to serialize arrays of type `char` or `unsigned char` directly, without any wrappers. All invocations of the `FLATDATA` wrappers that are obsoleted by this are removed. This includes a patch by @ryanofsky to make `char` casting type safe. The serialization of `CSubNet` is changed to serialize a `bool` directly rather than though `FLATDATA`. This makes the serialization independent of the size of the bool type (and will use 1 byte everywhere). This is a small change taken from bitcoin#10785. Tree-SHA512: a41f61ca5fdc2fadb2d0e1702351a58a23841d551f505292a9542602cdb19f90d8944b8df14b872810a56bd201648fa4c0e958f3e9427fe829886284e85b9bfd Signed-off-by: pasta <[email protected]> # Conflicts: # src/test/serialize_tests.cpp

…+ Add file syncing logging and error handling d593e7e Migrate remaining FLATDATA serialization, we are natively supporting it now. (furszy) 051970d addrdb: Remove temporary files created in SerializeFileDB. Fixes non-determinism in unit tests. (practicalswift) 4be426f Add logging and error handling for file syncing (Wladimir J. van der Laan) 8662fb3 Remove unused double_safe_addition & double_safe_multiplication functions. (furszy) a7c3885 Add native support for serializing char arrays without FLATDATA (Pieter Wuille) 459ecb9 Deduplicate addrdb.cpp and use CHashWriter/Verifier (Pieter Wuille) a5d2f8a Fix copypasted comment. (Pavel Janík) Pull request description: Digging down the peers and ban databases commits path in upstream found some good stuff, back ported the following PRs: * bitcoin#9216. * bitcoin#10248. * bitcoin#12740. * bitcoin#13039. * bitcoin#16212. ACKs for top commit: random-zebra: ACK d593e7e Fuzzbawls: ACK d593e7e Tree-SHA512: 8d567c68a2c36f43c749d5faa4b7f8a299dbfdda133495df434ac642c1a2ac96dc2259123ad85decc9039c62c46602665f6be4aadf6d5bf729344c51692ec60e

@ryanofsky

…ithout FLATDATA a7c45bc Add native support for serializing char arrays without FLATDATA (Pieter Wuille) Pull request description: Support is added to serialize arrays of type `char` or `unsigned char` directly, without any wrappers. All invocations of the `FLATDATA` wrappers that are obsoleted by this are removed. This includes a patch by @ryanofsky to make `char` casting type safe. The serialization of `CSubNet` is changed to serialize a `bool` directly rather than though `FLATDATA`. This makes the serialization independent of the size of the bool type (and will use 1 byte everywhere). This is a small change taken from bitcoin#10785. Tree-SHA512: a41f61ca5fdc2fadb2d0e1702351a58a23841d551f505292a9542602cdb19f90d8944b8df14b872810a56bd201648fa4c0e958f3e9427fe829886284e85b9bfd Signed-off-by: pasta <[email protected]> # Conflicts: # src/test/serialize_tests.cpp

fanquake added the Refactoring label Mar 21, 2018

eklitzke approved these changes Mar 21, 2018

View reviewed changes

dcousens reviewed Mar 21, 2018

View reviewed changes

dcousens approved these changes Mar 21, 2018

View reviewed changes

ryanofsky reviewed Mar 21, 2018

View reviewed changes

src/serialize.h Outdated

Copy link

Contributor

ryanofsky Mar 21, 2018

Choose a reason for hiding this comment

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

That's my understanding too.

sipa force-pushed the 201803_chararray branch from 7d29fa4 to a7c45bc Compare March 21, 2018 21:14

laanwj merged commit a7c45bc into bitcoin:master Mar 30, 2018

sipa mentioned this pull request May 17, 2018

Serialization improvements #10785

Closed

str4d mentioned this pull request Jan 29, 2020

util: CBufferedFile fixes zcash/zcash#4265

Merged

furszy mentioned this pull request Apr 9, 2021

Addrdb code deduplication + Native support for FLATDATA + Add file syncing logging and error handling PIVX-Project/PIVX#2304

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add native support for serializing char arrays without FLATDATA #12740

Add native support for serializing char arrays without FLATDATA #12740

Uh oh!

Conversation

sipa commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eklitzke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sipa commented Mar 21, 2018 •

edited

Loading

sipa Mar 21, 2018 •

edited

Loading

ryanofsky Mar 21, 2018 •

edited

Loading

sipa Mar 21, 2018 •

edited

Loading

dcousens Mar 21, 2018 •

edited

Loading