Repository navigation
fix(libc): classify x87 unnormals as denormal in _LDtest and _LDclass - #11283
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 85b823e078ab88e33a31bfec5248df7f7df5ec45. The production split matches the stated behavior: TestExtended now treats noncanonical finite exponent/integer-bit encodings like the console _LDtest, while __fpclassifyl retains its separate exponent-based behavior. The remaining blocker is a test prerequisite: the PR says these noncanonical-value checks need #10888 because AppleClang folds Extended() into a canonical long double; #10888 is still open, and current main’s helper has no noinline attribute. Yet this checklist says Depends on: none. Please list #10888 as a prerequisite or make the test construction self-contained in this PR, and verify the exact resulting head on macOS. The author reports the 20-million-encoding comparison and Linux focused tests; I have not independently rerun that binary harness. Hosted policy/conventions pass, Ubuntu Debug/Release register-context pass, and cross-platform relinker/ASan plus macOS checks are still pending; Linux/Windows full product jobs are skipped.
|
Windows run of head
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
The main code change and the Windows/Linux reports still support the |
|
@MarcZX10 #10888 is merged now (bb61ff9), so |
|
Windows run of this head
AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
The classification change is narrowly scoped, and the added cases exercise an unnormal, negative pseudo-zero, and largest unnormal against the bundled module's reported results. I am not approving the submitted head yet: its base is acb900c, and current main is 3fea2a9. Main has since changed GuestMathExact.cpp, including the macOS pseudo-denormal fixture adjustment; that is the test file this PR extends.
Please rebase onto current main and rerun guest_math_exact on the rebased tree (including the available macOS coverage). I found no separate implementation defect in MathExact.cpp, which has not changed on main since the PR base; this request is to validate the updated test integration before approval.
85b823e to
8130d34
Compare
|
@MarcZX10 Rebased onto macOS: I have no Mac, and none of the hosted macOS jobs (relinker, register-context, write-watch) builds |
|
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 #11270, #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 8130d34. The production change preserves the separate exponent-based behavior of __fpclassifyl while making TestExtended match the bundled _LDtest treatment of unnormals and pseudo-zeros. I found no separate logic defect.
Independent focused check: I applied the PR's two changed files to the clean #11213-based Release checkout, rebuilt guest_math_exact_tests, and ran guest_math_exact three times; all passed. This reproduces the Linux test on the PR's 1089f2b base plus unrelated #11213 changes. A current-main merge preview is clean, but the PR branch is still 30 commits behind current main, and the report does not include an AppleClang run of guest_math_exact. The PR itself notes that none of the hosted macOS jobs builds this target; the new noncanonical long double fixtures were added specifically because AppleClang can canonicalize them.
Please update the branch to current main and provide a run of the exact guest_math_exact cases under AppleClang (or wire this target into available macOS CI). The current hosted macOS checks do not establish that these encodings survive the compiler on that platform.
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the current head 8130d34. The classifier split matches the bundled _LDtest behavior while preserving __fpclassifyl's separate exponent-based behavior; I found no logic defect.
Independent Linux check: applied the PR's MathExact.cpp and GuestMathExact.cpp changes to the clean #11213-based Release checkout, rebuilt guest_math_exact_tests, and ran guest_math_exact three times; all passed. I did not test under AppleClang. Current main's merge preview is clean, and the files this PR edits have not changed since its 1089f2b base, so the earlier branch-currency concern is resolved.
The remaining validation gap is specific to this test: the PR says its noncanonical long-double fixtures need the noinline construction fix for AppleClang, but no available macOS CI job builds guest_math_exact. Please run these cases with AppleClang or add this target to macOS CI before approval.
|
A runtime differential of this head against the Setup:
Results:
AI-assisted: yes (Claude Code). |
|
@MarcZX10 AppleClang run of the exact
The Tested section now lists these runs. |
MarcZX10
left a comment
There was a problem hiding this comment.
The earlier AppleClang validation gap is now addressed. The author ran the exact guest_math_exact cases on a macOS 15 Intel runner with Apple clang 17: three runs passed on this head, while the control built with main's MathExact.cpp aborts on the same noncanonical encodings. The unrelated guest_math abort also reproduces on main. This is useful evidence for the compiler-specific fixture concern I raised.
My focused GNU build and repeated guest_math_exact runs passed in the earlier review; the current-main merge preview is clean and hosted checks are green, with the Linux product job skipped. I found no remaining blocker in the x87 classification change. Approved.
|
Checked the diff against the bundled |
What
_LDtestand_LDclasscalled every 80-bit encoding with a nonzero, non-maximal exponent finite (−1). That includes the unnormals (integer bit clear) and the pseudo-zeros (zero mantissa). The console calls both denormal (−2).The evidence is the
libc.prxbundled with PPSA21564, the library's own code._LDtest(A+1YXWOGpuo, 0x108a0) decides from the 16-bit words of the value:guest_math_exactalready checks._LDclass(hdcGjNpcr4w, 0x54660) copies its argument withfld/fstpand returns_LDtestof the copy, sign-extended.The console's
__fpclassifyl(zwV79ZJ9qAU, 0x54830) is separate code that never reads the integer bit:AnyPS5's
__fpclassifylalready returned exactly that, but throughTestExtended. Now thatTestExtendedfollows_LDtest,__fpclassifylclassifies by the exponent itself, so its results are unchanged.__isinfland__isnanlonly compareTestExtendedwith infinite and NaN, which this does not touch.Tested
On Ubuntu 24.04 under WSL2 (GCC 13.3, Release), head 8130d34 on
main1089f2b unless noted:libc.prx. I used the harness from fix(libc): compute acosl with FreeBSD msun's ld80 code as the bundled libc.prx does #11260, with thatlibc.prx's segments mapped into a Linux process. It ran the binary's_LDtest,_LDclass,__fpclassifyl,__isinfland__isnanlnext to AnyPS5's on 20,000,000 random 80-bit encodings. Three quarters of them were biased towards zero and all-ones exponents, a clear integer bit, short mantissas, zero mantissas and lone integer bits.main:_LDtestand_LDclassdiffer on 11,228,922 of the encodings, and the other three on none.guest_math_exactpasses.CheckClassificationgains three non-canonical encodings: an unnormal0x4000000000000000:3fff, a negative pseudo-zero0:8001, and the largest unnormal0x7fffffffffffffff:7ffe. For each it requires_LDtestand_LDclassto return −2, and__fpclassifyland__isnormallto return −1 and 1, as the binary does. The test aborts onmain.Extended()helper as the existing pseudo-denormal, whichmainnow marks[[gnu::noinline]](fix(tests): keep guest_math_exact's pseudo-denormal out of constant folding #10888, 38a570a) so AppleClang does not fold its result into a canonical value. This head includes that change, andguest_math_exactpasses 3 runs on it. None of the hosted macOS jobs buildsguest_math_exact, so I ran it with a fork-only workflow on GitHub'smacos-15-intelrunner (x86_64, Apple clang 17.0.0, clang-1700.0.13.5, Release):guest_math_exactpasses 3 runs (run).main'sMathExact.cpp(43de474), it aborts (run), so the three encodings reach the classifier unchanged under AppleClang.guest_mathaborts on that runner atmain1089f2b too, with only the workflow added (run), so that abort is not from this PR.guest_mathpasses (3 runs).python3 tools/check_conventions.py --base origin/mainpasses.python3 tools/pr_overlap.py --branch fix/libc-ldtest-unnormalsreports no overlap. feat(libc): add Dinkumware float bit helpers from firmware #8397 and feat(libc): implement _LExp #10780 also touchMathExact.cpporGuestMathExact.cpp, andgit merge-treeshows no conflict with either._LExpresults withTestExtended. Those results are always canonical, so this change does not affect them.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)