Repository navigation
fix(libc): return the sign mask from __signbit, __signbitf and __signbitl - #11278
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head d58ce8353d3a439117811643cc0cfc1fe6c0a3fb. The 0x8000 sign mask matches the bundled libc.prx instruction sequences described in the report, and the new guest cases cover positive/negative zero, finite values, infinities and NaNs across float, double and long double. The PR reports exhaustive comparisons against the bundled binary; I did not independently rerun that harness. Its dependency on #11277 is recorded. No blocking code finding. Policy and conventions checks pass; full product builds are skipped under the PR path policy.
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
I found an unreported behavior change in the diff beyond the three sign-mask exports. __fpclassifyf_nid_postfix now returns _FDtest_nid_postfix's internal -2/-1/0/1/2 codes, and CheckFloatClassification was changed to expect those codes. Before this PR, the export mapped those categories to the project's FP_SUBNORMAL/FP_NORMAL/FP_ZERO/FP_INFINITE/FP_NAN values (0x08/0x04/0x10/0x01/0x02). That changes an independent public result for every classification and is not supported by the signbit disassembly or the reported differential tests, which compare only __signbit, __signbitf, and __signbitl.
Please restore the existing __fpclassifyf mapping in this signbit PR, or provide direct evidence for the console's __fpclassifyf return codes and update the scope/report and tests to cover that behavior. My earlier approval did not catch this collateral change; I am correcting it now.
MarcZX10
left a comment
There was a problem hiding this comment.
Correction to my immediately preceding review: I attributed the __fpclassifyf change to this signbit commit because GitHub shows the full stack diff against main. The exact commit diff confirms that the classification change is entirely in prerequisite #11277, whose PR provides the libc.prx disassembly and exhaustive float differential; this PR's commit only changes the three sign-mask exports and their tests. #11278 correctly records Depends on: #11277. The concern in my prior review was mistaken, and I withdraw it. The existing signbit evidence, test coverage and completed hosted matrix support approval.
|
Windows run of head
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
Windows run of this head
AI-assisted: yes (Claude Code) |
|
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 other open libc PRs #11260, #11262, #11266, #11632, #11634, #11270, #11283, #10929, all merged cleanly, so the run is a joint one; the tests of this PR are AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
|
A runtime cross-check on 11 more titles. A scratch loader runs each decrypted All 11 export all three, and every one returns:
That matches the description's table. On PPSA07923, 200,000 random AI-assisted: yes (Claude Code). |
d58ce83 to
def9214
Compare
BunnyFl
left a comment
There was a problem hiding this comment.
Approving head def9214119. I read the diff and ran firmware 13.42's __signbit, __signbitf and __signbitl: the decrypted system libSceLibcInternal.sprx's exports, natively on a PC (not a console). Three identical runs.
For -1, -0, -inf, a negative NaN and the smallest negative subnormal, and for 1, 0, inf, a positive NaN and 1e300, all three return 0x8000 when the sign bit is set and 0 otherwise. That is what this change returns and what the new and updated test rows expect, so the system module agrees with the PPSA21564 libc.prx reading in the description.
What
__signbit,__signbitfand__signbitlreturned 1 for a negative argument, following FreeBSD'ssignbit(#3640). The console's versions return the sign bit where it sits in the top 16-bit word of the value: 0x8000 for any argument whose sign bit is set, 0 otherwise. That is also what AnyPS5's_LDsignalready returns.A title that only tests for nonzero is unaffected. A title that compares the result, stores it, or shifts it saw a different value from the console's.
The evidence is the
libc.prxbundled with PPSA21564, the library's own code:__signbitRw4J-22tu1Uvmovmskpd eax, xmm0; shl rax, 63; shr rax, 48__signbitfCjQROLB88a4vmovmskps eax, xmm0; shl eax, 31; shr eax, 16__signbitlCj81LPErPCcmovzx eax, word [rsp+0x10]; and eax, 0x8000NaNs follow their sign bit like every other value.
Tested
On Ubuntu 24.04 under WSL2 (GCC 13.3, Release), head def9214 (this PR rebased onto
maina13ee2d):Native comparison. I used the harness from fix(libc): compute acosl with FreeBSD msun's ld80 code as the bundled libc.prx does #11260, with that
libc.prx's segments mapped into a Linux process, and ran the binary's three functions next to AnyPS5's. There are 0 mismatches with this PR:__signbitfon all 4,294,967,296floatbit patterns;__signbiton 20,000,000 randomdoubleencodings, half of them biased to zero, subnormal, infinite and NaN exponents;__signbitlon 20,000,000 random 80-bit encodings, including unnormals, pseudo-denormals, pseudo-infinities and pseudo-zeros.On
main, every negative input differs: 2,147,483,648floatpatterns, 10,003,161 doubles and 10,002,538 long doubles.guest_mathpasses (3 runs). Its__signbitlchecks now expect 0x8000. New checks cover__signbitand__signbitfon signed zeros, a subnormal, infinity and NaNs of both signs.Conventions.
python3 tools/check_conventions.py --base origin/mainpasses.Overlap.
python3 tools/pr_overlap.py --branch fix/libc-signbit-sign-maskreports no line overlap. It could not compare 29 PRs that conflict withmain; each gives the samegit merge-treeconflicts with this branch as withmain.Stacking. The branch was stacked on fix(libc): return Dinkumware codes from __fpclassifyf #11277, because
__signbitfis the line after the__fpclassifyfthat PR rewrites. fix(libc): return Dinkumware codes from __fpclassifyf #11277 has merged, so only this PR's commit is left.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)