Repository navigation
Fix legacy 64-bit OCD layout detection on big-endian platforms - #1472
Merged
Merged
Conversation
The BinaryDict and DartsDict deserializers detected the legacy 64-bit layout by checking whether probe bytes [4..7] are zero, which only holds on little-endian hosts; on big-endian platforms (s390x, ppc64, sparc64, hppa) legacy files were misread as fixed-width and loaded as empty dictionaries. Replace the byte-offset idiom at all three sites with a shared LooksLikeLegacy64Field() helper that loads the probe as a native uint64_t and checks its high 32 bits, which is correct on both byte orders and compiles to the same behavior on little-endian. Verified with BinaryDictTest and DartsDictTest on arm64 and on emulated s390x. Fixes BYVoid#1471
Contributor
|
Tests on Debian side can confirm things to be working. Thanks! |
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
2 participants
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.
Fixes #1471.
Problem
The BinaryDict and DartsDict deserializers detect the legacy 64-bit OCD layout by checking whether probe bytes
[4..7]are all zero. That assumes the high half of a nativeuint64_tsits at bytes[4..7], which is only true on little-endian hosts. On big-endian platforms (s390x, ppc64, sparc64, hppa) the high half is at bytes[0..3], so a legacy file is always misdetected as fixed-width: the firstuint32_tread picks up the zero high half,numItems/dartsSizecome out as 0, and the dictionary silently loads as empty. This is what breaks the five legacy-layout tests on Debian's big-endian buildds.Fix
Replace the byte-offset idiom at all three sites (
BinaryDict.cppDetectFieldWidth,DartsDict.cppNewFromFileandNewFromBuffer) with a sharedLooksLikeLegacy64Field()helper in the private headerBinaryDict.hpp: load the 8-byte probe as a nativeuint64_tviamemcpyand check(word >> 32) == 0. Since legacy files are written in native byte order by construction, a native load recovers the original value on either byte order:word >> 32is exactly bytes[4..7]— identical to the current behavior, zero change.uint32field (numItems/dartsSize), non-zero for any non-empty dictionary. The empty-dictionary corner case is already excluded by the existingsize != 3 * sizeof(uint32_t)guard.No changes to the legacy read paths themselves (
ReadField,LegacyUnit64,ValidateLegacy64) — those already do native loads of natively-written data. Legacy.ocdfiles remain same-byte-order-only by construction;.ocd2is unaffected.Verification
//src:binary_dict_testand//src:darts_dict_testpass via Bazel, 22/22 before and after.BinaryDictTest10/10 andDartsDictTest12/12 pass, including the five tests failing on the Debian buildds (AcceptsLegacy64BitLayout,RejectsHugeValueTotalLength,LoadsLegacy64bitFormat,Legacy64PrefixSearchBeyond64Matches,Legacy64SerializeToFileThrows). The test fixtures are generated at runtime with native writes, so they exercise the real big-endian legacy layout.@hosiet a confirmation run on a real Debian porterbox would still be welcome.