U64toa 单元测试用例不通过 #123

Activity

1. lkkey80

   changed the title [-]U64toa 单元测试用例结果不正确[/-] [+]U64toa 单元测试用例不通过[/+] on Jan 21, 2026
2. tatictraian

   added a commit that references this issue on Feb 4, 2026

   fix: resolve multiple issues - itoa bug, sorted keys, SetRaw API, mem…

   ...

   This issue will close once commit a46d20c is merged into the 'master' branch.

   a46d20c
3. linhongyu510 commented on Sep 17, 2026

   

   linhongyu510

   on Sep 17, 2026

   More actions

   I tried to reproduce this and could not on Apple clang 21 / arm64, but the investigation turned up something that may explain the GCC 8.3.1 result — and a separate detail about U64toa's buffer contract that seems worth recording either way.

   Not reproducible with clang -O2

   Compiling the real U64toa from include/sonic/internal/itoa.h at -O2, both reported values are correct, along with every leading-1 length from 1 to 20 digits:

   12345                -> 12345                len=5  OK
   1234567890123        -> 1234567890123        len=13 OK
   18446744073709551615 -> 18446744073709551615 len=20 OK
   mismatches: 0
   

   Also clean under -fsanitize=address,undefined. So this is not architecture-independent, and I cannot confirm or deny the GCC 8.3.1 behaviour — I have no GCC on this machine (gcc here is an Apple clang shim). Treating that as a limitation, not as evidence against the report.

   Why 10345 is a plausible miscompile symptom, not a random corruption

   The reported 12345 → 10345 corrupts exactly the second byte, and 1234567890123 → 1034567890123 corrupts exactly the same position. That is not arbitrary — it is the byte the leading-zero-skip trick deliberately writes and expects to be overwritten.

   For 12345, Utoa_1_8 takes the 5–6 digit branch with hi = 1, so lz = 1 and it copies two bytes from kDigits[hi*2 + lz] = index 3:

   kDigits[0..7] = "00010203"
                index 3,4 = '1','0'
   

   It then advances only 2 - lz = 1 byte, so the stray '0' must be overwritten by the following Copy2Digs. The reported output is precisely what you get if that subsequent write does not land — the '1' survives and the '0' is never replaced.

   That makes the symptom consistent with the compiler mis-ordering or eliding one of the overlapping 2-byte stores, rather than with a logic error in the branch. Two things support that reading:

   • itoa.h:48-49 already carries #pragma GCC diagnostic ignored "-Warray-bounds" around exactly this function, so GCC is known to be unhappy about these accesses;
   • the reporter says removing -fsanitize=address is what exposes it, which is the classic signature of an optimisation-dependent issue rather than a plain bug.

   If that is right, -fno-strict-aliasing or making Copy2Digs write through a memcpy of 2 bytes would be the things to try on that toolchain. I can't test either claim here, so I'd rather flag the hypothesis than assert it.

   Separate finding: U64toa writes past its returned end pointer

   While testing I sized buffers exactly to the digit count and let ASan watch. U64toa overruns for some lengths:

    1 digit   -> needs 1 byte of slack
    2..8      -> 0
    9..16     -> needs 8 bytes of slack
   17..20     -> 0
   

   The 9–16 range is the Utoa_8 path; ASan reports the offender as a WRITE of size 16, i.e. a vectorised store covering more than the digits produced.

   This is not a live bug — I checked the real callers before concluding anything. serialize.h:94 does wb.Grow(kNumberSize) with kNumberSize = 33 (serialize.h:45), which comfortably covers 20 digits plus the widest overrun measured. And tests/itoa_test.cpp already comments "reserved buffer should be 32 bytes". So the over-write is an intended part of the contract.

   The reason I mention it: that contract is currently expressed only in a test comment and an implicit 33. Anyone calling U64toa directly from new code — it is in internal, but the header is shipped — would reasonably size a buffer to the return value and get a heap overflow for 9–16 digit values. A one-line comment on U64toa stating the minimum reservation would make that safe to use.

   Happy to send a PR for just that comment if you think it is worth having. I have deliberately not touched the algorithm, since the reported failure is toolchain-specific and I cannot verify a fix for it here.

   Probes used are self-contained if anyone wants to re-run them on GCC 8.3.1 — that is the one data point that would settle the original report.
4. linhongyu510 commented on Sep 18, 2026

   

   linhongyu510

   on Sep 18, 2026

   More actions

   Following up on the U64toa buffer question I raised earlier — I went and measured it, and there is no bug there. Recording the numbers so nobody else has to re-derive them:

   I instrumented the write extent with a canary buffer under ASan, across double max/min, denormals, 1e±300, u64 max, i64 min and the branch boundaries, in both default and kSerializeFloatFormatJava modes. Worst case touched 24 bytes, against the 33 that serialize.h reserves via wb.Grow(kNumberSize). So the slack the digit-copy relies on is comfortably covered, and my earlier concern about the overwrite-dependent Copy2Digs was unfounded for every real call site.

   I also round-tripped F64toa through strtod over ~840k values (random bit patterns, powers of ten and their nextafter neighbours, hand-picked hard cases) in both float formats, comparing bit patterns: zero mismatches.

   The original GCC 8.3.1 -O2 report stands as I described it before — I still cannot reproduce it, having no real GCC on this machine, and that remains an environment limitation rather than a refutation.

   While auditing that path I did find an unrelated, reproducible defect in the JSONPath on-demand backend and opened #152 for it: a negative array index returns kErrorNoneNoMatch, the same code as an absent key, so callers cannot distinguish "unsupported path" from "member not present". That one is independent of this issue.