-
Notifications
You must be signed in to change notification settings - Fork 16
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
BitSet serialization error for MSB #24
Comments
Test code to demonstrate this is https://github.com/mdavidsaver/pvDataCPP/blob/testbitsetserialize/testApp/misc/testBitSet.cpp#L196, which depends on #17. |
Actually, this is not a bug. The serialization might not be the nicest (from human perspective), but it is efficient.
First whole long-s (64-bit) are serialized in selected byte order, then the remaining in LSB -> MSB (regardless of endianness). |
@msekoranja the code you quote is quite reasonable by itself, as is the code in deserialize(). This issue is that they are inconsistent. Please run the test code. |
I see now. C++ BitSet serialization was not consistent w/ the rest of implementation (deserialization and Java code). The idea is if length of bitSet structure (in bytes) is multiple of sizeof(uint64) then the bitSet should be serialized only as an array of longs (from LS to MS). If not, the remaining bytes are added (LSB -> MSB). I will create a pull request to pvDataCPP. |
Meaning that the bug is limited to C++ serializing for BE destination.
... based on how you would choose to resolve this issue. This proposed fix would change how LE targets behave, which is technically a compatibility break. It would be nice to know if anyone actually has type definitions affected by this bug* (my guess is no). In the absence of this information my inclination is then to let Matej fix this as he wishes.
|
@msekoranja (as you know) I sent around a mail yesterday with a request for testing to find if anyone is actually affected by this issue. Assuming there are no positive responses this week I'd like to see your fix merged. |
I agree. What do others say (e.g. @dhickin - the module owner)? |
check for definitions affected by a BitSet serialization issue epics-base#24 track number of fields in largest structure/union seen so far.
Fixed by #25 |
check for definitions affected by a BitSet serialization issue epics-base#24 track number of fields in largest structure/union seen so far.
When I discovered my mistakes in #22 I did what we should all do, which is write test code. In the process I discovered a nasty little bug with serialization of BitSet. As an optimization the high element is serialized as bytes.
Unfortunately, in the case where a bit is set in the high byte of the high word, and the number of bytes to be serialized is a multiple of 8, the logic of serialize() calls putByte() eight times (LSB first) while deserialize() calls getLong() once. On little endian systems this makes no difference, but on big endian systems this is backwards.
To avoid a wire protocol compatibility break on LE systems the fix will almost certainly be to change deserialize().
The text was updated successfully, but these errors were encountered: