Repository navigation
fix(libc): compute _LLog with the bundled libc.prx's Dinkumware code - #11270
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
The port's main value paths are carefully compared against the bundled binary, but the implementation and its regression test do not enforce the x87 precision requirement that the PR itself records in docs/dev/debt/libc.md. _LLog performs its normalization, division and polynomial arithmetic as long double; on x87, a caller's 53-bit precision-control setting can round those intermediates before the final 80-bit result is stored. The reported harness comparison appears to use its default control word, and TestLongLog never changes or verifies the x87 control word. Please add a scoped 64-bit-precision environment around the calculation, restoring the caller's original control word on every return, and add a regression that invokes representative cases under 53-bit precision and checks restoration. If the console contract instead preserves the caller's precision mode, please provide evidence for that and test the expected results under both modes. Policy/convention checks pass; hosted product jobs are still pending or skipped, and I have not independently run the author's local comparison.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Windows run of head
AI-assisted: yes (Claude Code); results above were run on this machine. |
fa65064 to
dcb89d6
Compare
|
@MarcZX10 The console keeps the caller's precision here too, so I added evidence and tests at both precisions instead of forcing 64 bits:
I also rebased onto @wachi35676 Thanks for the Windows thread check. AnyPS5 starts guest threads on Windows with |
dcb89d6 to
ecd40b3
Compare
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
MarcZX10
left a comment
There was a problem hiding this comment.
The _LLog port has unusually strong reference evidence in the PR description, including results, errno, and exception flags across multiple control words. I am not approving the current submitted ref yet: its merge-base with main is 8df2c3e, while the Tested section says the run was on main 5b5d24c. Current main is 3fea2a9, and docs/dev/debt/libc.md has changed since the submitted base.
Please update the branch and debt entry against current main, and reconcile the test report with the exact submitted head. The implementation and test files have not changed on main since this base, so I found no separate code regression; this is a branch-currency and evidence-to-head issue required before approval.
ecd40b3 to
4edd28e
Compare
|
@MarcZX10 Rebased onto
|
|
Windows result for head Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, The tree also carried the open libc PRs #11283, #11632, #11634, all merged cleanly, so the run is a joint one; the test of this PR is AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the current head 4edd28e. The normalization, zero/negative handling, split constants, and base selection are consistent with the behavior described from the bundled _LLog. I found no separate implementation defect.
Independent focused check: I applied this head's DinkumwareInternals.cpp, GuestDinkumwareInternals.cpp, and debt changes to the clean #11213-based Release checkout, rebuilt guest_dinkumware_internals_tests, and ran guest_dinkumware_internals three times; all passed. The source and test files this PR changes have not changed on current main since the PR's 1089f2b base, and a merge preview with current main is clean. This supports the submitted results, but does not satisfy the repository's current-main branch requirement: the PR branch is still 30 commits behind the current main tip.
Please rebase/update the branch onto current main and let the checks run on that resulting head. The earlier evidence-to-head concern is resolved by the updated report and the focused local run; the remaining request is branch currency before approval.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 4edd28e. The _LLog implementation's normalization, polynomial evaluation, exceptional-value behavior, split constants, and base selection match the behavior described from the bundled binary. I found no specific implementation defect.
Independent local check: applied this PR's DinkumwareInternals.cpp, GuestDinkumwareInternals.cpp, and debt changes to a clean #11213-based Release checkout, built guest_dinkumware_internals_tests, and ran guest_dinkumware_internals three times; all passed. The current-main merge preview is clean, and main has not changed the files this PR touches since its 1089f2b base. The additional multi-precision, errno, flags, and native binary comparisons in the PR description are author-reported; I did not rerun that harness.
I approve this implementation and focused test evidence.
|
A runtime differential of this head against the Setup:
Results:
Only positive finite AI-assisted: yes (Claude Code). |
32848d0 to
2eb8d8d
Compare
2eb8d8d to
268b424
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
The native _LLog differential is extensive, but the reviewed head is based on main 3a2b919816, 36 commits behind current main 52c09d567d2fa453494dd720fcf53a98149468e4. The repository checklist requires a current-main base. The reported differential and focused test therefore need to be repeated or confirmed on the refreshed integration before approval.
What I ran
- Reviewed exact head
268b424b3d91f1f95854025f636eb9783897a986against currentmain. - Ran
git merge-tree --write-treeandgit diff --check. - Checked the native comparison, focused test report, and current hosted checks.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean; current main is not in the branch history |
git diff --check |
Passed |
| Native comparison | Author reports 0 mismatches against bundled libc.prx for the tested arguments and floating-point precisions on the older base |
| Focused test | Author reports guest_dinkumware_internals passes three runs on that head |
| Current-main run | Not reported |
Please update onto current main and rerun the focused math tests and native differential on the integrated head. I did not run the test or native comparison locally.
268b424 to
7b6c2e8
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
The update reports exact-head guest_dinkumware_internals and guest_math passes, plus the native _LLog comparison. The branch is based on 1b580f13cf; current main is ba5b5330b3 and has changed the libc debt file this PR edits. Please merge current main and rerun conventions and the focused tests on the resulting exact head. The report is useful evidence for 7b6c2e82ec, but it does not establish the required current-main integration.
|
Checked the port against the described |
What
_LLog(x, base)dispatched to the hostlogl/log10l/log2l. On 4 million random arguments, those differ from the console's result on 869,197, 2,000,696 and 306,649 respectively; glibc'slog10l(1000), for example, is not exactly 3 while the console's is. This PR ports the_LLogof thelibc.prxthat PPSA21564 bundles (0x3fd50, the library's own code). That function is Dinkumware'sxlog; the same binary'slogl,log10landlog2lare one-call wrappers passing 0, 1 and −1.What the binary does, and what the port reproduces:
_LDunscale(0x24e0). An all-ones exponent is NaN or ∞. A zero mantissa counts as zero whatever the exponent, so pseudo-zeros do too. Otherwise the mantissa is normalized into [0.5, 1) with exponentmax(e, 1) - shift - 0x3ffe, which also covers denormals and unnormals._Feraise(FE_DIVBYZERO)(ERANGE) and −∞. NaN and +∞ returnx. A negative argument, −∞ included, gives_Feraise(FE_INVALID)(EDOM) and the positive quiet NaN0xc000000000000000:7fff.0xb504f333f9de6484:3ffe), double it and decrement the exponent. Theny = x − 1,z = y/(x + 1),w = z²,p=_LPoly(w)over the 8 coefficients at 0x15ea90 (Horner from the first), andq = z*(y − p*w).floathigh part, exactly as the binary groups them.(y − (q + c₁e)) + e·c₂.((e₂y − (qL + d₁e)) + y·f₁) + e·c₃.((q·k + g·y) + y·h) + e._Feraise(0x11b00) is whylog(0)setsERANGEand negative arguments setEDOM.x87 precision. The binary's
_LLog, and the_LDunscaleand_LPolyit calls, have no instruction that reads or sets the x87 control word (nofnstcw,fldcw,fldenvorfninit), so it computes at the caller's precision and leaves the control word as it found it. The port does the same and needed no change for it: its results match the binary's at 64, 53 and 24 bits (below).The
_LLogdebt entry now records the evidence and what remains: it runs at the caller's precision, so a guest thread that starts at 53 bits (the thread-start gap theasinlentry records) gets the binary's 53-bit result, not the console's 64-bit one; andlogl/log10l/log2lstill call the host. Wiring those up would touch the long-double debt line that #11260–#11266 already edit, so it is left for a follow-up.Tested
On Ubuntu 24.04 under WSL2 (GCC 13.3, Release), head 7b6c2e8 on
main1b580f1:_LLog_nid_postfixin this head'slibc.prx. I used the harness from fix(libc): compute acosl with FreeBSD msun's ld80 code as the bundled libc.prx does #11260 with the.text, read-only, RELRO and.datasegments mapped. The__stack_chk_guardand__errorGOT entries point at host variables, so the binary's_Feraisepath runs for real. The port matched the binary's 80-bit result,errnoandFE_INVALID/FE_DIVBYZEROflags on every case, 0 mismatches, at each of 64-, 53- and 24-bit precision (control words 0x37f, 0x27f and 0x07f, set before both are called):guest_dinkumware_internalspasses (3 runs on this head, as doesguest_math). It gainsTestLongLog: 22 known-answer pairs from that binary, including six where the host differs. Examples arelog10(1000) = 3exactly and0xecad15fd3a0eaf66:3ffd → 0xc589acfa7c6c1534:bffeunder base 0, where the host gives…35. It also checks −∞, ERANGE andFE_DIVBYZEROfor four zero encodings, and the positive NaN,EDOMandFE_INVALIDfor −1, −denormal and −∞.TestLongLogAt53Bitsruns seven of the pairs at 53 bits under bases 0, 1 and −1: each must give the binary's 53-bit result (for examplelog(8)is0x851591f9dd5b9800:4000instead of0x851591f9dd5b9b41:4000) and leave the control word unchanged. The existing_LLogchecks still pass, and so doguest_mathandguest_math_exact.python3 tools/check_conventions.py --base origin/mainpasses.python3 tools/pr_overlap.py --branch fix/libc-llog-dinkumwarereports fix(libc): parse scaled NaN subjects without numeric conversion #12659, which inserts a new entry directly above the_LLogline this PR rewrites. This PR was approved first, so it lands first, and I asked on fix(libc): parse scaled NaN subjects without numeric conversion #12659 for it to follow (Depends on: #11270). fix(libc): match title libc.prx for hexadecimal strtod underflow #12291, which rewrote the_Stodline above, has merged, and this branch is rebased onto it. fix(libc): parse scaled NaN subjects without numeric conversion #12659 now declaresDepends on: #11270. The 25 pull requests the tool could not compare give the samegit merge-treeconflicts with this branch as withmain. feat(libc): implement _Exp and _FExp #10786 and docs(debt): cut the long-double Dinkumware entry to _LLog with the bundled libc.prx code #11217, which touched the same code earlier, have merged.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)