Repository navigation
Serialize BinaryDict with fixed-width fields for word-size-independent OCD output - #1414
Merged
Merged
Conversation
…t OCD output OCD files embed a BinaryDict section whose integer fields were written as native size_t, so 32-bit and 64-bit builds produced different bytes for the same dictionary, and files did not load across word sizes. All fields are now written as uint32_t (matching what 32-bit builds already produced), and the deserializer detects the legacy 8-byte layout, so files written by old 64-bit builds still load, on any platform. Investigation: - PR BYVoid#1372/BYVoid#1373 made the darts trie section word-size independent: vendored darts.h id_type was fixed to uint32_t, the dartsSize field became uint32_t, and a legacy 64-bit reader with validation was added. Those changes covered only the darts array; the trailing BinaryDict section (key/value buffers plus offset tables) kept writing native size_t fields, a layout that dates back to 641bfe9 which deliberately chose size_t to silence MSVC x64 warnings. - As a result, .ocd files produced on 32-bit platforms (including wasm32 and armv7) differed byte-wise from 64-bit output for the same input, and could not be loaded across word sizes at all: BinaryDict::NewFromFile read sizeof(size_t) fields of whatever platform was running. - The existing legacy-format tests could not catch this because SerializeAsLegacy64 copied the BinaryDict section verbatim. On a 64-bit host the fixture carried 8-byte fields and the reader consumed 8-byte fields, so the test was self-consistent yet word-size dependent; the same fixture would have failed on a 32-bit host. - Layout detection reuses the high-bytes-zero probe already proven for dartsSize: an 8-byte numItems always has zero bytes at [4..7] for any realistic dictionary, while the fixed-width layout places keyTotalLength there, which is non-zero whenever the dictionary has entries. The only all-zero-probe fixed-width file is an empty dictionary, exactly 12 bytes long, which is disambiguated by size. - Endianness is intentionally out of scope: the whole OCD/OCD2 family is native-endian (darts array, marisa trie, SerializedValues), so this change keeps BinaryDict consistent with the rest of the format on big-endian platforms rather than fixing one section in isolation. Verification: - bazel test //src/... : all 18 targets pass, including the new binary_dict_test cases (fixed-width layout size assertion, empty-dict round trip, legacy 64-bit loading, out-of-bounds rejection in both layouts) and darts_dict_test with the widened legacy fixture. - dict_converter round trip text -> ocd -> text reproduces the input, and a hex dump of the generated .ocd confirms 4-byte numItems/keyTotalLength/ valueTotalLength and per-entry fields after the darts section. Detailed Changes: - **src/BinaryDict.cpp**: - Write all integer fields as uint32_t; reject values above UINT32_MAX. - Detect the field width on load via DetectFieldWidth and parse both layouts with a single bounds-checked reader. - Route NewFromFile through NewFromBuffer to share the detection logic. - **src/DartsDict.cpp**: - Update layout-detection comments; no behavior change. - **Tests**: - Make crafted BinaryDict files explicit about field width instead of depending on the host size_t, and cover both layouts. - Assert the serialized layout size is fully determined by content. - Cover the empty-dictionary round trip and legacy 64-bit loading. - Widen the BinaryDict tail in SerializeAsLegacy64 so legacy OCD fixtures match real files from old 64-bit builds end to end.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OCD files embed a BinaryDict section whose integer fields were written as native size_t, so 32-bit and 64-bit builds produced different bytes for the same dictionary, and files did not load across word sizes. All fields are now written as uint32_t (matching what 32-bit builds already produced), and the deserializer detects the legacy 8-byte layout, so files written by old 64-bit builds still load, on any platform.
Investigation:
Verification:
Detailed Changes: