Skip to content

fix(libc): return posix_memalign allocation errors - #973

Merged
oneandonlydean merged 2 commits into
boykopovar:mainfrom
ebarr24:codex/libc-posix-memalign-errors
Oct 7, 2026
Merged

oneandonlydean merged 2 commits into
boykopovar:mainfrom
ebarr24:codex/libc-posix-memalign-errors

Conversation

@ebarr24

@ebarr24 ebarr24 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

What

When a registered allocator returns ENOMEM, posix_memalign currently throws instead of returning that status. Forward the callback's nonzero error code, keep the caller output unchanged on failure, return EINVAL for invalid alignment/output, and preserve errno on returned statuses. Successful allocations still pass the existing non-null and alignment checks.

As requested in review, there is no outer bad_alloc catch: the default allocator already converts allocation failure to ENOMEM. A registered allocator returning success with a null pointer violates its contract and still throws std::bad_alloc. A regression now checks that distinction and unchanged caller output.

Found through source inspection and synthetic fixtures; no title encountered or reproduced this. The return-code contract is described by POSIX and FreeBSD. This improves the existing libc allocation capability in README Status. Measured badge impact is zero: libraries: 2504/2861 -> 2504/2861, shaders: 1135/1166 -> 1135/1166.

No observed competing POSIX callback-status correction as of 2026-10-06T23:50:59.765879+00:00; reviewed all 184 open PRs and 29 issues plus relevant diffs/discussions. #313 adds C++ nothrow allocation and shares a mechanically conflicting test hunk; it has no semantic dependency on this change. #1001 changes heap-finalization order. #639, #864, #887 and #968 change other functions. Depends on: none.

Tested

Windows 11 26200, MinGW-w64 GCC 15.2.0, CMake 4.4.4, Ninja 1.13.2, Release with BUILD_TESTING and SPIRV-Tools. All build targets and patched libs built. No game, firmware or console execution.

  • Current base: ab9437e34df49c00b32a514a2516e15bd75145db; validated clean head: c6dde16023464136ee84a9a703671bc51e274bf8.
  • Isolated base with the identical ApplicationHeap.cpp test source: application_heap_replacement fails because ENOMEM 12 becomes an exception. Other heap controls and the new review case pass on the final head.
  • Before removing the catch, the new review case failed on the rebased submitted implementation: success with null returned 12 instead of throwing. The callback deliberately sets null and returns zero; the test catches precisely std::bad_alloc.
  • Final heap suite: 3/3 pass. Complete Full suite: 367/367 pass, zero skips. Build/test provenance: artifacts/AnyPS5/20261006T234533917Z-357747, artifacts/AnyPS5/20261006T234548143Z-40d83b, artifacts/AnyPS5/20261006T234600394Z-d6e9a0 in the local workspace.
  • Commands: dot-source pipelines/environment.ps1; pipelines/invoke.ps1 -Project AnyPS5 -BuildName AnyPS5-five-prs -Action Build -Jobs 8; -Action Test -TestRegex '^application_heap'; then -Action Test without a filter. Baseline: -SourcePath artifacts/AnyPS5/review-followup/base -BuildName AnyPS5-posix-review-base -Action Validate -TestRegex '^application_heap_replacement$' -Jobs 8, using the same pinned cached FFmpeg package.
  • Equivalent upstream commands: cmake --build build --parallel 8; cmake --build build --target libs --parallel 8; ctest --test-dir build --output-on-failure --timeout 120 -R '^application_heap'; repeat CTest without -R for Full.
  • Local pipelines/contribution.py check artifacts/AnyPS5/review-followup/contribution.json passed before pushing. Workspace tools and reports are excluded from the PR.

011010 independently reported Linux heap checks on the earlier implementation, including reproducing the ENOMEM exception with main's production sources substituted. That report does not validate this revised head; I have not executed it on Linux.

Checklist

  • Based on current main; no observed competing implementation of this behavior
  • One topic per PR; scoped review correction
  • No comments in code added
  • Invalid/unsupported paths retain diagnostics; no silent stub
  • No notes, logs, images or generated fixtures added to the repository
  • No new third-party dependencies
  • Generic behavior, not title-specific
  • Depends on: none
  • AI-assisted: yes, implementation and validation with Codex

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

Pull request Conflicting files (lines on main) Same exports Same new files
#313 core/libs/tests/ApplicationHeap.cpp L203

@github-actions github-actions Bot added the libc label Oct 6, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks. Returning the registered allocator's own error code, and EINVAL for an invalid alignment, matches POSIX and the defaultPosixAlign already on main.

One point on the catch (const std::bad_alloc&). With a registered allocator, the only bad_alloc that can reach it comes from requireAllocation(result): the title's callback returned 0 but left the pointer null. That's the allocator breaking its own contract, not an out-of-memory case, so main's throw is the more accurate answer there. Could you drop the catch (or let that case throw) and keep only the forwarded error code?

Also, did a title hit this, or did you find it by reading the code? If a title's allocator returns ENOMEM here, please name it.

@011010

011010 commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Independent Linux verification (you listed Linux execution as unverified).

Ubuntu 24.04, x86-64 (linux/amd64 container), GCC 13, CMake + Ninja, Release with BUILD_TESTING=ON, ctest -R application_heap:

  • With pr973: 3/3 pass.
  • With the same test and main's core/libs/prx/libc/src/ApplicationHeap.cpp and FileAndHeap.cpp (sources reverted, test kept): application_heap_replacement fails with
posix_memalign callback: expected ENOMEM 12, received exception

So the regression reproduces on Linux, not only Windows, and the fix makes the allocation error come back as ENOMEM instead of an exception. application_heap_invalid_metadata and application_heap_default pass in both runs (positive controls).

@ebarr24
ebarr24 force-pushed the codex/libc-posix-memalign-errors branch from 0412065 to c6dde16 Compare October 6, 2026 23:54
@ebarr24

ebarr24 commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

@oneandonlydean Fixed in c6dde160. I removed the outer std::bad_alloc catch. Nonzero callback statuses are forwarded, and the default allocator still handles its own allocation failure. A registered callback returning success with a null pointer now retains the requireAllocation exception.

I added a regression for that exact case: before the correction it returned 12; after it, the test catches precisely std::bad_alloc and verifies the caller output is unchanged. The existing ENOMEM-return, errno, invalid-alignment and successful-allocation checks remain. The identical test fails on an isolated current upstream base because ENOMEM becomes an exception, then passes on this head. All three heap tests and the complete Windows suite pass: 367/367, zero skips. The branch is rebased onto ab9437e3; the PR description now contains the current revisions and commands.

This was found through source inspection and synthetic fixtures. No title hit it, and I have no title whose allocator returned ENOMEM to name.

@011010 Thanks for independently verifying the earlier implementation on Linux, including the ENOMEM failure with main's production sources substituted and the two positive controls. I've linked your report in the description and kept it separate from my final-head Windows validation; I have not executed this revised head on Linux.

The overlap with #313 is shared test context for its distinct C++ nothrow-allocation change; there is no semantic dependency. The updated PR is ready for another look. AI assistance: Codex, including this correction and regression.

@oneandonlydean
oneandonlydean merged commit 9ab937b into boykopovar:main Oct 7, 2026
3 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