Skip to content

feat(libc): implement ffs, ffsl, ffsll, fls and flsll bit search functions - #11494

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
iAbdulrrahman:feat/libc-bit-search-functions
Oct 27, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
iAbdulrrahman:feat/libc-bit-search-functions

Conversation

@iAbdulrrahman

Copy link
Copy Markdown
Contributor

What

Implement FreeBSD/POSIX bit search operations ffs, ffsl, ffsll, fls, and flsll in libc using C++20 std::countr_zero and std::countl_zero.

Tested

Windows 11 x86_64, unit tests in GuestWideStrings.cpp exercising zero, power-of-two, signed boundaries (INT_MIN, INT64_MIN, LLONG_MIN), arbitrary bit patterns, and all-ones (-1) for each routine.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Progress report (88795ed → 91d5a05)

📈 System libraries: 51.12% (+0.02%, +5 implemented functions)

✅ 5 implemented
Library Name
libc ffs_nid_postfix
libc ffsl_nid_postfix
libc ffsll_nid_postfix
libc fls_nid_postfix
libc flsll_nid_postfix

Changed libraries (implemented exports)

Library Before After Change
libc 1373/1384 (99.21%) 1378/1389 (99.21%) +0.00 pp

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the exact head . The bit-index arithmetic is consistent with the BSD / convention: zero returns 0, otherwise indices are one-based; converting signed inputs to the corresponding unsigned width preserves the sign-bit cases. The added cases cover zero, low/high bits, all-ones and signed minima. The conventions check and pass on this head. I could not run locally: the available configured build is relinker-only, and the hosted Linux/Windows product jobs are skipped by path policy. The relinker/ASan matrix is still completing, so those jobs are not evidence for the libc test.\n\nThe branch is based on , while current is . CONTRIBUTING.md asks branches to stay up to date with . The intervening main changes do not touch these three files, and I found no implementation defect in this review, but please sync the branch and rerun the focused test before approval.

@MarcZX10

Copy link
Copy Markdown
Contributor

Correction to my review body above: its inline-code formatting was stripped by the shell, so the displayed text omitted identifiers. The intended findings are: I reviewed head 5b1905f; the ffs/fls bit-index arithmetic is consistent with BSD convention (zero -> 0, otherwise one-based), and the unsigned conversions cover signed-minimum cases. Conventions and git diff --check pass. I could not run guest_wide_strings locally because the available build is relinker-only; hosted Linux/Windows product jobs are skipped by path policy.

Current main is 88795ed, while this branch is based on 48c160c. CONTRIBUTING.md requires branches to stay current. The intervening main changes do not touch these three files. Please sync and run guest_wide_strings; I found no code defect, but the sync/test condition remains before approval.

@iAbdulrrahman
iAbdulrrahman force-pushed the feat/libc-bit-search-functions branch from 5b1905f to 91d5a05 Compare October 26, 2026 17:30
@iAbdulrrahman

Copy link
Copy Markdown
Contributor Author

Synced to current upstream main (88795ed), re-verified conventions check, and confirmed tests pass on the updated head.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for syncing. I fetched exact head 91d5a05; its merge-base is current main 88795ed. I compiled the changed Strings.cpp with the PR header and ran a focused harness comparing all five functions against independent std::bit expectations for 100,000 generated 32/64-bit patterns plus INT_MIN, INT64_MIN and LLONG_MIN. The harness passed. Current-main conventions and git diff --check pass; the published Windows test report is consistent with this result. The hosted relinker jobs are still running and product Linux/Windows jobs are skipped by path policy; those checks do not exercise the libc changes. No defect found; approved.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for syncing. I fetched exact head 91d5a05; its merge-base is current main 88795ed. I compiled the changed Strings.cpp with the PR header and ran a focused harness comparing all five functions against independent std::bit expectations for 100,000 generated 32/64-bit patterns plus INT_MIN, INT64_MIN and LLONG_MIN. The harness passed. Current-main conventions and git diff --check pass; the published Windows test report is consistent with this result. Hosted product Linux/Windows tests are skipped by path policy. No implementation defect found; approved.

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 5021 core/libs/prx/libc/src/Strings.cpp
PR 7851 core/libs/prx/libc/include/Api.hpp
PR 10324 core/libs/prx/libc/include/Api.hpp
PR 10749 core/libs/prx/libc/include/Api.hpp
PR 11341 core/libs/prx/libc/src/Strings.cpp

@MarcZX10

Copy link
Copy Markdown
Contributor

I manually checked the incomplete overlap warnings against the PR-file patches: #11341 changes only the wcstold platform assertions; #5021 changes the integer parsers/QuietNan and does not touch the bit-search additions; #7201 changes the integer-parser wrapper paths; #7851 changes only _Atomic_assert’s declaration; and #9772 adds _Stopfx declarations. None changes the new ffs/fls functions or their assertions, so I see no merge dependency for this patch.

@github-actions github-actions Bot added the libc label Oct 26, 2026
@oneandonlydean oneandonlydean self-assigned this Oct 26, 2026
@oneandonlydean
oneandonlydean merged commit 32906a0 into boykopovar:main Oct 27, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants