Skip to content

feat(libc): implement ffs, ffsl, ffsll, labs and imaxabs - #1977

Closed
Pedroj-64 wants to merge 2 commits into
boykopovar:mainfrom
Pedroj-64:feat/libc-ffs-labs
Closed

Pedroj-64 wants to merge 2 commits into
boykopovar:mainfrom
Pedroj-64:feat/libc-ffs-labs

Conversation

@Pedroj-64

@Pedroj-64 Pedroj-64 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

What

ffs, ffsl, ffsll, labs and imaxabs were not exported.

  • ffs* return the 1-based index of the least significant set bit, 0 for 0.

  • labs and imaxabs negate in unsigned arithmetic, so the minimum value maps to itself instead of overflowing.

  • core/libs/prx/libc/src/Math.cpp: the five functions.

  • guest_math test: zero, negative values, bits above 32 and the minimum values.

Tested

Linux (CachyOS), GCC 16.2.1, Release.

  • The full suite (494 tests) passes on this branch.
  • tools/check_conventions.py --base upstream/main passes.

Not tested on Windows or macOS.

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 in TechnicalDebt
  • 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
  • AI-assisted: yes

@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.

The PR currently contains two commits: feat(libc): implement strsep and memccpy (the exact head commit of open PR #1976) plus the ffs/labs commit. Since this PR targets main, its diff duplicates #1976, while the checklist says Depends on: none. Could you either rebase this branch onto main with only the ffs/labs commit, or make the dependency/stack explicit and update the base/description to match the project workflow? This will keep the contribution one-topic and avoid overlapping changes being merged twice.

@Pedroj-64

Copy link
Copy Markdown
Contributor Author

Thanks for catching that. I rebased the branch onto current main and it now contains only the ffs/ffsl/ffsll/labs/imaxabs commit, so the diff no longer overlaps with #1976 and "Depends on: none" is accurate.

@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.

One cross-platform ABI issue remains: this project targets an LP64 guest, but native long is 32 bits on Windows. Math.cpp already documents this for guest lround and uses std::int64_t; these new labs/ffsl signatures and unsigned long arithmetic instead use the host width. On Windows, ffsl cannot see bits 32–63, labs has the wrong return width, and 0x100000000L in the test is not a 64-bit value. Please use the project’s 64-bit guest-long type (std::int64_t, with std::uint64_t for bit operations) for labs/ffsl and make those test values explicitly 64-bit. This needs a Windows build/test before the PR is ready.

@github-actions github-actions Bot added the libc label Oct 9, 2026
@Pedroj-64

Copy link
Copy Markdown
Contributor Author

Good catch, thanks. labs and ffsl now use std::int64_t (and std::uint64_t for the negation and countr_zero), as lround does, so they are 64-bit on Windows too. The test declarations use the same types and the ffsl value is INT64_C(0x100000000).

Linux: the full suite (494 tests) passes and tools/check_conventions.py --base upstream/main passes. I could not run Windows locally, so I am relying on the Windows CI job for it.

@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 verified commit 44f2875: the duplicate #1976 commit is gone, and labs/ffsl plus their tests now use 64-bit guest-long types. That resolves both review points. GitHub currently reports no checks for this head, though the description says Windows CI will validate the ABI fix. Please request/trigger the checks; I can finish the review once they report on this exact 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.

The new head’s runs are now visible as action_required for Build, Conventions, and Progress report (all on 44f2875). That is the repository’s workflow-approval gate, not a source failure. A maintainer must approve the runs; once they complete, I can verify Windows CI and finalize the review.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Same as #1976: please reopen with the title and the log line if one reaches these on main.

@popcoder565 popcoder565 mentioned this pull request Oct 9, 2026
9 tasks done
@popcoder565

Copy link
Copy Markdown
Contributor

First Windows run, since the description says it wasn't tested on Windows.

Setup: Windows 10 (19045), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Merged onto main b7218b3 together with nine other small libc/libkernel PRs: #1975, #1976, #1859, #1685, #1676, #1675, #1641, #1656, #1807.

  • guest_math passes.
  • Full ctest: 510/510 (one GPU test skipped for a device limit).

🤖 Addressed by Claude Code

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.

4 participants