Updating for latest github macos image. #1965
Conversation
WalkthroughAdds a Homebrew tap and switches macOS local formula installs to a tap/path-based workflow; bumps Binutils (2.43→2.45), GCC (14.2.0→15.2.0) and GDB (15.1→16.3); applies inline zlib zutil.h patches; updates CI and installer brew commands; and adds explicit function‑pointer casts and C prototype modernizations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant VSX as VSCode installer
participant GH as GitHub Actions
participant Brew as Homebrew
Dev->>VSX: trigger installToolchain
VSX->>Brew: brew install --verbose nikitabobko/tap/brew-install-path
Note over VSX,Brew: make `install-path` helper available
VSX->>Brew: brew install-path --verbose ./tools/macos-mips/mipsel-none-elf-binutils.rb
VSX->>Brew: brew install-path --verbose ./tools/macos-mips/mipsel-none-elf-gcc.rb
Brew-->>VSX: logs & exit codes
VSX-->>Dev: install result
sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Brew as Homebrew
GH->>Brew: brew install --verbose nikitabobko/tap/brew-install-path
Note over GH,Brew: CI preparatory tap install
GH->>Brew: brew install-path --verbose ./tools/macos-mips/mipsel-none-elf-binutils.rb
GH->>Brew: brew install-path --verbose ./tools/macos-mips/mipsel-none-elf-gcc.rb
Brew-->>GH: logs / exit codes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/vscode-extension/tools.js (2)
149-156: Quote the curl substitution in brew installer commandUsing
$(curl ...)unquoted risks word-splitting and globbing; match Homebrew’s official installer with quotes.- await terminal.run('/bin/bash', [ - '-c', - '$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)' - ]) + await terminal.run('/bin/bash', [ + '-c', + '"$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"' + ])Apply the same change in installToolchain (darwin), installGDB (darwin), and installCMake (darwin).
Also applies to: 219-226, 340-347
291-311: Windows CMake install: use msiexec (start is a cmd builtin, not an executable)
execFile('start', ...)won’t work becausestartis a cmd.exe builtin. Use msiexec to launch the MSI reliably.- await execFile('start', [filename]) + await execFile('msiexec', ['/i', filename])Optionally add
/passiveand/norestartfor non-interactive installs.
🧹 Nitpick comments (1)
src/mips/psyqo/GETTING_STARTED.md (1)
111-118: Minor wording polish (optional)Style nit: “very easy” is a weak intensifier; consider “easy to debug.” No functional impact.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/macos-toolchain.yml(1 hunks)README.md(1 hunks)src/mips/psyqo/GETTING_STARTED.md(2 hunks)tools/vscode-extension/tools.js(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tools/vscode-extension/tools.js (3)
tools/vscode-extension/extension.js (2)
tools(4-4)terminal(11-11)tools/vscode-extension/pcsx-redux.js (1)
terminal(15-15)tools/vscode-extension/templates.js (1)
terminal(7-7)
🪛 LanguageTool
src/mips/psyqo/GETTING_STARTED.md
[style] ~114-~114: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...slow and bloated, but debugging will be very easy. - make BUILD=SmallDebug This will en...
(EN_WEAK_ADJECTIVE)
[grammar] ~116-~116: There might be a mistake here.
Context: ...ng is needed while retaining some speed and small code footprint. - `make BUILD=LTO...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: aur-build
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: build-openbios
- GitHub Check: cross-arm64
- GitHub Check: toolchain
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build
🔇 Additional comments (4)
.github/workflows/macos-toolchain.yml (1)
22-27: Remove path consistency check — documentation and CI both uniformly reference./tools/macos-mips/..., so no drift exists.
• Consider renaming the CI step to “Install brew-install-path command” for clarity.Likely an incorrect or invalid review comment.
README.md (1)
133-136: Fix GCC formula path to match binutils and CIUse
tools/macos-mipsfor GCC like binutils and the CI workflow, otherwise copy-paste will fail.-brew install-path ./tools/macos/mipsel-none-elf-gcc.rb +brew install-path ./tools/macos-mips/mipsel-none-elf-gcc.rbLikely an incorrect or invalid review comment.
src/mips/psyqo/GETTING_STARTED.md (1)
29-32: LGTM: matches CI and the new brew install-path flowThe macOS instructions correctly use
nikitabobko/tap/brew-install-pathandtools/macos-mipsfor both formulas. Please mirror this in README (it currently points GCC totools/macos).See the script attached in the workflow comment to confirm consistency.
tools/vscode-extension/tools.js (1)
110-116:brew install-pathsupports multiple PATH arguments
Thebrew-install-pathscript collects all<PATH>arguments and loops through them, invokingbrew installseparately for each (seepaths.each do |path| … install_formula/install_cask), so a single multi-arg call already installs each path individually. Splitting into two invocations is redundant. (github.com)Likely an incorrect or invalid review comment.
nicolasnoble
force-pushed
the
brew-hotfix
branch
from
bebe362 to
327f4bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mips/psyqo/GETTING_STARTED.md (1)
111-118: Minor wording polish in Build options descriptions for clarity.Tighten phrasing (“very easy”, “with still some optimizations”) and keep tone consistent.
-This is the default build, and will enable a release build of the library and your binary. It will be fast and small, but debugging will be hard. +This is the default build and enables a release build of the library and your binary. It will be fast and small, but debugging will be difficult. -This will enable a full debug build of the library and your binary. It will be slow and bloated, but debugging will be very easy. +This enables a full debug build of the library and your binary. It will be slower and larger, but debugging will be straightforward. -This will enable a debug build of the library and your binary with still some optimizations. Debugging is possible, but will be hindered at times. It is the best compromise if debugging is needed while retaining some speed and small code footprint. +This enables a debug build of the library and your binary with some optimizations still enabled. Debugging is possible but may be hindered at times. This is a good compromise when debugging is needed while retaining some performance and a smaller code footprint. -This will enable a release build of the library and your binary with [LTO](https://gcc.gnu.org/wiki/LinkTimeOptimization) enabled. It will be really fast and really small, but debugging will be impossible. +This enables a release build of the library and your binary with [LTO](https://gcc.gnu.org/wiki/LinkTimeOptimization) enabled. It will be very fast and small, but debugging will be impossible.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/macos-toolchain.yml(1 hunks)README.md(1 hunks)src/mips/psyqo/GETTING_STARTED.md(2 hunks)tools/vscode-extension/tools.js(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/macos-toolchain.yml
- README.md
- tools/vscode-extension/tools.js
🧰 Additional context used
🪛 LanguageTool
src/mips/psyqo/GETTING_STARTED.md
[style] ~114-~114: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...slow and bloated, but debugging will be very easy. - make BUILD=SmallDebug This will en...
(EN_WEAK_ADJECTIVE)
[grammar] ~116-~116: There might be a mistake here.
Context: ...ng is needed while retaining some speed and small code footprint. - `make BUILD=LTO...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: aur-build
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build-openbios
- GitHub Check: coverage
- GitHub Check: asan
- GitHub Check: build
🔇 Additional comments (1)
src/mips/psyqo/GETTING_STARTED.md (1)
29-31: Ignore path consistency suggestionGETTING_STARTED.md and README.md both reference ./tools/macos-mips/…; no changes required.
Likely an incorrect or invalid review comment.
nicolasnoble
force-pushed
the
brew-hotfix
branch
from
327f4bf to
68d0be5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mips/psyqo/GETTING_STARTED.md (2)
29-31: Clarify working directory for install-path and make commands robust to cwd.As written, the relative paths only work from the repo root. Add a note inline so users don’t fail with “No such file or directory” when they downloaded just the two .rb files or ran from another directory.
Apply:
-brew install nikitabobko/tap/brew-install-path -brew install-path ./tools/macos-mips/mipsel-none-elf-binutils.rb -brew install-path ./tools/macos-mips/mipsel-none-elf-gcc.rb +# From the repository root (or adjust the paths accordingly): +brew install nikitabobko/tap/brew-install-path +brew install-path tools/macos-mips/mipsel-none-elf-binutils.rb +brew install-path tools/macos-mips/mipsel-none-elf-gcc.rbIf users report “brew: command not found: install-path”, ask them to re-run the first line (it installs the subcommand) and then re-run their shell to refresh shims.
111-118: Tighten wording in build option descriptions (concise, neutral tone).Minor phrasing nits; this also addresses the style/grammar lints.
Apply:
- - `make BUILD=Debug` - This will enable a full debug build of the library and your binary. It will be slow and bloated, but debugging will be very easy. + - `make BUILD=Debug` + This will enable a full debug build of the library and your binary. It will be slower and larger, but debugging will be easier. - - `make BUILD=SmallDebug` - This will enable a debug build of the library and your binary with still some optimizations. Debugging is possible, but will be hindered at times. It is the best compromise if debugging is needed while retaining some speed and small code footprint. + - `make BUILD=SmallDebug` + This will enable a debug build of the library and your binary with some optimizations retained. Debugging is possible but sometimes hindered. It is a good compromise when debugging is needed while retaining some speed and a smaller code footprint. - - `make BUILD=LTO` - This will enable a release build of the library and your binary with [LTO](https://gcc.gnu.org/wiki/LinkTimeOptimization) enabled. It will be really fast and really small, but debugging will be impossible. + - `make BUILD=LTO` + This will enable a release build of the library and your binary with [LTO](https://gcc.gnu.org/wiki/LinkTimeOptimization) enabled. It will be very fast and small, but debugging will be impossible.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/macos-toolchain.yml(1 hunks)README.md(1 hunks)src/mips/psyqo/GETTING_STARTED.md(2 hunks)tools/vscode-extension/tools.js(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- tools/vscode-extension/tools.js
- .github/workflows/macos-toolchain.yml
🧰 Additional context used
🪛 LanguageTool
src/mips/psyqo/GETTING_STARTED.md
[style] ~114-~114: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...slow and bloated, but debugging will be very easy. - make BUILD=SmallDebug This will en...
(EN_WEAK_ADJECTIVE)
[grammar] ~116-~116: There might be a mistake here.
Context: ...ng is needed while retaining some speed and small code footprint. - `make BUILD=LTO...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build-openbios
- GitHub Check: asan
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: aur-build
- GitHub Check: build
nicolasnoble
force-pushed
the
brew-hotfix
branch
from
68d0be5 to
a759cb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mips/psyqo/GETTING_STARTED.md (2)
29-31: Homebrew tap + install-path: good change; add a quick usage note to avoid path confusion.The commands look correct. Minor UX improvement: clarify that users should run these from the repo root so the relative ./tools paths resolve. Optionally add a fallback for older Homebrew where installing via tap path can fail.
brew install nikitabobko/tap/brew-install-path +# From the PCSX-Redux repo root so ./tools/... resolves brew install-path ./tools/macos-mips/mipsel-none-elf-binutils.rb brew install-path ./tools/macos-mips/mipsel-none-elf-gcc.rb +# If the first line fails on older Homebrew, try: +# brew tap nikitabobko/tap && brew install brew-install-path
111-118: Build options bullets LGTM; tighten two phrases for style/grammar.Small copyedits improve clarity.
-This will enable a full debug build of the library and your binary. It will be slow and bloated, but debugging will be very easy. +This will enable a full debug build of the library and your binary. It will be slow and bloated, but debugging will be easy. -This will enable a debug build of the library and your binary with still some optimizations. Debugging is possible, but will be hindered at times. It is the best compromise if debugging is needed while retaining some speed and small code footprint. +This will enable a debug build of the library and your binary with some optimizations. Debugging is possible, but will be hindered at times. It is the best compromise if debugging is needed while retaining some speed and a small code footprint.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/macos-toolchain.yml(1 hunks)README.md(1 hunks)src/mips/psyqo/GETTING_STARTED.md(2 hunks)tools/vscode-extension/tools.js(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- tools/vscode-extension/tools.js
- .github/workflows/macos-toolchain.yml
🧰 Additional context used
🪛 LanguageTool
src/mips/psyqo/GETTING_STARTED.md
[style] ~114-~114: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...slow and bloated, but debugging will be very easy. - make BUILD=SmallDebug This will en...
(EN_WEAK_ADJECTIVE)
[grammar] ~116-~116: There might be a mistake here.
Context: ...ng is needed while retaining some speed and small code footprint. - `make BUILD=LTO...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: aur-build
- GitHub Check: build
- GitHub Check: toolchain
- GitHub Check: build-openbios
- GitHub Check: coverage
- GitHub Check: asan
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
nicolasnoble
force-pushed
the
brew-hotfix
branch
from
b8b5285 to
d911c2b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1965 +/- ##
=======================================
Coverage 9.30% 9.30%
=======================================
Files 469 469
Lines 145164 145164
=======================================
Hits 13510 13510
Misses 131654 131654 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nicolasnoble
force-pushed
the
brew-hotfix
branch
from
40e22a6 to
00bd1db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/win32-mips/Dockerfile (1)
83-83: Artifact filename still says 14.2.0 — update to 15.2.0.This will produce a misnamed ZIP. Fix to match the upgraded GCC version.
-RUN C:\msys64\usr\bin\bash.exe -l -c 'cd /DIST && zip /c/g++-mipsel-none-elf-14.2.0.zip . -r' +RUN C:\msys64\usr\bin\bash.exe -l -c 'cd /DIST && zip /c/g++-mipsel-none-elf-15.2.0.zip . -r'
🧹 Nitpick comments (6)
tools/linux-mips/spawn-compiler.sh (2)
16-20: Make download/extract robust and resilient to .tar.xz/.tar.gz.Your URLs are pinned to .tar.gz; if mirrors redirect, extraction will fail. Prefer auto-compress tar and glob.
Apply this diff for the extraction steps:
-tar xvfz binutils-2.45.tar.gz +tar -xf binutils-2.45.tar.* cd binutils-2.45
26-30: Optional refactor: auto-detect tar compression and enable parallel make
Replace the fixed.tar.gzextraction and single-threaded builds with compression-agnostic unpacking and-jparallelism (with a fallback ifnprocisn’t available):File: tools/linux-mips/spawn-compiler.sh Lines: 26-30 -tar xvfz gcc-15.2.0.tar.gz +tar -xf gcc-15.2.0.tar.* cd gcc-15.2.0 ./contrib/download_prerequisites mkdir build cd build -../configure --target=mipsel-none-elf … --prefix=$PREFIX -make all-gcc -make install-strip-gcc -make all-target-libgcc -make install-strip-target-libgcc -make all-target-libstdc++-v3 -make install-strip-target-libstdc++-v3 +../configure --target=mipsel-none-elf … --prefix=$PREFIX +MAKEFLAGS="-j$(nproc 2>/dev/null || getconf _NPROCESSORS_ONLN)" +make $MAKEFLAGS all-gcc install-strip-gcc +make $MAKEFLAGS all-target-libgcc install-strip-target-libgcc +make $MAKEFLAGS all-target-libstdc++-v3 install-strip-target-libstdc++-v3tools/vscode-extension/scripts/mipsel-none-elf-gdb.rb (1)
10-14: Revisit Python dependency.Homebrew may have deprecated [email protected]. Either bump to a supported version (e.g., [email protected]) or pass --without-python in configure to drop the dep, aligning with the Windows build.
Would you like a patch updating the dependency and configure flags here and in tools/macos-mips/mipsel-none-elf-gdb.rb for parity?
tools/win32-mips/Dockerfile (3)
65-65: Fix config.log path on failure (binutils).The fallback logs /BUILD/config.log, which doesn’t exist here. Point to the real config.log.
-RUN C:\msys64\usr\bin\bash.exe -l -c 'cd /c/binutils-2.45 && /c/binutils-2.45/configure --target=mipsel-none-elf --disable-multilib --disable-nls --disable-werror --prefix=/DIST || (cat /BUILD/config.log && exit 1)' +RUN C:\msys64\usr\bin\bash.exe -l -c 'cd /c/binutils-2.45 && /c/binutils-2.45/configure --target=mipsel-none-elf --disable-multilib --disable-nls --disable-werror --prefix=/DIST || (cat /c/binutils-2.45/config.log && exit 1)'
73-73: Fix config.log path on failure (GCC).Same here: the correct log is under the build dir.
-RUN C:\msys64\usr\bin\bash.exe -l -c 'mkdir /c/gcc-15.2.0/build && cd /c/gcc-15.2.0/build && ../configure --target=mipsel-none-elf --without-isl --disable-nls --disable-threads --disable-shared --disable-libssp --disable-libstdcxx-pch --disable-libgomp --disable-werror --without-headers --disable-hosted-libstdcxx --with-as=/DIST/bin/mipsel-none-elf-as --with-ld=/DIST/bin/mipsel-none-elf-ld --enable-languages=c,c++ --prefix=/DIST || (cat /BUILD/config.log && exit 1)' +RUN C:\msys64\usr\bin\bash.exe -l -c 'mkdir /c/gcc-15.2.0/build && cd /c/gcc-15.2.0/build && ../configure --target=mipsel-none-elf --without-isl --disable-nls --disable-threads --disable-shared --disable-libssp --disable-libstdcxx-pch --disable-libgomp --disable-werror --without-headers --disable-hosted-libstdcxx --with-as=/DIST/bin/mipsel-none-elf-as --with-ld=/DIST/bin/mipsel-none-elf-ld --enable-languages=c,c++ --prefix=/DIST || (cat /c/gcc-15.2.0/build/config.log && exit 1)'
3-3: Update header comment to reflect 15.2.0.Keep the doc in sync with the produced artifact.
-# Dockerfile to generate the Windows g++-mipsel-none-elf-14.2.0.zip package. +# Dockerfile to generate the Windows g++-mipsel-none-elf-15.2.0.zip package.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.github/workflows/macos-toolchain.yml(1 hunks)tools/linux-mips/spawn-compiler.sh(1 hunks)tools/macos-mips/mipsel-none-elf-gcc.rb(1 hunks)tools/macos-mips/mipsel-none-elf-gdb.rb(1 hunks)tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb(1 hunks)tools/vscode-extension/scripts/mipsel-none-elf-gdb.rb(1 hunks)tools/win32-gdb/Dockerfile(2 hunks)tools/win32-mips/Dockerfile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/macos-mips/mipsel-none-elf-gcc.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb (1)
4-6: Version bump OK; manually verify SHA256 for both URLs
Please confirm that the tarball from https://ftpmirror.gnu.org/gnu/gcc/gcc-15.2.0/gcc-15.2.0.tar.xz and https://mirrors.kernel.org/gnu/gcc/gcc-15.2.0/gcc-15.2.0.tar.xz matches SHA256 438fd996826b0c82485a29da03a72d71d6e3541a83ec702df4271f6e3541a83ec702df4271f6fe025d24e.tools/vscode-extension/scripts/mipsel-none-elf-gdb.rb (1)
4-6: Replaceshasumwithsha256sumand re-verify checksum
The environment lacked theshasumtool; update the check to usesha256sum(or a fallback) and confirm the tarball’s SHA256 equalsbcfcd095528a987917acf9fff3f1672181694926cc18d609c99d0042c00224c5.tools/win32-gdb/Dockerfile (3)
3-3: Good: comment reflects 16.3 artifact produced.
7-7: SHELL normalization LGTM.
48-66: Harden downloads — use HTTPS for 7‑Zip and add SHA256 verification for the GDB tarball.
- Replace the 7‑Zip HTTP installer in tools/win32-gdb/Dockerfile (line 9: http://www.7-zip.org/a/7z1604-x64.exe) with an HTTPS URL and update to a maintained 7‑Zip release.
- Add explicit SHA256 verification for the GDB tarball (ARG GDB=https://ftpmirror.gnu.org/gnu/gdb/gdb-16.3.tar.xz; download RUN around lines 50–53). Fetch the published checksum (or .sha256) and fail the build if the computed hash (Get-FileHash/sha256sum) does not match, before extracting.
- Verification attempt failed in the sandbox (certutil not available); enforce the checksum check inside the Dockerfile or CI instead.
tools/macos-mips/mipsel-none-elf-gdb.rb (2)
10-14: Verify Homebrew support for [email protected] or update dependency
Homebrew isn’t available in this environment to confirm[email protected]support; please check on your local setup whether that formula still exists and, if not, align to a supported version (e.g.,[email protected]) or add--without-python.
4-6: Ensure SHA256 verification succeeds on a supported environment
The sandbox lacks bothshasumandsha256sum, causing the mirror‐and‐checksum script to fail. Please run the download and checksum (e.g. viaopenssl dgst -sha256) against both URLs to confirm the hashbcfcd095528a987917acf9fff3f1672181694926cc18d609c99d0042c00224c5matches..github/workflows/macos-toolchain.yml (1)
25-27: Disable Homebrew auto-updates, cleanup & analytics in CI
In both thebuild mips binutilsandbuild mips gccsteps, add anenv:block:env: HOMEBREW_NO_AUTO_UPDATE: "1" HOMEBREW_NO_INSTALL_CLEANUP: "1" HOMEBREW_NO_ANALYTICS: "1"Verify that your CI runner provides the
brew install-pathcommand before merging.tools/win32-mips/Dockerfile (1)
48-61: Version bumps look correct.BINUTILS 2.45 and GCC 15.2.0 URLs and extraction targets align; good.
| - name: install dependencies | ||
| run: brew install --verbose nikitabobko/tap/brew-install-path |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the tap commit to avoid supply-chain drift.
Installing the external brew command from a moving tap HEAD can break CI unexpectedly. Pin the tap to a known commit, then install the formula.
- - name: install dependencies
- run: brew install --verbose nikitabobko/tap/brew-install-path
+ - name: install brew-install-path (pinned)
+ run: |
+ brew tap nikitabobko/tap https://github.com/nikitabobko/homebrew-tap
+ git -C "$(brew --repo nikitabobko/tap)" checkout <commit-sha>
+ brew install --verbose nikitabobko/tap/brew-install-path
+ shell: bashReplace with the tested revision.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: install dependencies | |
| run: brew install --verbose nikitabobko/tap/brew-install-path | |
| - name: install brew-install-path (pinned) | |
| run: | | |
| brew tap nikitabobko/tap https://github.com/nikitabobko/homebrew-tap | |
| git -C "$(brew --repo nikitabobko/tap)" checkout <commit-sha> | |
| brew install --verbose nikitabobko/tap/brew-install-path | |
| shell: bash |
🤖 Prompt for AI Agents
.github/workflows/macos-toolchain.yml lines 22-23: currently installing an
external brew formula directly from the tap HEAD—pin the tap to a specific
commit to avoid supply-chain drift by (1) tapping the nikitabobko/tap repo, (2)
fetching and checking out the tested <commit-sha> in the tap repository, and
then (3) running the brew install of nikitabobko/tap/brew-install-path; replace
<commit-sha> with the known-good revision and ensure these steps are executed in
the workflow before the installation step.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tools/macos-mips/zlib.patch (1)
11-15: Align patch file paths to binutils’ zlib layout
In tools/macos-mips/zlib.patch, change the diff header from:diff --git a/zutil.h b/zutil.hto:
diff --git a/zlib/zutil.h b/zlib/zutil.hso the patch applies cleanly to the zlib/zutil.h in binutils.
tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb (2)
10-10: Patch :DATA is fine; consider de-dup with macOS formula (optional).Same inline patch also exists in tools/macos-mips/mipsel-none-elf-binutils.rb. If both are likely to drift, consider sourcing a single patch file (repo-hosted or raw URL) to DRY.
1-24: Namespace or rename duplicate MipselNoneElfBinutils classes
tools/macos-mips/mipsel-none-elf-binutils.rb and tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb both defineclass MipselNoneElfBinutils, which will trigger a Ruby constant redefinition warning if loaded in the same process; consider renaming one or wrapping it in a module to avoid collisions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
tools/macos-mips/mipsel-none-elf-binutils.rb(2 hunks)tools/macos-mips/zlib.patch(1 hunks)tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: aur-build
- GitHub Check: build
- GitHub Check: build-openbios
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
- GitHub Check: toolchain
🔇 Additional comments (10)
tools/macos-mips/zlib.patch (1)
19-31: Preserve the fdopen-removal patch: binutils-2.45’s zutil.h still contains the#if defined(MACOS) || defined(TARGET_OS_MAC)block and multiple#define fdopenshims, so this patch is required and will apply cleanly.tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb (4)
12-20: Build/install stanza LGTM.
21-23: Test is adequate.c++filt smoke test is appropriate.
26-69: Upstream zutil.h (binutils-2.45) still contains the original#if defined(MACOS) || defined(TARGET_OS_MAC)guard at line 140; the fdopen shim removal patch will apply cleanly.
4-6: Checksum verified The sha256 matches the downstream download (8a3eb4b10e7053312790f21ee1a38f7e2bbd6f4096abb590d3429e5119592d96).tools/macos-mips/mipsel-none-elf-binutils.rb (5)
10-10: Patch :DATA is appropriate here.No issues.
12-20: Configure/make/install LGTM.Flags look standard for mipsel-none-elf.
21-23: Test is fine.c++filt demangle check is reliable.
4-6: Verify binutils-2.45.tar.gz SHA256 checksum manually.
The CI environment couldn’t compute the digest—please download the tarball and confirm its SHA256 matches
8a3eb4b10e7053312790f21ee1a38f7e2bbd6f4096abb590d3429e5119592d96.
26-69: Confirm necessity against upstream binutils-2.45 and sync with tools/macos-mips/zlib.patch
- Verify whether upstream binutils-2.45’s zlib/zutil.h has already removed the vestigial
fdopendefines; if so, drop the inline patch to avoid redundancy.- If the patch is still needed, adjust tools/macos-mips/zlib.patch so its file paths (
zlib/zutil.h) and hunk context exactly match the inline patch in mipsel-none-elf-binutils.rb.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tools/macos-mips/zlib.patch (1)
11-33: Remove or align tools/macos-mips/zlib.patch
No Homebrew formula references this external patch (they all use inlinepatch :DATA), so either delete tools/macos-mips/zlib.patch or update the formulae to apply it—otherwise it will drift.tools/macos-mips/mipsel-none-elf-gcc.rb (1)
14-15: Embedded patch duplication—consider de-dup or automated sync.Same patch appears in multiple formulae. If you want to keep :DATA, consider generating it from a single source to avoid drift.
tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb (2)
4-6: Optional: prefer .xz tarball to reduce download size.Swap to the .xz artifact and update the checksum (saves bandwidth/time for CI/local builds).
- url "https://ftpmirror.gnu.org/gnu/binutils/binutils-2.45.tar.gz" - mirror "https://mirrors.kernel.org/gnu/binutils/binutils-2.45.tar.gz" - sha256 "8a3eb4b10e7053312790f21ee1a38f7e2bbd6f4096abb590d3429e5119592d96" + url "https://ftpmirror.gnu.org/gnu/binutils/binutils-2.45.tar.xz" + mirror "https://mirrors.kernel.org/gnu/binutils/binutils-2.45.tar.xz" + sha256 "c50c0e7f9cb188980e2cc97e4537626b1672441815587f1eab69d2a1bfbef5d2"
10-10: Scope the inline patch to macOS to minimize blast radius.Functionally the patch is macOS-motivated; guard it so Linux users of this script don’t carry an unnecessary source delta.
- patch :DATA + on_macos do + patch :DATA + end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
tools/macos-mips/mipsel-none-elf-binutils.rb(2 hunks)tools/macos-mips/mipsel-none-elf-gcc.rb(2 hunks)tools/macos-mips/zlib.patch(1 hunks)tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb(2 hunks)tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: aur-build
- GitHub Check: cross-arm64
- GitHub Check: toolchain
🔇 Additional comments (19)
tools/macos-mips/zlib.patch (3)
1-9: Patch provenance looks good; keep commit metadata.This matches Mark Adler’s upstream intent (“Remove fdopen #defines in zutil.h.”). Keeping the From/Subject/Date aids traceability.
38-49: Manual verification required: ensure no remainingfdopen()calls in the bundled zlib sources. The patch removes all fdopen mappings; please grep your GCC/binutils zlib directories forfdopen(to confirm no references remain.
19-21: Verify Darwin/macOS still maps to OS_CODE 19 via APPLE
Confirm that your vendored zutil.h contains the#ifdef __APPLE__branch definingOS_CODE 19; without it, modern macOS builds would no longer set the correct OS_CODE.tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb (5)
16-41: Install flags look consistent for a bare cross toolchain.No blockers observed for configure/make targets.
43-53: Test is minimal but sufficient.Object compile-only smoke test is fine for CI speed.
56-96: AI summary mismatch: this patch removes, not adds, fdopen mappings.The embedded diff deletes fdopen shims (MACOS/TARGET_OS_MAC, BEOS/RISCOS, MSVC). Please disregard the “adds MSVC mapping” note in the AI summary.
14-15: Ensure the embedded patch still applies cleanly to GCC 15.2.0’s in-tree zlibRun a dry-run of the patch against the official source:
curl -Ls -o gcc.tar.gz https://ftpmirror.gnu.org/gnu/gcc/gcc-15.2.0/gcc-15.2.0.tar.gz tar -xzf gcc.tar.gz patch -p1 --dry-run -d gcc-15.2.0 < <( awk '/^__END__$/ {skip=1; next} skip' tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb )If this reports hunks failed, update or remove the in-DATA patch accordingly.
4-6: Checksum and mirrors validated for gcc-15.2.0.tar.xz.
SHA256 matches and both ftpmirror.gnu.org and mirrors.kernel.org return HTTP 200.tools/macos-mips/mipsel-none-elf-gcc.rb (4)
43-53: Test snippet OK.Simple compile test suffices here.
56-96: AI summary mismatch: this patch removes, not adds, fdopen mappings.Same note as in the VSCode script formula.
4-6: GCC 15.2.0 bump: manual checksum verification required
Sandbox lacks bothshasumandsha256sum; please manually verify that the tarball at https://ftpmirror.gnu.org/gnu/gcc/gcc-15.2.0/gcc-15.2.0.tar.xz has SHA-256 hash
438fd996826b0c82485a29da03a72d71d6e3541a83ec702df4271f6fe025d24e.
16-41: No changes required for --disable-hosted-libstdcxx – this option is supported by libstdc++-v3’s configure (acinclude.m4/configure) to disable the hosted C++ library build as intended.tools/macos-mips/mipsel-none-elf-binutils.rb (5)
12-20: Install flags are standard for cross binutils.Looks good.
21-23: Test is appropriate.c++filt smoke test is fine.
26-66: AI summary mismatch: patch removes fdopen shims.As above, the diff deletes BEOS/RISCOS/MSVC fdopen defines and narrows MACOS handling.
10-11: Keep the zlib patch — upstream still defines macOS fdopen macros
Confirmed in binutils-2.45/zlib/zutil.h that the#if defined(MACOS) || defined(TARGET_OS_MAC)block still definesfdopen(fd,mode) NULL(lines 140–147), so the in-tree zlib patch remains necessary.
4-6: Verify SHA256 for Binutils 2.45 bump
Automated checksum verification failed due to missing tools in the test environment; please manually confirm that the SHA256 matches the upstream release.tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb (2)
4-6: LGTM: version bump and checksum match upstream release.binutils 2.45 tar.gz SHA-256 is 8a3eb4b10e7053312790f21ee1a38f7e2bbd6f4096abb590d3429e5119592d96; URLs are correct. (lists.gnu.org)
26-69: Patch correctly removes TARGET_OS_MAC handling and fdopen redefinitions (fixes Xcode 16.3 breakage).
- Dropping “|| defined(TARGET_OS_MAC)” avoids mis-detecting macOS since Apple Clang 17/Xcode 16.3 predefines TARGET_OS_* and triggered fdopen macro conflicts.
- Removing legacy fdopen defines aligns with upstream zlib, which eliminated these macros.
This is consistent with upstream discussion/commits and downstream fixes. (mail-archive.com, gnats.netbsd.org, mail-index.netbsd.org)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb (1)
10-10: Deduplicate zlib patch and unify copies
Inline patches in tools/vscode-extension/scripts and tools/macos-mips formulas match each other but drift from tools/macos-mips/zlib.patch. Extract the patch into the standalone zlib.patch and reference it in all scripts (or vice-versa), then add a CI check to flag any future mismatches.tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb (1)
14-14: Inline patch OK; prefer dedup or a guardrail.Same duplication concern as the binutils formula. Consider a CI check to enforce identical patch bodies.
tools/macos-mips/mipsel-none-elf-binutils.rb (1)
10-10: Inline patch duplication.Consider dedup/CI guard as suggested earlier.
tools/macos-mips/mipsel-none-elf-gcc.rb (1)
14-14: Inline patch duplication.Same dedup/CI guard recommendation applies.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
tools/macos-mips/mipsel-none-elf-binutils.rb(2 hunks)tools/macos-mips/mipsel-none-elf-gcc.rb(2 hunks)tools/macos-mips/zlib.patch(1 hunks)tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb(2 hunks)tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: aur-build
- GitHub Check: toolchain
🔇 Additional comments (13)
tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb (3)
21-23: LGTM: test covers c++filt demangling.
4-6: Verify SHA-256 checksum for binutils-2.45
In tools/vscode-extension/scripts/mipsel-none-elf-binutils.rb (lines 4–6), runsha256sum(or equivalent) on both
• https://ftpmirror.gnu.org/gnu/binutils/binutils-2.45.tar.gz
• https://mirrors.kernel.org/gnu/binutils/binutils-2.45.tar.gz
and confirm they both match
8a3eb4b10e7053312790f21ee1a38f7e2bbd6f4096abb590d3429e5119592d96
35-37: Do not reintroduce TARGET_OS_MAC. Upstream deliberately droppeddefined(TARGET_OS_MAC)to fix zlib on Xcode 16.3+, which now always defines that macro and would otherwise reenable the OS_CODE and fdopen shims erroneously (mail-archive.com, groups.google.com). The fdopen‐shim removals mirror upstream commit 4bd9a71… removing unused mappings (mail-index.netbsd.org). No change needed.Likely an incorrect or invalid review comment.
tools/macos-mips/zlib.patch (2)
38-49: Confirm host support expectations.This removes all Windows fdopen mappings. If any Windows-hosted builds still consume this patch, validate no regressions.
11-15: Ignore path update suggestion The zlib upstream tree places zutil.h in its top-level directory, so keepingdiff --git a/zutil.h b/zutil.his correct.Likely an incorrect or invalid review comment.
tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb (2)
43-53: LGTM: smoke test compiles a simple C TU.
4-6: Manual verification required: confirm SHA-256 for GCC 15.2.0 source
Download and checksum both URLs on your local machine—ensure they match the expected digest:curl -sSL https://ftpmirror.gnu.org/gnu/gcc/gcc-15.2.0/gcc-15.2.0.tar.xz | shasum -a 256 curl -sSL https://mirrors.kernel.org/gnu/gcc/gcc-15.2.0/gcc-15.2.0.tar.xz | shasum -a 256 # Expected: 438fd996826b0c82485a29da03a72d71d6e3541a83ec702df4271f6fe025d24etools/macos-mips/mipsel-none-elf-binutils.rb (3)
4-6: Verify source integrity for binutils 2.45.Same as the VSCode copy: confirm SHA-256 for both URLs.
(You can reuse the script provided in the other comment.)
21-23: LGTM: test demangling via c++filt.
35-37: Retain the upstream change removing TARGET_OS_MAC
The patch mirrors the official zlib fix for Xcode 16.3, which intentionally dropsTARGET_OS_MACto avoid spuriousfdopenmacro collisions on modern macOS (see Georgi Valkov’s zlib patch) (mail-archive.com). RestoringTARGET_OS_MACwould reintroduce the very build failure this change fixes.Likely an incorrect or invalid review comment.
tools/macos-mips/mipsel-none-elf-gcc.rb (3)
4-6: Verify source integrity for GCC 15.2.0.Same checksum verification as the VSCode copy.
43-53: LGTM: compile-only test is adequate here.
56-57: I’ve extracted the embedded patch context from the formula.
| -#if defined(MACOS) || defined(TARGET_OS_MAC) | ||
| +#if defined(MACOS) | ||
| # define OS_CODE 7 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Retain TARGET_OS_MAC in the condition.
Same rationale as in the embedded patches: avoid changing OS_CODE semantics on Apple hosts.
-#if defined(MACOS)
+#if defined(MACOS) || defined(TARGET_OS_MAC)🤖 Prompt for AI Agents
In tools/macos-mips/zlib.patch around lines 19 to 21, the preprocessor condition
was changed to drop TARGET_OS_MAC; restore the original combined condition so
Apple-host detection still triggers. Replace the line with a condition that
checks both defined(MACOS) || defined(TARGET_OS_MAC) (i.e., revert to the
original compound #if) so OS_CODE remains unchanged on Apple hosts.
| __END__ | ||
| diff --git a/zlib/zutil.h b/zlib/zutil.h |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Preserve TARGET_OS_MAC in the zlib guard and confirm Windows macro removals.
Mirror the recommendation from the binutils formula: keep TARGET_OS_MAC in the conditional to avoid changing OS_CODE; validate host impact of removing Windows fdopen mappings.
-#if defined(MACOS)
+#if defined(MACOS) || defined(TARGET_OS_MAC)Also applies to: 65-67, 84-95
🏁 Script executed:
#!/bin/bash
set -eux
# Locate the Ruby script containing the embedded zlib patch
script=$(find . -type f -name mipsel-none-elf-gcc.rb | head -n1)
echo "Using script: $script"
# Show the patch applied after __END__
echo "===== Embedded zlib/zutil.h patch ====="
awk '/^__END__/{p=1; next} p' "$script"
# Locate the zlib/zutil.h file in-tree
file=$(find . -type f -path '*/zlib/zutil.h' | head -n1)
echo "Found zlib/zutil.h at: $file"
# Display the conditional around MACOS in zutil.h
echo "===== zutil.h guard around MACOS ====="
sed -n '130,170p' "$file"Length of output: 1813
Restore TARGET_OS_MAC in the zlib guard and verify Windows fdopen mappings
In tools/macos-mips/mipsel-none-elf-gcc.rb, update the embedded zlib/zutil.h patch so the Mac guard reads:
-#if defined(MACOS)
+#if defined(MACOS) || defined(TARGET_OS_MAC)Also review the removal of the BEOS/RISCOS and MSVC fdopen definitions (formerly around lines 65–67 and 84–95) to ensure no Windows‐host builds break.
nicolasnoble
force-pushed
the
brew-hotfix
branch
from
8fa5c57 to
909eb22
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb (1)
65-67: Restore TARGET_OS_MAC in the zlib guard (macOS detection regression).Dropping TARGET_OS_MAC risks changing OS_CODE and behavior on Apple hosts. Keep the original inclusive check.
Apply within the embedded patch:
-#if defined(MACOS) +#if defined(MACOS) || defined(TARGET_OS_MAC)
🧹 Nitpick comments (3)
src/mips/openbios/card/device.c (1)
635-637: Prefer typed no-op wrappers over function-pointer casts.Casting
psxdummyto mismatched function-pointer types silences warnings but can mask ABI/UB if these pointers are ever invoked. Define small, correctly-typed no-op stubs and reference them here. Also, nit: there’s a double space inint, int.Apply within this block:
- .action = (int (*)(struct File *, enum FileAction))psxdummy, + .action = bu_action_noop, - .ioctl = (int (*)(struct File *, int, int))psxdummy, + .ioctl = bu_ioctl_noop,Add near the top of this file (or a common stub header) to keep types correct:
static int bu_action_noop(struct File *f, enum FileAction a) { (void)f; (void)a; return 0; } static int bu_ioctl_noop(struct File *f, int a, int b) { (void)f; (void)a; (void)b; return 0; }Verification: please confirm your GCC/Clang flags don’t elevate
-Wcast-function-type/-Wcast-function-type-strictto errors for other sites still using casts, or adopt the wrapper pattern repo-wide.tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb (1)
14-15: Avoid patch drift between duplicate formulae.This file and tools/macos-mips/mipsel-none-elf-gcc.rb embed identical END patches; consider sourcing a single patch file to keep them in sync.
tools/macos-mips/mipsel-none-elf-gcc.rb (1)
14-15: De-duplicate the embedded patch across both formulae.Reference a single patch file to prevent divergence with the VSCode script copy.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/mips/openbios/card/device.c(1 hunks)src/mips/openbios/cdrom/cdrom.c(1 hunks)tools/macos-mips/mipsel-none-elf-gcc.rb(2 hunks)tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/mips/openbios/cdrom/cdrom.c (1)
src/mips/openbios/cdrom/filesystem.c (4)
dev_cd_read(263-283)dev_cd_firstFile(290-315)dev_cd_nextFile(317-343)dev_cd_chdir(345-351)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-openbios
- GitHub Check: aur-build
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: toolchain
- GitHub Check: asan
- GitHub Check: macos-build-and-test-toolchain
🔇 Additional comments (3)
tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb (1)
4-6: GCC bump: manually verify tarball checksum and mirror reachability
Environment lacks a checksum utility—please confirm the sha256 value matches the upstream SHA256SUMS and that both URLs download the correct gcc-15.2.0.tar.xz.tools/macos-mips/mipsel-none-elf-gcc.rb (2)
84-98: Non-macOS fdopen overrides dropped
The patch removes BEOS, RISCOS and Windows-specific fdopen definitions as intended.
4-6: GCC bump: manual verification required. Confirm the SHA256 in https://ftp.gnu.org/gnu/gcc/gcc-15.2.0/sha256.sum matches438fd996826b0c82485a29da03a72d71d6e3541a83ec702df4271f6fe025d24e, and that both https://ftpmirror.gnu.org/gnu/gcc/gcc-15.2.0/gcc-15.2.0.tar.xz and https://mirrors.kernel.org/gnu/gcc/gcc-15.2.0/gcc-15.2.0.tar.xz return HTTP 200.
| .close = (int (*)(struct File *))psxdummy, | ||
| .ioctl = (int (*)(struct File *, int, int))psxdummy, | ||
| .read = dev_cd_read, | ||
| .write = psxdummy, | ||
| .erase = psxdummy, | ||
| .write = (int (*)(struct File *, void *, int))psxdummy, | ||
| .erase = (int (*)(struct File *, const char *))psxdummy, | ||
| .undelete = psxdummy, | ||
| .firstFile = dev_cd_firstFile, | ||
| .nextFile = dev_cd_nextFile, | ||
| .format = psxdummy, | ||
| .format = (int (*)(struct File *))psxdummy, | ||
| .chdir = dev_cd_chdir, | ||
| .rename = psxdummy, | ||
| .rename = (int (*)(struct File *, const char *, struct File *, const char *))psxdummy, | ||
| .deinit = deinitCDRom, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Replace repeated casts with correctly-typed stubs to avoid UB and reduce warning surface.
Multiple fields cast psxdummy to incompatible signatures. Safer and clearer to use typed no-op wrappers. Also, nit: fix int, int spacing.
Apply within this block:
- .action = (int (*)(struct File *, enum FileAction))psxdummy,
- .close = (int (*)(struct File *))psxdummy,
- .ioctl = (int (*)(struct File *, int, int))psxdummy,
+ .action = cd_action_noop,
+ .close = cd_close_noop,
+ .ioctl = cd_ioctl_noop,
.read = dev_cd_read,
- .write = (int (*)(struct File *, void *, int))psxdummy,
- .erase = (int (*)(struct File *, const char *))psxdummy,
+ .write = cd_write_noop,
+ .erase = cd_erase_noop,
.undelete = psxdummy,
.firstFile = dev_cd_firstFile,
.nextFile = dev_cd_nextFile,
- .format = (int (*)(struct File *))psxdummy,
+ .format = cd_format_noop,
.chdir = dev_cd_chdir,
- .rename = (int (*)(struct File *, const char *, struct File *, const char *))psxdummy,
+ .rename = cd_rename_noop,Add near the top of this file (or shared):
static int cd_action_noop(struct File *f, enum FileAction a){ (void)f; (void)a; return 0; }
static int cd_close_noop(struct File *f){ (void)f; return 0; }
static int cd_ioctl_noop(struct File *f, int a, int b){ (void)f; (void)a; (void)b; return 0; }
static int cd_write_noop(struct File *f, void *buf, int sz){ (void)f; (void)buf; (void)sz; return -1; } // write not supported
static int cd_erase_noop(struct File *f, const char *p){ (void)f; (void)p; return -1; }
static int cd_format_noop(struct File *f){ (void)f; return -1; }
static int cd_rename_noop(struct File *f, const char *o, struct File *u, const char *n){
(void)f; (void)o; (void)u; (void)n; return -1;
}Verification: run a quick scan to catch any remaining casts so we can standardize on wrappers.
🏁 Script executed:
#!/bin/bash
# Find device initializers still casting psxdummy
rg -nP '\.\s*(action|close|ioctl|write|erase|format|rename)\s*=\s*\([^)]*\)\s*psxdummy' -C2 src | sed -n '1,200p'Length of output: 113
🏁 Script executed:
#!/bin/bash
# List all occurrences of psxdummy casts or assignments in the CD-ROM driver
rg -n 'psxdummy' -C2 src/mips/openbios/cdrom/cdrom.cLength of output: 867
🏁 Script executed:
#!/bin/bash
# Show the struct initializer context in cdrom.c to identify field types
sed -n '1,50p' src/mips/openbios/cdrom/cdrom.cLength of output: 2153
🏁 Script executed:
#!/bin/bash
# Display the struct initializer for the CD-ROM device to inspect all psxdummy assignments
sed -n '80,150p' src/mips/openbios/cdrom/cdrom.cLength of output: 1994
Replace all psxdummy casts in the s_cdromDevice initializer with correctly-typed no-op stubs
Define static stub functions for each signature at the top of cdrom.c (or in a shared helper), for example:
static void cd_init_noop(struct Device *d){ (void)d; }
static int cd_action_noop(struct File *f, enum FileAction a){ (void)f; (void)a; return 0; }
static int cd_close_noop(struct File *f){ (void)f; return 0; }
static int cd_ioctl_noop(struct File *f, int a, int b){ (void)f; (void)a; (void)b; return 0; }
static ssize_t cd_read_noop(struct File *f, void *buf, int sz){ (void)f; (void)buf; (void)sz; return -1; }
static int cd_erase_noop(struct File *f, const char *p){ (void)f; (void)p; return -1; }
static int cd_format_noop(struct File *f){ (void)f; return -1; }
static int cd_undelete_noop(struct File *f){ (void)f; return 0; }
static int cd_rename_noop(struct File *f, const char *o, struct File *u, const char *n){
(void)f; (void)o; (void)u; (void)n; return -1;
}
static int cd_check_noop(void){ return 0; }Then update each initializer field in src/mips/openbios/cdrom/cdrom.c accordingly (and fix the double space in .ioctl = … int, int).
| -#if defined(MACOS) || defined(TARGET_OS_MAC) | ||
| +#if defined(MACOS) | ||
| # define OS_CODE 7 |
There was a problem hiding this comment.
Restore TARGET_OS_MAC in zlib’s MAC guard.
Same concern as the VSCode script: keep TARGET_OS_MAC to avoid OS_CODE/behavior changes on Apple.
-#if defined(MACOS)
+#if defined(MACOS) || defined(TARGET_OS_MAC)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -#if defined(MACOS) || defined(TARGET_OS_MAC) | |
| +#if defined(MACOS) | |
| # define OS_CODE 7 | |
| #if defined(MACOS) || defined(TARGET_OS_MAC) | |
| # define OS_CODE 7 |
🤖 Prompt for AI Agents
In tools/macos-mips/mipsel-none-elf-gcc.rb around lines 65 to 67, the
preprocessor guard was changed to remove TARGET_OS_MAC; restore TARGET_OS_MAC so
the condition checks both MACOS and TARGET_OS_MAC. Replace the current
single-symbol check with a compound check that uses defined(MACOS) ||
defined(TARGET_OS_MAC) so the original behavior and OS_CODE mapping remain
unchanged.
| -#if defined(_BEOS_) || defined(RISCOS) | ||
| -# define fdopen(fd,mode) NULL /* No fdopen() */ | ||
| -#endif | ||
| - | ||
| #if (defined(_MSC_VER) && (_MSC_VER > 600)) && !defined __INTERIX | ||
| # if defined(_WIN32_WCE) | ||
| -# define fdopen(fd,mode) NULL /* No fdopen() */ | ||
| # ifndef _PTRDIFF_T_DEFINED | ||
| typedef int ptrdiff_t; | ||
| # define _PTRDIFF_T_DEFINED | ||
| # endif | ||
| -# else | ||
| -# define fdopen(fd,type) _fdopen(fd,type) | ||
| # endif | ||
| #endif |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unrelated Windows/BEOS/RISCOS changes—reduce blast radius.
These hunks alter fdopen mappings for non-macOS platforms in GCC’s bundled zlib. Since this formula builds on macOS, keep the patch minimal and avoid modifying other OS paths.
Proposed change: drop this entire hunk from the END patch so upstream behavior remains for those platforms.
-@@ -163,19 +154,12 @@
-#if defined(_BEOS_) || defined(RISCOS)
-# define fdopen(fd,mode) NULL /* No fdopen() */
-#endif
-
-#if (defined(_MSC_VER) && (_MSC_VER > 600)) && !defined __INTERIX
-# if defined(_WIN32_WCE)
-# ifndef _PTRDIFF_T_DEFINED
- typedef int ptrdiff_t;
-# define _PTRDIFF_T_DEFINED
-# endif
-# else
-# define fdopen(fd,type) _fdopen(fd,type)
-# endif
-#endif🏁 Script executed:
#!/bin/bash
set -eux
file=$(find . -type f -path "*/tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb")
echo "Inspecting $file"
sed -n '1,200p' "$file"Length of output: 3392
Drop non-macOS fdopen patches from zutil.h
Remove the BEOS/RISCOS and Windows CE fdopen mappings from the __END__ patch in tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb so only the macOS-specific zlib fix remains.
🤖 Prompt for AI Agents
In tools/vscode-extension/scripts/mipsel-none-elf-gcc.rb around lines 84 to 98,
the patch currently contains non-macOS fdopen mappings (BEOS/RISCOS and Windows
CE) that should be removed so only the macOS-specific zlib fix remains; update
the __END__ patch by deleting the blocks defining fdopen for BEOS/RISCOS and the
Windows CE/_fdopen mapping (including the conditional branches that add those
defines) and ensure the macOS zlib fdopen change is the only fdopen-related
alteration left in the patch.
f89dac0 to
a7035f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mips/openbios/tty/tty.c (2)
80-93: Same here: reduce cast noise and potential UB; keep signature consistencyMirror the approach above for the dummy device: use the macro (or typed wrappers) for clarity and to avoid calling through incompatible function-pointer types. Also consider void (*)(void) for deinit if the struct’s prototype is prototyped.
Apply within this hunk:
- .open = (int (*)(struct File *, const char *, int))psxdummy, - .action = (int (*)(struct File *, enum FileAction))psxdummy, - .close = (int (*)(struct File *))psxdummy, - .ioctl = (int (*)(struct File *, int, int))psxdummy, - .read = (int (*)(struct File *, void *, int))psxdummy, - .write = (int (*)(struct File *, void *, int))psxdummy, - .erase = (int (*)(struct File *, const char *))psxdummy, + .open = PSX_DUMMY(int (*)(struct File *, const char *, int)), + .action = PSX_DUMMY(int (*)(struct File *, enum FileAction)), + .close = PSX_DUMMY(int (*)(struct File *)), + .ioctl = PSX_DUMMY(int (*)(struct File *, int, int)), + .read = PSX_DUMMY(int (*)(struct File *, void *, int)), + .write = PSX_DUMMY(int (*)(struct File *, void *, int)), + .erase = PSX_DUMMY(int (*)(struct File *, const char *)), @@ - .format = (int (*)(struct File *))psxdummy, + .format = PSX_DUMMY(int (*)(struct File *)), @@ - .rename = (int (*)(struct File *, const char *, struct File *, const char *))psxdummy, + .rename = PSX_DUMMY(int (*)(struct File *, const char *, struct File *, const char *)), @@ - .deinit = (void (*)())psxdummy, + .deinit = (void (*)(void))psxdummy, /* if compatible with Device.deinit */Style nit: match spacing used on firstFile/nextFile initializers, e.g., keep a space before psxdummy consistently.
- .rename = PSX_DUMMY(int (*)(struct File *, const char *, struct File *, const char *)), + .rename = PSX_DUMMY(int (*)(struct File *, const char *, struct File *, const char *)),
57-68: Use PSX_DUMMY macro for psxdummy casts and remove extra spaces
- Define PSX_DUMMY near the top (e.g. in a shared header or above the includes):
#ifndef PSX_DUMMY #define PSX_DUMMY(type) ((type)psxdummy) #endif- Replace all raw casts in this hunk with PSX_DUMMY, for example:
- .close = (int (*)(struct File *))psxdummy, + .close = PSX_DUMMY(int (*)(struct File *)), - .deinit = (void (*)())psxdummy, + .deinit = PSX_DUMMY(device_deinit),- Remove inconsistent spaces before
psxdummyin any remaining initializers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/mips/openbios/card/device.c(1 hunks)src/mips/openbios/cdrom/cdrom.c(1 hunks)src/mips/openbios/tty/tty.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mips/openbios/card/device.c
- src/mips/openbios/cdrom/cdrom.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: build-openbios
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: build
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mips/psyqo/src/xprintf.c (2)
728-734: Fix vsnprintf when n==0 (out-of-bounds write today).When n==0, arg.last = &buf[n-1] underflows and then *buf = 0 writes out of bounds. Route writes to a local dummy byte instead and leave buf untouched, per C99.
int vsnprintf(char *buf, size_t n, const char *fmt, va_list ap) { struct s_strargument arg; - arg.next = buf; - arg.last = &buf[n - 1]; - *buf = 0; - return vxprintf(sout, &arg, fmt, ap); + if (n == 0) { + char dummy = 0; + arg.next = &dummy; + arg.last = &dummy; + return vxprintf(sout, &arg, fmt, ap); + } else { + arg.next = buf; + arg.last = &buf[n - 1]; + *buf = 0; + return vxprintf(sout, &arg, fmt, ap); + } }
752-768: Make mout realloc logic robust to OOM and avoid pointer loss; also apply const.Current code overwrites zText with the result of realloc/malloc without checking success, leaking the previous buffer on failure and breaking subsequent writes.
-static void mout(const char *zNewText, int nNewChar, void *arg) { +static void mout(const char *zNewText, int nNewChar, void *arg) { struct sgMprintf *pM = (struct sgMprintf *)arg; - if (pM->nChar + nNewChar + 1 > pM->nAlloc) { - pM->nAlloc = pM->nChar + nNewChar * 2 + 1; - if (pM->zText == pM->zBase) { - pM->zText = psyqo_malloc(pM->nAlloc); - if (pM->zText && pM->nChar) __builtin_memcpy(pM->zText, pM->zBase, pM->nChar); - } else { - pM->zText = psyqo_realloc(pM->zText, pM->nAlloc); - } - } + if (pM->nChar + nNewChar + 1 > pM->nAlloc) { + int needed = pM->nChar + nNewChar + 1; + int newAlloc = needed * 2; /* grow */ + if (pM->zText == pM->zBase) { + char *newp = (char *)psyqo_malloc(newAlloc); + if (newp) { + if (pM->nChar) __builtin_memcpy(newp, pM->zBase, (size_t)pM->nChar); + pM->zText = newp; + pM->nAlloc = newAlloc; + } else { + /* OOM: keep using base buffer; drop new input chunk */ + nNewChar = 0; + } + } else { + char *newp = (char *)psyqo_realloc(pM->zText, newAlloc); + if (newp) { + pM->zText = newp; + pM->nAlloc = newAlloc; + } else { + /* OOM: keep existing buffer; drop new input chunk */ + nNewChar = 0; + } + } + } if (pM->zText) { __builtin_memcpy(&pM->zText[pM->nChar], zNewText, nNewChar); pM->nChar += nNewChar; pM->zText[pM->nChar] = 0; } }
🧹 Nitpick comments (1)
src/mips/psyqo/src/xprintf.c (1)
703-719: Flatten sout and avoid nested loops; use memcpy with bounds.Minor cleanup and speed-up. Also prepares for n==0 handling in vsnprintf.
-static void sout(const char *txt, int amt, void *arg) { - register char *head; - register const char *t; - register int a; - register char *tail; - a = amt; - t = txt; - head = ((struct s_strargument *)arg)->next; - tail = ((struct s_strargument *)arg)->last; - if (tail) { - while (a-- > 0 && head < tail) *(head++) = *(t++); - } else { - while (a-- > 0) *(head++) = *(t++); - } - *head = 0; - ((struct s_strargument *)arg)->next = head; -} +static void sout(const char *txt, int amt, void *arg) { + register char *head = ((struct s_strargument *)arg)->next; + register char *tail = ((struct s_strargument *)arg)->last; + int tocopy = amt; + if (tail) { + int cap = (int)(tail - head); + if (cap < tocopy) tocopy = cap; + } + if (tocopy > 0) __builtin_memcpy(head, txt, (size_t)tocopy); + head += (tocopy > 0 ? tocopy : 0); + if (!tail || head <= tail) *head = 0; + ((struct s_strargument *)arg)->next = head; +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/mips/psyqo/src/xprintf.c(3 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/xprintf.c
[warning] 703-703: ❌ New issue: Bumpy Road Ahead
sout has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: macos-arm-build-and-test
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
- GitHub Check: build
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: aur-build
nicolasnoble
force-pushed
the
brew-hotfix
branch
from
f1b5ad5 to
d6e1526
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mips/psyqo/src/xprintf.c (1)
728-734: vsnprintf UB when n == 0 (writes through buf and forms &buf[-1]).Per C99/C11, vsnprintf may be called with n==0 and buf==NULL; current code dereferences buf and creates an invalid pointer.
Apply this patch (adds a no-op sink and guards n==0):
@@ struct s_strargument { /* Describes the string being written to */ char *next; /* Next free slot in the string */ char *last; /* Last available slot in the string */ }; +/* Callback that discards output but lets vxprintf count. */ +static void nout(const char *txt, int amt, void *arg) { + (void)txt; (void)amt; (void)arg; +} @@ int vsnprintf(char *buf, size_t n, const char *fmt, va_list ap) { - struct s_strargument arg; - arg.next = buf; - arg.last = &buf[n - 1]; - *buf = 0; - return vxprintf(sout, &arg, fmt, ap); + if (n == 0 || buf == NULL) { + /* Count only; do not touch buf. */ + return vxprintf(nout, NULL, fmt, ap); + } + struct s_strargument arg; + arg.next = buf; + arg.last = &buf[n - 1]; + *buf = 0; + return vxprintf(sout, &arg, fmt, ap); }
♻️ Duplicate comments (1)
src/mips/psyqo/src/xprintf.c (1)
318-318: Const-correct callback signature — fix looks good and resolves prior UB risk.vxprintf now takes void ()(const char, int, void*), matching sout/mout updates. This addresses the earlier function-pointer type mismatch.
Run to confirm all declarations/prototypes match and no stale casts remain:
#!/bin/bash # Declarations rg -nP --type=c --type=cpp -C2 $'int\\s+vxprintf\\s*\\(\\s*void\\s*\\(\\*\\s*func\\s*\\)\\s*\\(\\s*const\\s+char\\s*\\*,\\s*int,\\s*void\\s*\\*\\s*\\)\\s*,\\s*void\\s*\\*,\\s*const\\s+char\\s*\\*,\\s*va_list\\s*\\)' # Any other prototypes rg -nP --type=c --type=cpp -C2 '\\bvxprintf\\s*\\(' # Suspicious casts to silence mismatches rg -nP --type=c --type=cpp -C2 '\(void\s*\(\*\)\(const?\s*char\s*\*,\s*int,\s*void\s*\*\)\)|\(void\s*\(\*\)\(char\s*\*,\s*int,\s*void\s*\*\)'
🧹 Nitpick comments (2)
src/mips/psyqo/src/xprintf.c (2)
703-719: sout const-correct; optional micro-optimization.When tail is null (unbounded), use memcpy to avoid the byte-at-a-time loop.
static void sout(const char *txt, int amt, void *arg) { @@ - if (tail) { - while (a-- > 0 && head < tail) *(head++) = *(t++); - } else { - while (a-- > 0) *(head++) = *(t++); - } + if (tail) { + while (a-- > 0 && head < tail) *(head++) = *(t++); + } else { + __builtin_memcpy(head, t, a); + head += a; + }
752-768: mout growth logic: add bounds/overflow guard and avoid double-realloc churn.Prevent negative/overflowing nAlloc and early-return on nNewChar<=0. Also grow to max(required, 2x).
static void mout(const char *zNewText, int nNewChar, void *arg) { struct sgMprintf *pM = (struct sgMprintf *)arg; - if (pM->nChar + nNewChar + 1 > pM->nAlloc) { - pM->nAlloc = pM->nChar + nNewChar * 2 + 1; - if (pM->zText == pM->zBase) { - pM->zText = psyqo_malloc(pM->nAlloc); - if (pM->zText && pM->nChar) __builtin_memcpy(pM->zText, pM->zBase, pM->nChar); - } else { - pM->zText = psyqo_realloc(pM->zText, pM->nAlloc); - } - } + if (nNewChar <= 0) return; + size_t needed = (size_t)pM->nChar + (size_t)nNewChar + 1u; + if ((size_t)pM->nAlloc < needed) { + size_t newAlloc = pM->nAlloc > 0 ? (size_t)pM->nAlloc * 2u : needed; + if (newAlloc < needed) newAlloc = needed; + char *newText; + if (pM->zText == pM->zBase) { + newText = (char *)psyqo_malloc(newAlloc); + if (newText && pM->nChar) __builtin_memcpy(newText, pM->zBase, pM->nChar); + } else { + newText = (char *)psyqo_realloc(pM->zText, newAlloc); + } + if (!newText) return; /* OOM: leave state unchanged */ + pM->zText = newText; + pM->nAlloc = (int)newAlloc; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/mips/psyqo/src/xprintf.c(3 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/xprintf.c
[warning] 703-703: ❌ New issue: Bumpy Road Ahead
sout has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: aur-build
- GitHub Check: cross-arm64
- GitHub Check: build-openbios
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: toolchain
- GitHub Check: macos-build-and-test-toolchain
nicolasnoble
changed the title
1 participant
Many breakages from github updating the "macos-latest" image label.
Also fixes https://github.com/orgs/Homebrew/discussions/6351