Add native support for serializing char arrays without FLATDATA

sipa commented at 0:40 am on March 21, 2018: member

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.

fanquake added the label Refactoring on Mar 21, 2018

in src/serialize.h:63 in 7d29fa4d98 outdated

58@@ -59,6 +59,12 @@ inline T* NCONST_PTR(const T* val)
59     return const_cast<T*>(val);
60 }
61 
62+//! Safely convert odd char pointer types to standard ones.
63+static inline char* CharCast(char* c) { return c; }

eklitzke commented at 1:02 am on March 21, 2018:

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

sipa commented at 1:16 am on March 21, 2018:

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.

ryanofsky commented at 7:14 pm on March 21, 2018:

That’s my understanding too.

sipa commented at 8:48 pm on March 21, 2018:

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.

sipa commented at 9:14 pm on March 21, 2018:

Fixed.

ryanofsky commented at 9:27 pm on March 21, 2018:

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.

sipa commented at 9:33 pm on March 21, 2018:

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.

ryanofsky commented at 9:59 pm on March 21, 2018:

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).

sipa commented at 10:20 pm on March 21, 2018:

@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.

eklitzke approved

eklitzke commented at 1:02 am on March 21, 2018: contributor

utACK 7d29fa4d98ecce780f1b0a2e3004c22c8f4d2395

in src/serialize.h:66 in 7d29fa4d98 outdated

58@@ -59,6 +59,12 @@ inline T* NCONST_PTR(const T* val)
59     return const_cast<T*>(val);
60 }
61 
62+//! Safely convert odd char pointer types to standard ones.
63+static inline char* CharCast(char* c) { return c; }
64+static inline char* CharCast(unsigned char* c) { return (char*)c; }
65+static inline const char* CharCast(const char* c) { return c; }
66+static inline const char* CharCast(const unsigned char* c) { return (const char*)c; }

dcousens commented at 3:13 am on March 21, 2018:

why not reinterpret_cast?

sipa commented at 3:21 am on March 21, 2018:

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

dcousens approved

dcousens commented at 3:14 am on March 21, 2018: contributor

once over utACK

ryanofsky commented at 7:19 pm on March 21, 2018: member

utACK 7d29fa4d98ecce780f1b0a2e3004c22c8f4d2395

Add native support for serializing char arrays without FLATDATA

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).

a7c45bce92

sipa force-pushed on Mar 21, 2018

laanwj commented at 8:52 pm on March 29, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12740/commits/a7c45bce9264f4a8fa48f2e7ecaf102971fc026f , good to finally get rid of the FLATDATA eyesore

laanwj merged this on Mar 30, 2018

laanwj closed this on Mar 30, 2018

laanwj referenced this in commit 8203c4c42e on Mar 30, 2018

PastaPastaPasta referenced this in commit abbf299a05 on Sep 27, 2020

PastaPastaPasta referenced this in commit ca268bd678 on Oct 22, 2020

random-zebra referenced this in commit 116bb50765 on Apr 20, 2021

gades referenced this in commit cbabeb575a on Jun 24, 2021

MarcoFalke locked this on Sep 8, 2021

Add native support for serializing char arrays without FLATDATA #12740