Add macOS build and runtime compatibility fixes#785
Merged
jensen-yan merged 3 commits intoxs-devfrom Mar 16, 2026
Merged
Conversation
Make the XiangShan gem5 tree build and run on macOS/Apple Silicon. This updates the Darwin toolchain setup, fixes runtime library lookup, and addresses a set of clang/libc++ and Python 3.14 compatibility issues that blocked local builds. It also adjusts DRAMsim3 and shared-memory handling for macOS, documents the degraded difftest loading semantics on Darwin, and fixes a handful of format-string and case-sensitivity issues exposed by the platform.
📝 WalkthroughWalkthroughAdds macOS-aware build and runtime handling, fixes C++17/template issues, corrects 64-bit format specifiers, centralizes platform-aware difftest library loading, introduces platform-specific shared-memory code and state, replaces deprecated Python API, and renames a debug flag. (50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f35a9495e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
SConstruct (1)
540-552: Consider breaking after finding the first valid Homebrew prefix.The current implementation adds both
/opt/homebrew(Apple Silicon) and/usr/local(Intel Mac) to the paths if both directories exist. While rare, this could lead to library version conflicts if both prefixes contain different versions of the same dependency. Typically, only one Homebrew prefix is active on a given system.♻️ Suggested fix to use only the first valid Homebrew prefix
# Homebrew installs headers and libraries under a non-system # prefix on macOS (e.g., /opt/homebrew on Apple Silicon). Add the # common prefixes here so Configure checks can find dependencies # such as zstd without requiring per-shell environment setup. for brew_prefix in ('/opt/homebrew', '/usr/local'): if os.path.isdir(brew_prefix): env.Prepend(CPPPATH=[os.path.join(brew_prefix, 'include')]) env.Prepend(LIBPATH=[os.path.join(brew_prefix, 'lib')]) env['ENV']['PKG_CONFIG_PATH'] = \ os.path.join(brew_prefix, 'lib', 'pkgconfig') + \ (':' + env['ENV']['PKG_CONFIG_PATH'] if 'PKG_CONFIG_PATH' in env['ENV'] and env['ENV']['PKG_CONFIG_PATH'] else '') + break # Use only the first valid Homebrew prefix🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SConstruct` around lines 540 - 552, Loop through brew_prefixes but stop after the first existing prefix to avoid adding both /opt/homebrew and /usr/local; in the SConstruct loop that iterates over ('/opt/homebrew', '/usr/local') (variable brew_prefix) check os.path.isdir(brew_prefix), apply env.Prepend(CPPPATH=...), env.Prepend(LIBPATH=...), and update env['ENV']['PKG_CONFIG_PATH'] as currently done, then break out of the loop (use a break) so only the first valid Homebrew prefix is used.src/sim/arch_db.cc (1)
195-198: Minor: Unnecessary casts forintparameters.The
wayandis_writeparameters areinttypes, so casting them tounsigned long longand using%lluis overkill. Using%dfor these integer parameters would be more appropriate and cleaner.♻️ Suggested improvement
sprintf(sql, "INSERT INTO dcacheWayPreTrace(PC,VADDR, WAY, Tick, IsWrite,SITE)" - "VALUES(%llu,%llu,%llu,%llu,%llu,'%s');", - static_cast<unsigned long long>(pc), static_cast<unsigned long long>(vaddr), - static_cast<unsigned long long>(way), static_cast<unsigned long long>(tick), - static_cast<unsigned long long>(is_write), "dacheWayPre"); + "VALUES(%llu,%llu,%d,%llu,%d,'%s');", + static_cast<unsigned long long>(pc), static_cast<unsigned long long>(vaddr), + way, static_cast<unsigned long long>(tick), + is_write, "dacheWayPre");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sim/arch_db.cc` around lines 195 - 198, The SQL value formatting currently casts way and is_write to unsigned long long and uses "%llu"; update the format string and arguments so that way and is_write use "%d" and pass them as ints (remove the static_cast<unsigned long long> for way and is_write) while keeping the existing casts for pc, vaddr, and tick; locate the VALUES(...) call in arch_db.cc that builds the SQL (the line containing "VALUES(%llu,%llu,%llu,%llu,%llu,'%s');") and change the two %llu placeholders for way and is_write to %d and stop casting those two variables to unsigned long long.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/difftest.cc`:
- Around line 43-57: openDiffTestHandle uses dlopen on macOS which will return
the same loaded image on repeated calls, causing ref-side globals to be shared
and corrupt multi-core difftest; modify openDiffTestHandle to add a static guard
that tracks whether the ref_so has already been loaded on macOS and, if a second
load is attempted, fail early (return nullptr or abort with a clear error)
instead of calling dlopen again; reference openDiffTestHandle and the multi-core
initialization path (e.g., NemuProxy::initState and its use of
difftest_set_mhartid/multiCore) so the guard prevents subsequent loads when
multiCore is enabled and documents the failure path for callers.
---
Nitpick comments:
In `@SConstruct`:
- Around line 540-552: Loop through brew_prefixes but stop after the first
existing prefix to avoid adding both /opt/homebrew and /usr/local; in the
SConstruct loop that iterates over ('/opt/homebrew', '/usr/local') (variable
brew_prefix) check os.path.isdir(brew_prefix), apply env.Prepend(CPPPATH=...),
env.Prepend(LIBPATH=...), and update env['ENV']['PKG_CONFIG_PATH'] as currently
done, then break out of the loop (use a break) so only the first valid Homebrew
prefix is used.
In `@src/sim/arch_db.cc`:
- Around line 195-198: The SQL value formatting currently casts way and is_write
to unsigned long long and uses "%llu"; update the format string and arguments so
that way and is_write use "%d" and pass them as ints (remove the
static_cast<unsigned long long> for way and is_write) while keeping the existing
casts for pc, vaddr, and tick; locate the VALUES(...) call in arch_db.cc that
builds the SQL (the line containing "VALUES(%llu,%llu,%llu,%llu,%llu,'%s');")
and change the two %llu placeholders for way and is_write to %d and stop casting
those two variables to unsigned long long.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec51d356-91d0-4537-8068-517e89a4ab00
📒 Files selected for processing (20)
SConstructext/drampower/src/CmdScheduler.hext/dramsim3/SConscriptsite_scons/gem5_scons/builders/add_local_rpath.pysrc/SConscriptsrc/arch/riscv/insts/vector.hhsrc/arch/riscv/isa/formats/fp.isasrc/arch/riscv/isa/vector/base/vector_arith.isasrc/arch/riscv/isa/vector/simple/vector_arith.isasrc/cpu/difftest.ccsrc/cpu/pred/SConscriptsrc/cpu/pred/general_arch_db.ccsrc/cpu/pred/tage.ccsrc/cpu/pred/tage_base.ccsrc/mem/cache/prefetch/cmc.ccsrc/mem/mem_util.ccsrc/mem/mem_util.hhsrc/mem/physical.ccsrc/python/m5/main.pysrc/sim/arch_db.cc
💤 Files with no reviewable changes (1)
- src/cpu/pred/SConscript
Change-Id: I21d87980fb91b9760f87f226d6859441e916b150
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Collaborator
Author
|
当前还没完成实现macos 上的difftest, 可以先关闭difftest 来做测试。 |
Change-Id: I27736c6fd4ca8132fcd6934d05457d9f7cd0d05b
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sim/arch_db.cc`:
- Around line 11-15: The function sqliteSignedInt currently casts uint64_t
values above INT64_MAX into negative numbers; add a runtime guard in
sqliteSignedInt that checks if value <= static_cast<uint64_t>(INT64_MAX) and
only then returns static_cast<long long>(static_cast<int64_t>(value)); otherwise
signal an error (e.g., throw std::overflow_error or std::out_of_range with a
clear message) to prevent silent wraparound; ensure any headers needed for the
exception ( <stdexcept> ) are included and callers handle or propagate the
exception appropriately.
- Around line 186-189: The code builds an SQL string into a fixed char sql[512]
with sprintf using site (from this->name().c_str()) which can overflow; replace
the sprintf call with snprintf(sql, sizeof(sql), "VALUES(%lld, %lld, %lld, %lld,
%lld, '%s');", sqliteSignedInt(pc), sqliteSignedInt(source),
sqliteSignedInt(paddr), sqliteSignedInt(vaddr), sqliteSignedInt(stamp), site)
and then validate the returned length (int n) is >= 0 and < (int)sizeof(sql); if
it’s >= sizeof(sql) handle truncation safely (e.g., log error and skip insert or
truncate site) and ensure sql is NUL-terminated before using it in the insert
routine in arch_db.cc where sql, sprintf, site, this->name().c_str(), and
sqliteSignedInt(...) are referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 998e0d93-fa52-4159-9ebc-f39646ca5fe9
📒 Files selected for processing (3)
SConstructsrc/cpu/pred/general_arch_db.ccsrc/sim/arch_db.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- SConstruct
- src/cpu/pred/general_arch_db.cc
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make the XiangShan gem5 tree build and run on macOS/Apple Silicon.
This updates the Darwin toolchain setup, fixes runtime library lookup, and addresses a set of clang/libc++ and Python 3.14 compatibility issues that blocked local builds.
It also adjusts DRAMsim3 and shared-memory handling for macOS, documents the degraded difftest loading semantics on Darwin, and fixes a handful of format-string and case-sensitivity issues exposed by the platform.
Summary by CodeRabbit
New Features
Bug Fixes
Chores