support ChampSim and CBP2025 trace simulation

[Lingrui98]

instructions are synthesized to RISC-V instructions, wrong-path are currently filled with nops.

refer to docs/tools/trace/trace_tools.md and src/cpu/o3/trace/README.md for more information

Summary by CodeRabbit

• New Features

  • End-to-end trace-driven simulation mode: ChampSim & CBP2025 readers, checkpoint/rollback, BP validation, wrong-path handling, and per-CPU trace configuration (file/format, address mapping, timing/PTW, mispredict controls).

• Tools

  • New trace utilities: dump, count, analyze, extract/align events, batch/parallel/distributed schedulers, rerun helpers, and helper scripts for trace workflows.

• Tests

  • Unit tests added for ChampSim trace reader.

• Documentation

  • Comprehensive docs, READMEs, usage guides, and developer notes for trace tooling and integration.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[github-actions]

🚀 Coremark Smoke Test Results

Branch	IPC	Change
Base (xs-dev)	2.1773	-
This PR	2.1773	➡️ 0.0000 (0.00%)

✅ Difftest smoke test passed!

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (6)

util/ClockAnalysis.py (1)

38-42: LGTM! Improved readability.

The multi-line formatting makes the SQL query more readable. The static analysis warning about SQL injection (S608) is a false positive here since start_clock, end_clock, and period are all type-checked as integers by argparse, preventing injection.

As a defensive coding practice, consider using parameterized queries instead of f-string formatting:

🔎 Optional refactor to parameterized query

-        df = pd.read_sql_query(
-            f"SELECT * FROM LifeTimeCommitTrace where AtCommit != 0 "
-            f"{clock_pick_cmd} ORDER BY ID ASC",
-            con,
-        )
+        params = {"start_clock": start_clock * period}
+        query = "SELECT * FROM LifeTimeCommitTrace WHERE AtCommit != 0 AND AtCommit >= :start_clock "
+        if end_clock >= start_clock:
+            params["end_clock"] = end_clock * period
+            query += "AND AtCommit <= :end_clock "
+        query += "ORDER BY ID ASC"
+        df = pd.read_sql_query(query, con, params=params)

src/cpu/o3/dyn_inst.hh (1)

641-695: Harden trace branch accessors against stale state

The trace branch fields are reset in clearTraceBranchInfo() and only populated via setTraceBranchInfo(), which is good. However, traceBranchTarget() ignores traceBranchInfoValid while the other accessors gate on it. That’s safe as long as all callers check hasTraceBranchInfo() / traceBranchHasTarget() first, but it’s easy to misuse later.

Consider tightening invariants by also gating traceBranchTarget() on traceBranchInfoValid (or documenting that callers must only use it when hasTraceBranchInfo() is true).

src/cpu/o3/SConscript (1)

63-68: Trace reader sources and tests are wired reasonably; double‑check duplicate stats/time objects

The new Source('trace/...') entries and DebugFlag('TraceReader') under non‑NULL ISA look fine, and the champsim_trace_reader.test wiring for both NULL and non‑NULL builds matches typical gem5 GTest patterns.

One thing to verify: in the non‑NULL branch you explicitly compile several stats/time/core objects (../../base/stats/*.cc, ../../sim/root.cc, ../../base/time.cc, etc.) that are likely also part of the main gem5 libraries linked into the test. If those symbols are already provided by the libraries, explicitly listing the same sources here may cause multiple-definition linker errors in some configurations.

Consider reusing the existing libs where possible, or at least confirm that these units are not otherwise linked into the test binary.

Also applies to: 93-93, 103-150

src/cpu/o3/fetch.cc (1)

2151-2159: Avoid retaining references to traceForThisInst inside TraceFetch

processSingleInstruction creates a stack TraceInstruction traceForThisInst and passes it by reference to bindPendingTraceMetadata, then later to postBranchPredict. That’s fine as long as TraceFetch only copies from traceForThisInst and never stores a pointer or reference to it beyond the scope of this call.

Please double‑check that TraceFetch never retains the reference; if it does, switch the interface to copy TraceInstruction by value (or use an internal owned object) to avoid dangling references.

Also applies to: 2188-2192

src/cpu/o3/commit.cc (1)

1488-1499: Trace commit difftest is rigorous; confirm index sentinel and coverage assumptions

traceCommitDifftest does a thorough job:

• Aligns traceCommitIndex[tid] on first use via getTraceIndexForSeqNum(sn).
• Compares commit PC to the trace PC (from per‑seqNum metadata when present, otherwise via getTracePCByIndex(traceCommitIndex[tid])), and panics on mismatch.
• Requires that every committed instruction in trace mode has metadata (isTraceInstruction(sn)), panicking if not.
• Classifies instruction type from StaticInst and compares to TraceInstruction::InstType, with a small FP/compressed special case, and panics on mismatch.
• Optionally logs non‑fatal mismatches for memory address/size and branch behavior.
• Bumps traceCommitIndex[tid] once per architecturally visible instruction boundary (onInstBoundary).

A couple of things to verify given how strict this is:

1. Index 0 as sentinel

   traceCommitIndex[tid] is initialized to 0 and later used both as:

   • “no mapping yet” sentinel in if (traceCommitIndex[tid] == 0 && mapped_idx != 0), and
   • argument to getTracePCByIndex(traceCommitIndex[tid]) when no metadata is found.

   This assumes that TraceFetch never uses index 0 for a valid trace entry (i.e., indices are 1‑based or 0 is otherwise reserved). Please confirm that findTraceIndexForSeqNum / getTracePCByIndex enforce this convention; otherwise, a missing per‑seqNum mapping could accidentally be interpreted as “use trace[0]”.

2. Coverage of helper / non‑traced instructions

   The unconditional panic on !cpu->isTraceInstruction(head_inst->seqNum) assumes that, in trace mode, every committed instruction corresponds to a trace entry. If you ever commit helper uops, trap‑only carriers, or other non‑trace instructions in trace mode, they will trip this check. If such instructions are expected, consider tagging them and skipping difftest for those seqNums instead of panicking.

If both of these assumptions hold in your trace flow, the difftest logic itself looks solid.

Also applies to: 1547-1549, 1615-1847

src/cpu/o3/trace/TraceFetch.cc (1)

1061-1131: Consider extracting encoding helpers to a separate utility.

The createMachInstFromTrace function is ~340 lines with many local lambdas for RISC-V instruction encoding (encode_b_imm, encode_j_imm, encode_cj_imm, etc.). While the current structure works, extracting these to a separate encoding utility class or namespace would:

• Improve testability of individual encoders
• Reduce cognitive load when reading the main function
• Enable reuse if other components need instruction synthesis

This is not blocking, as the current implementation is functionally correct with good defensive clamping and fallback handling.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 24d73b0 and 060150a.

📒 Files selected for processing (17)

• .github/workflows/manual-perf.yml
• README.md
• src/cpu/o3/BaseO3CPU.py
• src/cpu/o3/SConscript
• src/cpu/o3/commit.cc
• src/cpu/o3/cpu.hh
• src/cpu/o3/dyn_inst.hh
• src/cpu/o3/fetch.cc
• src/cpu/o3/fetch.hh
• src/cpu/o3/lsq_unit.cc
• src/cpu/o3/trace/TraceFetch.cc
• src/cpu/o3/trace/TraceFetch.hh
• src/cpu/o3/trace/TraceReader.cc
• src/cpu/pred/btb/btb_ittage.cc
• src/cpu/pred/btb/btb_tage.cc
• util/ClockAnalysis.py
• util/loadtrace.py

✅ Files skipped from review due to trivial changes (4)

• src/cpu/pred/btb/btb_ittage.cc
• README.md
• .github/workflows/manual-perf.yml
• src/cpu/pred/btb/btb_tage.cc

🚧 Files skipped from review as they are similar to previous changes (1)

• src/cpu/o3/lsq_unit.cc

🧰 Additional context used

🧬 Code graph analysis (7)

src/cpu/o3/trace/TraceFetch.hh (4)

src/cpu/o3/cpu.hh (19)

• fetch (374-374)
• fetch (377-380)
• fetch (628-632)
• tid (223-223)
• tid (226-226)
• tid (229-229)
• tid (232-232)
• tid (241-241)
• tid (244-244)
• tid (249-249)
• tid (261-261)
• tid (264-264)
• tid (279-279)
• tid (299-299)
• tid (371-371)
• tid (393-393)
• tid (401-401)
• tid (410-410)
• tid (572-572)

src/cpu/o3/fetch.cc (2)

• fetch (2048-2070)
• fetch (2049-2049)

src/cpu/o3/trace/TraceFetch.cc (2)

• TraceFetch (48-84)
• TraceFetch (86-86)

src/cpu/o3/dyn_inst.hh (2)

• next_pc (702-710)
• predPC (630-630)

src/cpu/o3/BaseO3CPU.py (2)

src/cpu/o3/trace/TraceReader.hh (1)

• traceFile (101-101)

src/cpu/o3/trace/TraceFetch.hh (2)

• traceBPValidation (123-123)
• traceEnableWrongPath (124-124)

src/cpu/o3/commit.cc (1)

src/cpu/o3/trace/TraceInstruction.hh (4)

• target (198-201)
• target (198-198)
• target (205-209)
• target (205-205)

src/cpu/o3/cpu.hh (1)

src/cpu/o3/trace/TraceFetch.hh (9)

• seqNum (111-112)
• seqNum (113-113)
• seqNum (114-114)
• seqNum (116-116)
• seqNum (117-117)
• seqNum (118-118)
• seqNum (119-119)
• seqNum (145-145)
• index (120-120)

src/cpu/o3/dyn_inst.hh (1)

src/cpu/o3/trace/TraceInstruction.hh (7)

• taken (197-197)
• taken (197-197)
• branchTarget (158-158)
• v (204-204)
• v (204-204)
• v (237-237)
• v (237-237)

src/cpu/o3/trace/TraceFetch.cc (7)

src/cpu/o3/trace/TraceFetch.hh (11)

• TraceFetch (73-73)
• TraceFetch (74-74)
• traceMode (76-76)
• traceMode (77-77)
• traceDecoupledFrontend (78-78)
• traceEnableWrongPath (124-124)
• traceBPValidation (123-123)
• traceWrongPathActive (125-125)
• instruction (149-151)
• traceInstr (131-131)
• traceInstr (147-148)

src/cpu/o3/trace/TraceReader.cc (2)

• createTraceReader (638-669)
• createTraceReader (639-642)

src/cpu/o3/cpu.hh (23)

• fetch (374-374)
• fetch (377-380)
• fetch (628-632)
• tid (223-223)
• tid (226-226)
• tid (229-229)
• tid (232-232)
• tid (241-241)
• tid (244-244)
• tid (249-249)
• tid (261-261)
• tid (264-264)
• tid (279-279)
• tid (299-299)
• tid (371-371)
• tid (393-393)
• tid (401-401)
• tid (410-410)
• tid (572-572)
• inst (398-398)
• inst (406-406)
• inst (616-625)
• inst (617-620)

src/mem/port.hh (1)

• warn (329-337)

src/cpu/o3/dyn_inst.hh (2)

• predPC (630-630)
• next_pc (702-710)

src/cpu/o3/trace/TraceInstruction.hh (2)

• srcRegs (166-166)
• dstRegs (167-167)

src/arch/riscv/insts/standard.hh (1)

• imm (72-74)

src/cpu/o3/fetch.hh (4)

src/cpu/o3/fetch.cc (2)

• Fetch (94-171)
• Fetch (173-173)

src/cpu/o3/cpu.hh (5)

• seqNum (267-267)
• seqNum (268-268)
• seqNum (271-271)
• seqNum (272-272)
• index (273-273)

src/cpu/o3/trace/TraceFetch.hh (10)

• seqNum (111-112)
• seqNum (113-113)
• seqNum (114-114)
• seqNum (116-116)
• seqNum (117-117)
• seqNum (118-118)
• seqNum (119-119)
• seqNum (145-145)
• seqNum (146-146)
• index (120-120)

src/cpu/o3/trace/TraceInstruction.hh (1)

• seqNum (168-168)

🪛 Cppcheck (2.19.0)

src/cpu/o3/commit.cc

[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

src/cpu/o3/trace/TraceReader.cc

[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

---

[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

src/cpu/o3/trace/TraceFetch.cc

[error] 119-119: Reference to temporary returned.

(returnTempReference)

---

[error] 132-132: Reference to temporary returned.

(returnTempReference)

---

[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

---

[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

🪛 Ruff (0.14.10)

util/ClockAnalysis.py

39-40: Possible SQL injection vector through string-based query construction

(S608)

src/cpu/o3/BaseO3CPU.py

257-257: Param may be undefined, or defined from star imports

(F405)

---

258-258: Param may be undefined, or defined from star imports

(F405)

---

259-259: Param may be undefined, or defined from star imports

(F405)

---

260-260: Param may be undefined, or defined from star imports

(F405)

---

261-261: Param may be undefined, or defined from star imports

(F405)

---

262-262: Param may be undefined, or defined from star imports

(F405)

---

265-265: Param may be undefined, or defined from star imports

(F405)

---

266-266: Param may be undefined, or defined from star imports

(F405)

---

267-267: Param may be undefined, or defined from star imports

(F405)

---

268-268: Param may be undefined, or defined from star imports

(F405)

---

271-271: Param may be undefined, or defined from star imports

(F405)

---

276-276: Param may be undefined, or defined from star imports

(F405)

---

280-280: Param may be undefined, or defined from star imports

(F405)

---

286-286: Param may be undefined, or defined from star imports

(F405)

---

290-290: Param may be undefined, or defined from star imports

(F405)

⏰ 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: Quick Build, Unit Tests & Smoke Test

🔇 Additional comments (23)

src/cpu/o3/dyn_inst.hh (2)

166-168: Last-trace flag wiring looks consistent

lastTraceInstFlag plus setLastTraceInst() / isLastTraceInst() cleanly expose the “last in trace” marker used by Commit; no functional issues spotted here.

Also applies to: 696-700

---

858-868: Trace-aware branchTarget override is reasonable

Using traceBranchHasTarget() to decide whether to build a concrete TheISA::PCState from traceBranchTarget() and otherwise falling back to staticInst->branchTarget(*pc) preserves pre-existing behavior outside trace mode while allowing trace targets to override static decoding. No issues here assuming TheISA::PCState is available via config/the_isa.hh, which it is.

util/loadtrace.py (1)

60-66: Multi-line SQL refactor is semantics-preserving

The multi-line string concatenation keeps the query identical while improving readability; no behavioral changes or issues.

src/cpu/o3/cpu.hh (1)

93-94: Trace-mode CPU facade API is coherent; ensure oldest-seq computation matches contract

The added CPU‑level methods (trace metadata queries, isTraceMode/isTraceEOF, isTracePipelineDrained, getOldestInFlightSeqNum) form a clean facade over Fetch/TraceFetch and keep the drain check encapsulated.

One thing to verify on the implementation side (in cpu.cc): getOldestInFlightSeqNum() should return the minimum seqNum over instList, and return std::numeric_limits<InstSeqNum>::max() (or equivalent) when instList is empty, as the comment states. The sliding‑window cleanup in cleanupTraceMetadataOnCommit() relies on that contract to avoid prematurely dropping metadata for still‑in‑flight instructions.

Also applies to: 266-274, 373-388, 435-440

src/cpu/o3/fetch.cc (7)

169-201: TraceFetch construction and mode helpers look sane

Constructing traceFetch once in the Fetch ctor and using isTraceMode() / isTraceEOF() as thin wrappers around it is straightforward. The trace-aware isDecoupledFrontend() check (respecting branchPred->isDecoupled() and allowDecoupledFrontend() in trace mode) also looks logically consistent with decoupled FTQ semantics.

---

299-307: Trace metadata stats are well-integrated

The added counters (traceMetaStores, traceMetaCleanupSquash*, traceMetaCleanupCommitCalls) are properly registered with units and prereq, and sit naturally alongside the other Fetch stats. No correctness issues here.

Also applies to: 381-388

---

428-431: Stage reset now correctly reinitializes trace state

Calling resetStage() to:

• reset per-thread fetch state,
• recompute usedUpFetchTargets = isDecoupledFrontend(), and
• delegate to traceFetch->resetStage()

ensures the new trace-mode state is consistent with the existing decoupled frontend machinery after startup or a takeover. The diagnostic DPRINTF is helpful and harmless.

Also applies to: 481-486, 487-490

---

2082-2087: Trace-mode memory supply path cleanly bypasses fetchBuffer

In checkMemoryNeeds, short‑circuiting to traceFetch->checkMemoryNeeds() under isTraceMode() cleanly separates trace‑driven instruction byte supply from the normal icache/fetchBuffer path, while leaving the RISC‑V 4‑byte supply logic unchanged for non‑trace runs. This looks correct as long as TraceFetch::checkMemoryNeeds mirrors the decoder’s expectations.

Also applies to: 2108-2116

---

1522-1555: Decoupled-frontend BP tick uses the right PC source

Using bp_pc = fetchBuffer[0].startPC when the decoupled frontend is enabled and the fetch buffer is valid, and falling back to pc[0]->instAddr() otherwise, is a sensible way to keep FTQ/BPU state aligned with what fetch is about to consume. The usedUpFetchTargets update and the extra trace-mode diagnostics are consistent with the rest of the decoupled frontend logic.

---

2499-2525: FTQ entry allocation in decoupled+trace mode is subtle but looks coherent

needNewFTQEntry() now always considers usedUpFetchTargets || !fetchBuffer[tid].valid, and when isTraceMode() && isDecoupledFrontend() it emits trace diagnostics but still drives the same decision. getNextFTQStartPC() immediately tries to supply a new FTQ entry when usedUpFetchTargets is true, resets the flag on success, and then derives startPC from the current supplying entry.

This matches the intent of letting trace mode still exercise the FTQ/BTB/FTB path. Just be aware that correctness now depends on the invariants:

• usedUpFetchTargets is always set when an FTQ entry is exhausted, and
• fetchBuffer[tid].valid is only set once icache data for that FTQ entry has arrived.

Those look to be maintained elsewhere, but it’s worth keeping in mind when debugging trace/FTQ mismatches.

Also applies to: 2531-2557

---

2754-2794: Fetch–CPU trace metadata forwarding is straightforward

The forwarding methods (getTraceInstMetadata, isTraceInstruction, cleanupTraceMetadataOnCommit, findTraceIndexForSeqNum, lookupTraceIndexForSeqNum, getTracePCByIndex) are thin wrappers around traceFetch with safe null‑checks and sensible fallbacks (nullptr/false/0). This is a clean way to expose trace metadata to CPU/Commit.

src/cpu/o3/commit.cc (2)

133-144: Trace-mode clean exits avoid artificial “stuck” tails

Two new early‑exit paths are sensible for trace‑driven runs:

• In stuckCheckEvent, when cpu->isTraceMode() and cpu->isTracePipelineDrained(), you now warn and exitSimLoop(...) instead of panicking on a “stuck” commit.
• In Commit::tick(), when isTraceMode() && isTraceEOF() && isTracePipelineDrained(), you also exit cleanly.
• Additionally, after a successful commit you exit immediately if head_inst->isLastTraceInst() is set.

Together, these prevent long idle tails or false “CommitStuck” panics once the trace stream is exhausted or the last traced instruction has committed. No correctness issues here.

Also applies to: 918-927, 1365-1372

---

94-116: The traceCtrlFaultPending and traceCtrlFaultSeqNum arrays are already properly initialized at declaration in commit.hh (lines 195-196) using uniform initialization syntax {}, which zero-initializes both the boolean and integer arrays. This provides the guarantees needed and is the modern C++ approach. No additional constructor-level initialization is required.

Likely an incorrect or invalid review comment.

src/cpu/o3/BaseO3CPU.py (1)

255-291: Well-structured trace-mode configuration parameters.

The new trace configuration parameters are comprehensive and well-documented, covering:

• Core trace mode controls (enableTraceMode, traceFile, traceFormat)
• Address mapping configuration (traceAddrMapMode, traceAddrBase, traceAddrSize, traceAddrPageAlign)
• Branch predictor training and validation (traceTrainBranches, traceBPValidation, traceMispredictPenalty)
• Wrong-path simulation (traceEnableWrongPath, traceWrongPathUseTraceInst)

The static analysis warnings about Param being undefined (F405) are false positives—Param is correctly imported via from m5.params import * on line 40, which follows the standard GEM5 configuration pattern.

src/cpu/o3/trace/TraceFetch.hh (1)

70-214: Clean TraceFetch class design with well-defined interface.

The TraceFetch class is well-structured:

• Clear separation between public delegation surface (lines 72-125) and private implementation (lines 127-213)
• Good use of std::unique_ptr for traceReader ownership
• In-class member initializers provide sensible defaults
• The "friend-style helper" pattern is documented, explaining the need for Fetch internals access

The interface aligns correctly with the TraceFetch.cc implementation and integrates with the TraceReader/TraceInstruction abstractions.

src/cpu/o3/trace/TraceReader.cc (2)

604-618: Silent instruction drop when buffer is full may cause trace desync.

When the instruction buffer reaches MAX_BUFFER_SIZE, new instructions are silently dropped (logged via DPRINTF only). This could lead to trace desynchronization issues during replay if the consumer doesn't keep up.

Consider whether this should be a warn() or tracked via a statistic to help diagnose potential issues during simulation.

---

647-664: Remove redundant address mapping configuration in factory function.

Both ChampSimTraceReader and CBP2025TraceReader configure address mapping in their constructors (via setAddrMapConfig). The explicit setAddressMapping() call on line 655 for ChampSimTraceReader duplicates this initialization with identical parameters and should be removed.

Likely an incorrect or invalid review comment.

src/cpu/o3/trace/TraceFetch.cc (3)

48-84: TraceFetch constructor properly wires CPU params to trace modeling knobs.

The constructor correctly initializes all trace-mode state from BaseO3CPUParams, including mispredict penalty, wrong-path settings, and BP validation flags. The call to createTraceReader with address mapping configuration is properly guarded by traceMode check, and the fatal() on reader creation failure provides clear error reporting.

---

129-141: Trace mode initialization assumes single-threaded execution (tid=0).

The initialization hardcodes thread ID 0 when setting PC state and thread context:

• fetch.pc[0], fetch.cpu->pcState(*tracePC, 0)
• fetch.cpu->getContext(0)

This limits trace-driven simulation to single-threaded mode. If multi-threaded trace support is needed in the future, this will need refactoring.

---

426-572: Comprehensive trace squash handling with clear wrong-path semantics.

The handleTraceSquash function correctly handles multiple squash scenarios:

• Boundary squash from the mispredicted branch itself
• Squash from instructions prior to the mispredicted branch
• Non-instruction squashes (TLB faults, traps, replays)

The logic properly distinguishes between staying in wrong-path mode vs. exiting, and the rollback/cleanup sequencing is correct. The extensive DPRINTF logging will aid debugging.

src/cpu/o3/fetch.hh (3)

218-219: Proper TraceFetch integration with Fetch class.

The integration additions are well-structured:

• Forward declarations (lines 79-80) avoid circular includes
• Friend declaration (line 218) enables the documented "friend-style helper" pattern
• Explicit destructor (line 240) is required for unique_ptr<TraceFetch> with incomplete type
• std::unique_ptr<TraceFetch> member (lines 641-642) provides clear ownership semantics

Also applies to: 240-240, 641-642

---

500-506: Clean trace metadata accessor delegation to TraceFetch.

The trace metadata accessors provide a clean public interface for CPU/Commit components while delegating implementation to TraceFetch. The API surface includes:

• getTraceInstMetadata() / isTraceInstruction() for metadata queries
• cleanupTraceMetadataOnCommit() for lifecycle management
• findTraceIndexForSeqNum() / lookupTraceIndexForSeqNum() / getTracePCByIndex() for trace navigation

---

1139-1147: Useful trace metadata accounting statistics.

The new statistics provide visibility into trace metadata lifecycle:

• traceMetaStores: Tracks metadata record creation
• traceMetaCleanupSquash{Calls,Entries}: Monitors squash-triggered cleanup
• traceMetaCleanupCommitCalls: Tracks commit-path cleanup

These will be valuable for debugging memory growth or performance issues in trace-driven simulation mode.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

src/cpu/o3/SConscript (1)

104-151: Test configuration duplicates sources for NULL vs ISA builds.

The test scaffolding correctly handles both NULL and ISA-enabled configurations. The duplication of source files between the two GTest blocks is necessary because of differing dependency requirements (NULL builds need explicit stats/core objects, while ISA builds need additional time/sim_object infrastructure).

Consider adding a comment explaining why the duplication is intentional to prevent future maintainers from attempting to consolidate them.

src/cpu/o3/trace/CommitTrace.cc (2)

273-294: Extract duplicated InstType-to-string conversion.

The switch statement for converting InstType to a string is duplicated verbatim in both the logging path (lines 273-293) and the panic path (lines 301-321). This violates DRY and makes maintenance error-prone.

🔎 Proposed refactor: Extract helper function

Add a helper function at namespace scope or as a static method:

namespace {
const char* instTypeToString(o3::TraceInstruction::InstType type) {
    switch (type) {
        case o3::TraceInstruction::InstType::ALU: return "ALU";
        case o3::TraceInstruction::InstType::LOAD: return "LOAD";
        case o3::TraceInstruction::InstType::STORE: return "STORE";
        case o3::TraceInstruction::InstType::COND_BRANCH: return "COND_BRANCH";
        case o3::TraceInstruction::InstType::UNCOND_DIRECT_BRANCH: return "UNCOND_DIRECT_BRANCH";
        case o3::TraceInstruction::InstType::UNCOND_INDIRECT_BRANCH: return "UNCOND_INDIRECT_BRANCH";
        case o3::TraceInstruction::InstType::FP: return "FP";
        case o3::TraceInstruction::InstType::SLOW_ALU: return "SLOW_ALU";
        case o3::TraceInstruction::InstType::CALL_DIRECT: return "CALL_DIRECT";
        case o3::TraceInstruction::InstType::CALL_INDIRECT: return "CALL_INDIRECT";
        case o3::TraceInstruction::InstType::RETURN: return "RETURN";
        default: return "UNDEFINED";
    }
}
} // anonymous namespace

Then replace both inline lambdas with calls to instTypeToString(commit_type).

Also applies to: 301-322

---

236-256: Consider extracting classifyInstType as a reusable utility.

This lambda implements instruction type classification logic that may be useful elsewhere. Consider making it a static member function of TraceInstruction or a free function in the trace namespace for reuse and testability.

src/cpu/o3/commit.cc (1)

1298-1300: Indentation inconsistency.

The if statement and its body have inconsistent indentation compared to surrounding code. The if block appears to be indented one level too far.

🔎 Proposed fix

-                    if (traceMaybeExitOnLastTraceInst(head_inst)) {
-                        return;
-                    }
+                if (traceMaybeExitOnLastTraceInst(head_inst)) {
+                    return;
+                }

src/cpu/o3/commit.hh (1)

194-196: Consider making trace fault bookkeeping private.

traceCtrlFaultSeqNum and traceCtrlFaultPending are declared in the public section but appear to be internal implementation details used only by trace-mode commit logic. Moving them to the private section would provide better encapsulation.

🔎 Proposed fix

Move these declarations to the private section near line 590 where other trace-related members are declared:

-  public:
-    /** Trace ctrl-flow fault bookkeeping: seqNum to notify fetch rollback. */
-    InstSeqNum traceCtrlFaultSeqNum[MaxThreads]{};
-    bool traceCtrlFaultPending[MaxThreads]{};
-
     /** Sets pointer to list of active threads. */
     void setActiveThreads(std::list<ThreadID> *at_ptr);

And add to private section:

     // Trace-mode commit stream index per thread: expected next trace instruction index
     uint64_t traceCommitIndex[MaxThreads] = {0};

+    /** Trace ctrl-flow fault bookkeeping: seqNum to notify fetch rollback. */
+    InstSeqNum traceCtrlFaultSeqNum[MaxThreads]{};
+    bool traceCtrlFaultPending[MaxThreads]{};
+
     // Trace-mode helpers (implementation in src/cpu/o3/trace/CommitTrace.cc)

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 060150a and 0a8e3d6.

📒 Files selected for processing (4)

• src/cpu/o3/SConscript
• src/cpu/o3/commit.cc
• src/cpu/o3/commit.hh
• src/cpu/o3/trace/CommitTrace.cc

🧰 Additional context used

🧬 Code graph analysis (1)

src/cpu/o3/commit.cc (1)

src/cpu/o3/trace/CommitTrace.cc (16)

• traceMaybeExitOnPipelineDrainFromStuckCheck (48-59)
• traceMaybeExitOnPipelineDrainFromStuckCheck (49-49)
• traceMaybeExitOnEofDrainFromTick (61-71)
• traceMaybeExitOnEofDrainFromTick (62-62)
• traceLogHandleInterrupt (151-156)
• traceLogHandleInterrupt (152-152)
• traceOnCommit (126-141)
• traceOnCommit (127-127)
• traceOnMacroCommit (143-149)
• traceOnMacroCommit (144-144)
• traceLogInstFault (158-164)
• traceLogInstFault (159-159)
• traceLogPrivReturn (166-174)
• traceLogPrivReturn (167-167)
• traceLogCommitBlockedCycles (176-181)
• traceLogCommitBlockedCycles (177-177)

🪛 Cppcheck (2.19.0)

src/cpu/o3/trace/CommitTrace.cc

[error] 119-119: Reference to temporary returned.

(returnTempReference)

---

[error] 132-132: Reference to temporary returned.

(returnTempReference)

---

[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

---

[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

src/cpu/o3/commit.cc

[error] 119-119: Reference to temporary returned.

(returnTempReference)

---

[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

🔇 Additional comments (13)

src/cpu/o3/SConscript (1)

63-68: LGTM!

The trace reader sources are properly integrated into the build system under the ISA-enabled path, with clear organization and appropriate comments.

src/cpu/o3/trace/CommitTrace.cc (4)

26-44: LGTM!

The TraceCtrlFlowFault class is well-designed for injecting control-flow changes during trace simulation. Proper encapsulation in anonymous namespace keeps it internal to this translation unit.

---

151-156: Static analysis false positive - shift is valid.

The static analysis hint about "shifting 64-bit value by 64 bits" at this location is incorrect. Line 155 uses (1ULL << 63), which shifts by 63 bits - a valid operation for setting the MSB of a 64-bit value.

---

326-364: Memory diff checks handle edge cases correctly.

The optional memory address and size mismatch checks properly guard against empty vectors and invalid addresses before comparison. Good defensive programming.

---

101-102: getFault() returns a mutable reference, but this pattern should be replaced with a setter.

The non-const getFault() method (line 616 of dyn_inst.hh) returns Fault&, allowing direct assignment. However, the code itself contains a TODO comment acknowledging this: "This I added for the LSQRequest side to be able to modify the fault. There should be a better mechanism in place." Consider implementing a dedicated setFault() method instead of relying on the mutable reference pattern.

src/cpu/o3/commit.cc (5)

104-109: LGTM!

The trace mode exit check is properly integrated into the stuck check event handler. When trace mode is active and the pipeline is drained, it exits gracefully instead of panicking.

---

182-182: LGTM!

Per-thread traceCommitIndex initialization to 0 in the constructor loop is consistent with other per-thread state initialization.

---

880-882: LGTM!

The EOF drain check at the end of tick() provides a clean exit path when trace mode reaches EOF and the pipeline has drained.

---

1232-1232: LGTM!

The control-flow fault injection is correctly placed before commitHead(), allowing trace-driven redirects to be processed during the commit cycle.

---

1462-1463: Incorrect indentation: traceOnMacroCommit call has confusing placement.

Line 1462 (traceOnMacroCommit(tid);) is indented at the same level as statements inside the if (count > 1) block (line 1457), but it appears after the block's closing brace on line 1461. The closing brace on line 1463 correctly closes the outer if (onInstBoundary) block from line 1444. However, the indentation of line 1462 contradicts its logical position—it should either be dedented to match the block's closing brace or moved inside the if statement.

Likely an incorrect or invalid review comment.

src/cpu/o3/commit.hh (3)

590-606: LGTM!

The trace-mode helper declarations are well-organized and properly documented with a comment indicating where the implementations reside. This separation of interface (header) from implementation (CommitTrace.cc) is a good practice for code organization.

---

610-615: LGTM!

The traceCommitDifftest declaration and getTraceCommitIndex accessor provide a clean public interface for trace-mode operations. The const qualifier on getTraceCommitIndex is appropriate for a read-only accessor.

---

511-513: LGTM!

Using brace initialization for committedStreamId{1} and committedTargetId{0} is consistent with modern C++ style and provides clear default values.

[github-actions]

🚀 Coremark Smoke Test Results

Branch	IPC	Change
Base (xs-dev)	2.1689	-
This PR	2.1702	📈 +0.0013 (+0.06%)

✅ Difftest smoke test passed!

[github-actions]

🚀 Coremark Smoke Test Results

Branch	IPC	Change
Base (xs-dev)	2.1691	-
This PR	2.1739	📈 +0.0047 (+0.22%)

✅ Difftest smoke test passed!

[coderabbitai]

Actionable comments posted: 13

🤖 Fix all issues with AI agents

In @configs/common/xiangshan.py:
- Around line 388-391: Validate args.trace_file before assigning: only enable
trace mode and set cpu.traceFile when args.trace_file is not None, and ensure
cpu.traceFormat is set only when a trace file is configured; update the block
that manipulates cpu.enableTraceMode, cpu.traceFile, and cpu.traceFormat
(references: cpu.enableTraceMode, cpu.traceFile, cpu.traceFormat,
args.trace_file, args.trace_format) to guard against None so the C++ trace
reader never receives a None path.

In @src/cpu/o3/BaseO3CPU.py:
- Around line 255-262: The linter complaint comes from using Param via a star
import; replace the wildcard import with an explicit import of Param (i.e.,
import Param from m5.params) or alternatively add a targeted noqa comment on the
existing "from m5.params import *" to silence F405; update the import so
references to Param in BaseO3CPU (e.g., enableTraceMode, traceFile, traceFormat,
enableDecoupledBPInTrace, traceCheckpointInterval, traceBPValidation) are
resolved without triggering Ruff F405.

In @src/cpu/o3/trace/TraceFetch.cc:
- Around line 889-913: In TraceFetch::maybeCreateTraceCheckpoint replace the
hardcoded CHECKPOINT_INTERVAL modulus check with the instance member
traceCheckpointInterval: first return early if traceCheckpointInterval == 0 to
disable checkpointing, then use (seqNum % traceCheckpointInterval == 0) to
decide when to create a checkpoint; keep the existing createCheckpoint(),
push_back and removal logic (traceCheckpoints, checkpointSeqNums, DPRINTF)
unchanged but reference traceCheckpointInterval instead of CHECKPOINT_INTERVAL
to avoid divide/mod-by-zero and respect configured cadence.
- Around line 963-1010: In TraceFetch::rollbackTraceReader, the 1-based trace
index stored in index must be converted to the 0-based value expected by
TraceReader::softSeekToInstruction; before calling
traceReader->softSeekToInstruction(index) pass index-1 (guarded to avoid
underflow) so the subsequent getNextInstruction() returns the intended
instruction; update the call site (traceReader->softSeekToInstruction) to use
(index - 1) and ensure the existing checks for index > 0 cover this decrement.

In @src/cpu/o3/trace/TraceFetch.hh:
- Around line 71-224: The header currently hardcodes static constexpr uint64_t
CHECKPOINT_INTERVAL which ignores BaseO3CPUParams::traceCheckpointInterval; add
a non-static member uint64_t traceCheckpointInterval (defaulted or uninitialized
in header) and in TraceFetch::TraceFetch(const BaseO3CPUParams &params) set
traceCheckpointInterval = params.traceCheckpointInterval; then update
maybeCreateTraceCheckpoint() (and any other sites using CHECKPOINT_INTERVAL) to
use this->traceCheckpointInterval instead of CHECKPOINT_INTERVAL so the
Python-exposed knob takes effect (keep traceCheckpoints and checkpointSeqNums
logic the same).

In @src/cpu/o3/trace/TraceReader.cc:
- Around line 72-116: TraceReader::TraceStream::open currently shells out via
popen using escapePath which is unsafe and non-portable; replace the
popen/command construction for Mode::Gzip and the xz branch with one of two safe
approaches: (a) use a library-based decompressor (e.g., zlib/gzFile for gzip and
liblzma for xz) to open and stream decompressed bytes directly instead of
building a shell command, or (b) if an external process is required, create a
pipe and use fork + exec with an argv array (no shell/quoting) to run gzip/xz
and capture stdout; remove reliance on escapePath for security, update members
(pipeHandle/rawStream) to hold the new file/stream handles, and ensure
TraceReader::TraceStream::reopen and ::close correctly manage and pclose/close
or gzclose/lzma cleanup and set eofFlag accordingly, including returning failure
on any open/exec/decompression init errors.
- Around line 603-623: The addToBuffer() implementation in TraceReader currently
drops instructions when instrBuffer.size() >= MAX_BUFFER_SIZE which corrupts the
trace stream; change this behavior in TraceReader::addToBuffer by replacing the
silent drop with a hard failure (panic/fatal) that includes PC and seqNum (use
existing DPRINTF info) or implement automatic growth of instrBuffer before
pushing; specifically, remove the early return on MAX_BUFFER_SIZE, and either
call panic/fatal with a descriptive message including instr.getPC() and
instr.getSeqNum() (and keep dumpInstrBuffer context tags) or resize/expand
instrBuffer to accept the new entry, then push and log as before.
- Around line 322-379: Compressed checkpoint restore fails because the
fastForward lambda invoked by restoreCheckpointCommon (when called from
ChampSimTraceReader::restoreCheckpoint with allowCompressedRewind=true) does not
rewind the underlying TraceStream before calling parseInstruction(), so
compressed streams cannot be backtracked; fix by invoking reopenTraceStream()
(or seekBegin() when available) on the TraceReader/TraceStream at the start of
the fastForward lambda (before any parseInstruction() or stream.tell() usage) so
the stream is reset to the beginning for compressed modes, then proceed with
fast-forward parsing and validating instructionIndex as before.

🧹 Nitpick comments (7)

src/cpu/o3/trace/TraceFetch.hh (1)

190-224: Potential dead fields / confusing state in the public header.
Fields like traceWrongPathCyclesLeft are declared but (from this PR’s .cc) not used, which makes it harder to reason about the actual wrong-path model. If they’re planned, add a short comment “reserved” or remove until implemented.

src/cpu/o3/fetch.cc (1)

170-202: Trace-mode FTQ/decoupled frontend invariants appear properly maintained, but critical execution paths merit integration testing.

The intentional bypass of fetchTargetAvailable() checks in trace mode (lines 1991, 1997, 2004) is compensated by explicit FTQ supply attempts via trySupplyFetchWithTarget() (line 1547) and careful usedUpFetchTargets flag management. Strong assertions validate this invariant at line 2513–2515 when decoupled frontend is active. Startup sequencing (initTraceMode check at line 429) and resetStage initialization (usedUpFetchTargets = isDecoupledFrontend() at line 483 with traceFetch->resetStage()) are correctly ordered.

That said, test coverage should exercise edge cases: start-of-run before first FTQ entry is primed, post-squash recovery with exhausted targets, and EOF scenarios where supply attempts fail. The instrumentation is good (DPRINTF at lines 488–490, 1548–1557, 2538–2561), so traces will reveal any invariant violations.

Also applies to: 429-431, 482-491

configs/common/Options.py (2)

714-721: Fragile manipulation of internal argparse state.

Accessing parser._actions relies on argparse's internal implementation. While the comment on lines 710-713 appropriately documents the limitation, consider using parser._option_string_actions with the option string '--generic-rv-cpt' as a slightly more stable approach, or document this as a known fragility in case argparse internals change.

-    if '--enable-trace-mode' in sys.argv:
-        # In trace mode, make generic-rv-cpt optional by providing a dummy value.
-        # Find the generic-rv-cpt action and remove its required flag.
-        for action in parser._actions:
-            if action.dest == 'generic_rv_cpt':
-                action.required = False
-                action.default = "trace_mode_dummy"
-                break
+    if '--enable-trace-mode' in sys.argv:
+        # In trace mode, make generic-rv-cpt optional by providing a dummy value.
+        # Access via _option_string_actions for slightly more stable lookup.
+        action = parser._option_string_actions.get('--generic-rv-cpt')
+        if action:
+            action.required = False
+            action.default = "trace_mode_dummy"

---

689-690: Missing required flag for --trace-file in trace mode.

The --trace-file option is documented as required in TRACE_USAGE.md but lacks a required=True flag. Consider adding validation either here or in xiangshan.py to provide a clear error message when trace mode is enabled without a trace file.

configs/common/xiangshan.py (3)

251-261: Confusing functional_tlb logic in trace mode.

Line 259 sets args.functional_tlb = True unconditionally, but lines 326-327 later override cpu.mmu.functional based on timing_ptw. The comment "Force functional TLB to bypass complex MMU translation" on line 258 is misleading when --trace-timing-ptw is enabled, as the value gets overridden to False.

Consider restructuring to make the intent clearer:

-        # Force functional TLB to bypass complex MMU translation
-        args.functional_tlb = True
+        # Set functional TLB based on timing-PTW setting:
+        # - timing-PTW enabled: use timing translation (functional=False)
+        # - timing-PTW disabled: use functional TLB (functional=True)
+        args.functional_tlb = not bool(getattr(args, 'trace_timing_ptw', False))

This would also allow removing the override logic at lines 326-327.

---

386-386: Remove unnecessary f-string prefix.

The f-string has no placeholders. As flagged by static analysis (F541):

-        print(f"Configuring CPUs for trace mode...")
+        print("Configuring CPUs for trace mode...")

---

464-494: Consider extracting duplicate trace alignment logic.

This block (Ruby path) is nearly identical to lines 527-556 (classic cache path). Extract into a helper function to reduce duplication:

def _align_trace_to_mem_ranges(args, cpus, mem_ranges, path_name=""):
    """Align trace address mapping window to physical memory ranges."""
    try:
        base = int(mem_ranges[0].start)
        total = sum(int(r.size()) for r in mem_ranges)
        for cpu in cpus:
            cpu.traceAddrBase = base
            cpu.traceAddrSize = total
            cpu.traceAddrMapMode = "linear"
        _apply_trace_timing_ptw_cpu_params(args, cpus)
        # ... print logic
    except Exception as e:
        print(f"Warning: failed to align trace mapping to mem ({path_name}): {e}")

Also, catching bare Exception (BLE001) is overly broad. Consider catching specific exceptions like AttributeError or TypeError that could realistically occur here.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7628ef and 06a611d.

📒 Files selected for processing (8)

• configs/common/Options.py
• configs/common/xiangshan.py
• src/cpu/o3/BaseO3CPU.py
• src/cpu/o3/fetch.cc
• src/cpu/o3/trace/TRACE_USAGE.md
• src/cpu/o3/trace/TraceFetch.cc
• src/cpu/o3/trace/TraceFetch.hh
• src/cpu/o3/trace/TraceReader.cc

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-16T08:28:46.184Z

Learnt from: Lingrui98
Repo: OpenXiangShan/GEM5 PR: 649
File: configs/common/xiangshan.py:0-0
Timestamp: 2025-12-16T08:28:46.184Z
Learning: In XiangShan trace mode (configs/common/xiangshan.py), there is no bootloader. Update the config by setting test_sys.workload.bootloader = '' (empty string) to indicate no bootloader, instead of providing a bootloader path. This ensures the test setup correctly reflects the absence of a bootloader for XiangShan trace mode.

Applied to files:

• configs/common/xiangshan.py

🧬 Code graph analysis (3)

src/cpu/o3/fetch.cc (4)

src/cpu/o3/fetch.hh (13)

• Fetch (239-239)
• Fetch (240-240)
• isDecoupledFrontend (536-536)
• seqNum (501-501)
• seqNum (502-502)
• seqNum (503-503)
• seqNum (504-504)
• seqNum (505-505)
• index (506-506)
• index (856-860)
• index (856-856)
• index (872-876)
• index (872-872)

src/cpu/o3/trace/TraceReader.cc (1)

• TraceReader (526-533)

src/cpu/o3/trace/TraceReader.hh (3)

• TraceReader (88-89)
• TraceReader (90-90)
• TraceReader (90-90)

src/cpu/o3/trace/TraceFetch.hh (7)

• seqNum (112-113)
• seqNum (114-114)
• seqNum (115-115)
• seqNum (117-117)
• seqNum (118-118)
• seqNum (119-119)
• index (121-121)

src/cpu/o3/trace/TraceFetch.hh (3)

src/cpu/o3/fetch.cc (2)

• fetch (2052-2074)
• fetch (2053-2053)

src/cpu/o3/cpu.hh (19)

• fetch (374-374)
• fetch (377-380)
• fetch (628-632)
• tid (223-223)
• tid (226-226)
• tid (229-229)
• tid (232-232)
• tid (241-241)
• tid (244-244)
• tid (249-249)
• tid (261-261)
• tid (264-264)
• tid (279-279)
• tid (299-299)
• tid (371-371)
• tid (393-393)
• tid (401-401)
• tid (410-410)
• tid (572-572)

src/cpu/o3/dyn_inst.hh (2)

• next_pc (702-710)
• predPC (630-630)

src/cpu/o3/trace/TraceReader.cc (4)

src/cpu/o3/trace/TraceReader.hh (34)

• TraceStream (116-116)
• TraceStream (117-117)
• pos (125-125)
• path (119-119)
• path (133-133)
• path (214-215)
• dst (123-123)
• trace_addr (172-173)
• trace_addr (174-175)
• trace_addr (176-177)
• trace_addr (180-181)
• cfg (104-104)
• cfg (104-104)
• trace_pc (178-179)
• pc (183-183)
• max_instructions (205-207)
• max_instructions (222-222)
• instructionIndex (188-196)
• stream (210-211)
• stream (212-213)
• checkpoint (106-106)
• checkpoint (198-204)
• checkpoint (216-220)
• startIndex (209-209)
• traceFile (101-101)
• TraceReader (88-89)
• TraceReader (90-90)
• TraceReader (90-90)
• TraceReaderStats (169-169)
• instr (223-223)
• instr (225-225)
• instr (226-226)
• instrIndex (94-94)
• instrIndex (107-107)

src/cpu/o3/trace/CBP2025TraceReader.hh (6)

• dst (115-115)
• max_instructions (75-75)
• instructionIndex (72-72)
• checkpoint (70-70)
• instr (76-76)
• instrIndex (71-71)

src/cpu/o3/trace/ChampSimTraceReader.hh (4)

• max_instructions (167-167)
• checkpoint (137-137)
• instr (174-174)
• instrIndex (144-144)

src/cpu/o3/trace/ChampSimTraceReader.cc (6)

• parseInstruction (169-184)
• parseInstruction (170-170)
• fillBuffer (147-167)
• fillBuffer (148-148)
• seekToInstruction (517-594)
• seekToInstruction (518-518)

🪛 Cppcheck (2.19.0)

src/cpu/o3/trace/TraceReader.cc

[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

---

[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

src/cpu/o3/trace/TraceFetch.cc

[error] 119-119: Reference to temporary returned.

(returnTempReference)

---

[error] 132-132: Reference to temporary returned.

(returnTempReference)

---

[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

---

[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

🪛 LanguageTool

src/cpu/o3/trace/TRACE_USAGE.md

[uncategorized] ~63-~63: 能愿动词不能成为‘把’字句、‘被’字句的谓语动词。应该是:"会把……页"。
Context: ...可能是 modulo/线性映射，为避免 trace 地址落入页表页导致自毁，需要把页表放在“永远不会被 trace 映射命中”的物理区： - 开启 `--trace-timing...

(wa3)

🪛 Ruff (0.14.10)

configs/common/xiangshan.py

249-249: makeBareMetalXiangshanSystem may be undefined, or defined from star imports

(F405)

---

249-249: SysConfig may be undefined, or defined from star imports

(F405)

---

320-320: PMAChecker may be undefined, or defined from star imports

(F405)

---

321-321: AddrRange may be undefined, or defined from star imports

(F405)

---

386-386: f-string without any placeholders

Remove extraneous f prefix

(F541)

---

479-479: Do not catch blind exception: Exception

(BLE001)

---

541-541: Do not catch blind exception: Exception

(BLE001)

src/cpu/o3/BaseO3CPU.py

256-256: Param may be undefined, or defined from star imports

(F405)

---

257-257: Param may be undefined, or defined from star imports

(F405)

---

258-258: Param may be undefined, or defined from star imports

(F405)

---

259-259: Param may be undefined, or defined from star imports

(F405)

---

260-260: Param may be undefined, or defined from star imports

(F405)

---

261-261: Param may be undefined, or defined from star imports

(F405)

---

264-264: Param may be undefined, or defined from star imports

(F405)

---

265-265: Param may be undefined, or defined from star imports

(F405)

---

266-266: Param may be undefined, or defined from star imports

(F405)

---

267-267: Param may be undefined, or defined from star imports

(F405)

---

270-270: Param may be undefined, or defined from star imports

(F405)

---

272-272: Param may be undefined, or defined from star imports

(F405)

---

274-274: Param may be undefined, or defined from star imports

(F405)

---

278-278: Param may be undefined, or defined from star imports

(F405)

---

283-283: Param may be undefined, or defined from star imports

(F405)

---

287-287: Param may be undefined, or defined from star imports

(F405)

---

293-293: Param may be undefined, or defined from star imports

(F405)

---

297-297: Param may be undefined, or defined from star imports

(F405)

⏰ 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: Quick Build, Unit Tests & Smoke Test

🔇 Additional comments (6)

src/cpu/o3/fetch.cc (2)

2155-2157: Check whether passing trace=true to buildInst() here is intended for non-trace runs.
If this was changed from conditional → unconditional, it may increase TRACING_ON overhead and/or change trace record emission behavior. If it was already unconditional before, ignore.

---

1811-1865: This concern is invalid—the code has an implicit invariant that prevents null dereference.

In Decode::squash() (the only source of squash signals to fetch), squash, branchMispredict, and mispredictInst are always set together atomically (decode.cc lines 318–321). Therefore, when handleDecodeSquash() checks if (fromDecode->decodeInfo[tid].squash), it's guaranteed that mispredictInst is non-null. The later check for branchMispredict (fetch.cc line 1833) is defensive but redundant given this invariant.

Likely an incorrect or invalid review comment.

src/cpu/o3/trace/TRACE_USAGE.md (1)

1-110: Comprehensive documentation for trace-driven simulation.

The documentation is well-structured and covers all essential aspects: quick start, CLI options with clear explanations, reader behavior, trace formats, and debugging guidance. The technical content aligns with the implementation in the related files.

configs/common/xiangshan.py (3)

64-81: LGTM!

The _trace_timing_ptw_settings helper cleanly extracts and validates PTW settings with appropriate error handling via fatal() for invalid inputs.

---

84-105: LGTM!

The function correctly propagates PTW settings to CPUs and includes important validation to ensure traceAddrSize exceeds reserved bytes before shrinking the window.

---

699-715: LGTM!

The trace options integration and difftest handling are correct. Trace mode appropriately disables difftest since reference model verification isn't applicable. Based on learnings, the bootloader configuration (line 278: test_sys.workload.bootloader = '') correctly indicates no bootloader for trace mode.

[Lingrui98]

Addressed the remaining unresolved review threads on trace-new (built: scons build/RISCV/gem5.opt).

• discussion_r2646805173: fetch.cc now includes debug/TraceReader.hh and uses DPRINTF(TraceReader, ...) for the trace diagnostics (no Override flag).
• discussion_r2646805174: add panic_if(cfg.size < PageSz, ...) in TraceReader::mapAddressLinear() when pageAlign is enabled, to avoid silently mapping everything to base + page_offset.
• discussion_r2674712808: de-duplicate trace-mode difftest toggling by making build_xiangshan_system() follow args.enable_difftest (normalized in xiangshan_system_init()).

Commit: 06a611d

[Lingrui98]

Pushed fixes for the latest unresolved threads (commit 664723e; built: scons build/RISCV/gem5.opt).

• configs/common/xiangshan.py: fatal if --enable-trace-mode without --trace-file.
• src/cpu/o3/trace/TraceFetch.hh/.cc: honor traceCheckpointInterval (0 disables), fix rollback off-by-one (softSeekToInstruction cursor=index-1), and add PTW page-table region bounds checks.
• src/cpu/o3/trace/TraceFetch.cc: traceTrainBranches now respects the knob (branches can be rendered as NOPs when disabled); traceWrongPathUseTraceInst is gated with fatal (still unimplemented).
• src/cpu/o3/trace/TraceReader.cc/.hh: replace popen("gzip|xz") pipeline with fork/exec + pipe; restoreCheckpointCommon rewinds stream before compressed fast-forward; TraceReader buffer overflow is fail-fast.

Note: I’m intentionally not addressing the Ruff-only import nit per our decision.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

configs/common/xiangshan.py (3)

388-388: Remove unnecessary f-string prefix.

The string has no placeholders, so the f prefix is unnecessary.

-        print(f"Configuring CPUs for trace mode...")
+        print("Configuring CPUs for trace mode...")

---

466-496: Consider extracting duplicated trace alignment logic.

The trace address mapping alignment code is nearly identical between the Ruby path (lines 466-496) and the classic cache path (lines 529-558). This duplication increases maintenance burden.

♻️ Suggested refactor

Extract a helper function:

def _align_trace_mapping_to_mem(args, test_sys, path_name):
    """Align trace address mapping window to physical memory size."""
    aligned_base = None
    aligned_total = None
    try:
        base = int(test_sys.mem_ranges[0].start)
        total = sum(int(r.size()) for r in test_sys.mem_ranges)
        for cpu in test_sys.cpu:
            cpu.traceAddrBase = base
            cpu.traceAddrSize = total
            cpu.traceAddrMapMode = "linear"
        aligned_base = base
        aligned_total = total
    except Exception as e:
        print(f"Warning: failed to align trace mapping to mem ({path_name}): {e}")
    
    _apply_trace_timing_ptw_cpu_params(args, test_sys.cpu)
    
    if aligned_base is not None:
        final_size = int(test_sys.cpu[0].traceAddrSize)
        reserved_bytes = int(getattr(args, 'trace_ptw_reserved_bytes', 0))
        if bool(getattr(args, 'trace_timing_ptw', False)):
            print(f"Trace mode: Align trace mapping to mem: base=0x{aligned_base:x}, "
                  f"size=0x{final_size:x} (reserved=0x{reserved_bytes:x})")
        else:
            print(f"Trace mode: Align trace mapping to mem: base=0x{aligned_base:x}, "
                  f"size=0x{aligned_total:x}")

Then call _align_trace_mapping_to_mem(args, test_sys, "Ruby path") and _align_trace_mapping_to_mem(args, test_sys, "classic cache path") respectively.

Also applies to: 529-558

---

481-482: Broad exception handling is acceptable here but could be more specific.

Catching bare Exception is flagged by static analysis (BLE001). While this is acceptable for configuration robustness to avoid crashing on edge cases, consider catching more specific exceptions if the expected failure modes are known (e.g., AttributeError, IndexError).

Also applies to: 543-544

src/cpu/o3/trace/TraceReader.cc (1)

707-728: Inconsistent address mapping configuration for ChampSimTraceReader.

The createTraceReader factory sets address mapping twice for ChampSim format: once in the constructor (lines 709-712) and again via setAddressMapping (line 715). This is redundant for ChampSim but not done for CBP2025.

♻️ Consider removing redundant setAddressMapping call

     if (format == "champsim") {
         auto reader = std::make_unique<ChampSimTraceReader>(trace_file, name,
                                                             addrMapMode,
                                                             addrBase,
                                                             addrSize,
                                                             pageAlign,
                                                             parent);
-        // Configure address mapping parameters
-        reader->setAddressMapping(addrBase, addrSize, addrMapMode, pageAlign);
         return reader;
     } else if (format == "cbp2025") {

If the constructor already configures address mapping, the subsequent call is unnecessary.

src/cpu/o3/trace/TraceFetch.cc (1)

143-144: Minor: Chinese comment could use English translation.

Line 143 contains a Chinese comment. For broader maintainability, consider adding an English translation:

-    // 同步清理 reader 内部缓冲/历史窗口，保持状态一致
+    // Synchronize cleanup of reader's internal buffer/history window to maintain consistent state
     traceReader->resetHistory();

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 06a611d and 664723e.

📒 Files selected for processing (6)

• configs/common/xiangshan.py
• src/cpu/o3/BaseO3CPU.py
• src/cpu/o3/trace/TraceFetch.cc
• src/cpu/o3/trace/TraceFetch.hh
• src/cpu/o3/trace/TraceReader.cc
• src/cpu/o3/trace/TraceReader.hh

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-16T08:28:46.184Z

Learnt from: Lingrui98
Repo: OpenXiangShan/GEM5 PR: 649
File: configs/common/xiangshan.py:0-0
Timestamp: 2025-12-16T08:28:46.184Z
Learning: In XiangShan trace mode (configs/common/xiangshan.py), there is no bootloader. Update the config by setting test_sys.workload.bootloader = '' (empty string) to indicate no bootloader, instead of providing a bootloader path. This ensures the test setup correctly reflects the absence of a bootloader for XiangShan trace mode.

Applied to files:

• configs/common/xiangshan.py

🧬 Code graph analysis (4)

src/cpu/o3/trace/TraceReader.cc (3)

src/cpu/o3/trace/TraceReader.hh (36)

• TraceStream (118-118)
• TraceStream (119-119)
• pos (127-127)
• path (121-121)
• path (135-135)
• path (217-218)
• modeFlag (132-132)
• eofFlag (130-130)
• dst (125-125)
• trace_addr (175-176)
• trace_addr (177-178)
• trace_addr (179-180)
• trace_addr (183-184)
• cfg (106-106)
• cfg (106-106)
• pc (186-186)
• pending (187-189)
• max_instructions (208-210)
• max_instructions (225-225)
• eofReached (98-98)
• instructionIndex (191-199)
• currentSeqNum (230-230)
• stream (213-214)
• stream (215-216)
• checkpoint (108-108)
• checkpoint (201-207)
• checkpoint (219-223)
• instrBuffer (99-99)
• TraceReader (90-91)
• TraceReader (92-92)
• TraceReader (92-92)
• instr (226-226)
• instr (228-228)
• instr (229-229)
• instrIndex (96-96)
• instrIndex (109-109)

src/cpu/o3/trace/CBP2025TraceReader.cc (6)

• parseInstruction (259-306)
• parseInstruction (260-260)
• fillBuffer (348-359)
• fillBuffer (349-349)
• seekToInstruction (396-401)
• seekToInstruction (397-397)

src/cpu/o3/trace/ChampSimTraceReader.cc (6)

• parseInstruction (169-184)
• parseInstruction (170-170)
• fillBuffer (147-167)
• fillBuffer (148-148)
• seekToInstruction (517-594)
• seekToInstruction (518-518)

configs/common/xiangshan.py (2)

configs/common/FSConfig.py (1)

• makeBareMetalXiangshanSystem (660-704)

configs/common/Options.py (1)

• addXiangshanTraceOptions (677-721)

src/cpu/o3/trace/TraceFetch.cc (3)

src/cpu/o3/trace/TraceFetch.hh (3)

• TraceFetch (74-74)
• TraceFetch (75-75)
• tc (164-164)

src/cpu/o3/trace/TraceReader.cc (2)

• createTraceReader (698-729)
• createTraceReader (699-702)

src/cpu/o3/trace/TraceReader.hh (1)

• createTraceReader (240-247)

src/cpu/o3/BaseO3CPU.py (2)

src/cpu/o3/trace/TraceReader.hh (1)

• traceFile (103-103)

src/cpu/o3/trace/TraceFetch.hh (2)

• traceBPValidation (124-124)
• traceEnableWrongPath (125-125)

🪛 Cppcheck (2.19.0)

src/cpu/o3/trace/TraceReader.cc

[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

---

[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

src/cpu/o3/trace/TraceFetch.cc

[error] 119-119: Reference to temporary returned.

(returnTempReference)

---

[error] 132-132: Reference to temporary returned.

(returnTempReference)

---

[error] 125-125: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

---

[error] 138-138: Shifting 64-bit value by 64 bits is undefined behaviour

(shiftTooManyBits)

🪛 Ruff (0.14.10)

configs/common/xiangshan.py

249-249: makeBareMetalXiangshanSystem may be undefined, or defined from star imports

(F405)

---

249-249: SysConfig may be undefined, or defined from star imports

(F405)

---

320-320: PMAChecker may be undefined, or defined from star imports

(F405)

---

321-321: AddrRange may be undefined, or defined from star imports

(F405)

---

388-388: f-string without any placeholders

Remove extraneous f prefix

(F541)

---

481-481: Do not catch blind exception: Exception

(BLE001)

---

543-543: Do not catch blind exception: Exception

(BLE001)

src/cpu/o3/BaseO3CPU.py

256-256: Param may be undefined, or defined from star imports

(F405)

---

257-257: Param may be undefined, or defined from star imports

(F405)

---

258-258: Param may be undefined, or defined from star imports

(F405)

---

259-259: Param may be undefined, or defined from star imports

(F405)

---

260-260: Param may be undefined, or defined from star imports

(F405)

---

261-261: Param may be undefined, or defined from star imports

(F405)

---

264-264: Param may be undefined, or defined from star imports

(F405)

---

265-265: Param may be undefined, or defined from star imports

(F405)

---

266-266: Param may be undefined, or defined from star imports

(F405)

---

267-267: Param may be undefined, or defined from star imports

(F405)

---

270-270: Param may be undefined, or defined from star imports

(F405)

---

272-272: Param may be undefined, or defined from star imports

(F405)

---

274-274: Param may be undefined, or defined from star imports

(F405)

---

278-278: Param may be undefined, or defined from star imports

(F405)

---

283-283: Param may be undefined, or defined from star imports

(F405)

---

287-287: Param may be undefined, or defined from star imports

(F405)

---

293-293: Param may be undefined, or defined from star imports

(F405)

---

297-297: Param may be undefined, or defined from star imports

(F405)

🔇 Additional comments (12)

configs/common/xiangshan.py (1)

64-105: LGTM: Well-structured PTW settings helpers.

The _trace_timing_ptw_settings and _apply_trace_timing_ptw_cpu_params functions provide clear validation and configuration for trace timing PTW. The validation logic (reserved bytes > 0, supported page sizes, traceAddrSize > reserved bytes) is thorough.

src/cpu/o3/BaseO3CPU.py (1)

254-298: LGTM: Comprehensive trace mode parameter surface.

The new parameters provide a well-organized configuration surface for trace-driven simulation:

• Core trace settings (enableTraceMode, traceFile, traceFormat)
• Address mapping (traceAddrMapMode, traceAddrBase, traceAddrSize, traceAddrPageAlign)
• Timing PTW modeling (traceTimingPTW, tracePTReservedBytes, tracePTLeafPageSize)
• BP training and control-flow modeling (traceTrainBranches, traceMispredictPenalty, traceEnableWrongPath)

The documentation strings are clear, and unimplemented/experimental features are appropriately marked.

Note: The static analysis warnings about Param being undefined (F405) are false positives - Param is imported via from m5.params import * on line 40, which is standard practice in GEM5 Python configs.

src/cpu/o3/trace/TraceFetch.hh (1)

71-225: LGTM: Well-designed TraceFetch interface.

The TraceFetch class provides a clean encapsulation of trace-driven fetch behavior with:

• Clear public API for lifecycle management (initTraceMode, resetStage, handleTraceSquash)
• Trace metadata binding and lookup APIs
• Wrong-path handling state and methods
• Per-thread trace streams with reasonable prefetch (TRACE_STREAM_MIN_FILL = 16)

The "friend-style" helper design (lines 67-70) is appropriate for tight integration with Fetch while keeping trace-specific code separate.

src/cpu/o3/trace/TraceReader.cc (3)

55-142: LGTM: Robust TraceStream implementation with fork/exec.

The TraceStream implementation using fork/exec for decompression (replacing popen as noted in PR commits) is a security improvement. Key observations:

• Child process properly redirects stdout/stderr and calls _exit(127) on failure
• Parent properly handles pipe lifecycle and child process cleanup
• The escapePath helper prevents shell injection (though not used with fork/exec)

Note: The static analysis warnings about "Shifting 64-bit value by 64 bits" at lines 125/138 appear to be false positives - these lines contain execvp and pipeHandle assignment respectively, not bit-shift operations.

---

241-263: Good validation in mapAddressLinear with pageAlign.

The panic_if check (line 248-252) correctly validates that cfg.size >= PageSz when pageAlign is enabled, as mentioned in the PR comments. This prevents silent mapping errors.

---

657-678: Good buffer overflow protection.

The addToBuffer function correctly panics on buffer overflow (line 663-669) rather than silently failing, which aligns with the PR's "fail-fast on TraceReader buffer overflow" commit message.

src/cpu/o3/trace/TraceReader.hh (1)

58-231: LGTM: Well-designed TraceReader abstract interface.

The TraceReader base class provides a comprehensive interface for trace format readers with:

• Proper virtual destructor (virtual ~TraceReader() = default; line 92)
• Clear separation of pure virtual methods for format-specific behavior
• Useful protected utilities for address mapping and checkpoint management
• Statistics integration via inheritance from statistics::Group
• Reasonable buffer/history capacities (MAX_BUFFER_SIZE=1024, HISTORY_CAPACITY=4096)

The TraceCheckpoint structure (lines 61-75) captures sufficient state for reliable rollback including buffer snapshot and pending instruction state.

src/cpu/o3/trace/TraceFetch.cc (5)

106-107: Good validation: fatal_if for unimplemented feature.

The fatal_if(traceWrongPathUseTraceInst, ...) correctly prevents use of an unimplemented feature, matching the parameter documentation in BaseO3CPU.py.

---

148-312: LGTM: Thorough PTW setup with comprehensive validation.

The setupTraceTimingPTW function builds a synthetic SV39 page table structure with:

• Proper validation of parameters (lines 151-167)
• Bounds checking for page table region (lines 180-198, 204-209)
• Correct SV39 PTE construction for non-leaf and leaf entries
• SATP register configuration (lines 296-303)

The validation ensures the page table region doesn't overlap the trace mapping window and fits within physical memory.

---

638-784: Complex but well-structured squash handling.

The handleTraceSquash method handles multiple squash scenarios:

• Wrong-path squashes (boundary vs. non-boundary)
• Non-instruction squashes (TLB/page fault, trap, replay)
• Normal squashes

The logic correctly distinguishes between squashing an instruction itself vs. squashing after it, and properly manages trace reader rollback.

One minor observation: The Chinese comments (lines 651, 709-710, etc.) might benefit from English translations for broader maintainability, though this is a style preference.

---

1261-1608: Comprehensive instruction synthesis from trace.

The createMachInstFromTrace method provides a thorough mapping from trace instruction types to RISC-V machine instructions, supporting both compressed (2-byte) and standard (4-byte) formats. Key observations:

• Proper immediate encoding for B-type, J-type, and compressed branch/jump formats
• Fallback to NOP for unsupported cases (safe behavior)
• Good clamping logic for out-of-range immediates with debug logging
• Handles edge cases like CALL_DIRECT not being compressible on RV64

The implementation correctly handles the synthesis requirement mentioned in the PR description: "Instructions from traces are synthesized to RISC-V instructions."

---

849-899: Sliding-window cleanup prevents unbounded memory growth.

The cleanupTraceMetadataOnCommit function implements a sliding-window cleanup strategy with a guard window (256 entries) behind the oldest in-flight instruction. This prevents unbounded metadata growth while keeping sufficient history for late squashes.

The use of std::numeric_limits<InstSeqNum>::max() as the default wrong-path boundary when not active is appropriate.

[github-actions]

🚀 Coremark Smoke Test Results

Branch	IPC	Change
Base (xs-dev)	2.1691	-
This PR	2.1739	📈 +0.0047 (+0.22%)

✅ Difftest smoke test passed!

[github-actions]

🚀 Performance test triggered: spec06-0.8c

[XiangShanRobot]

[Generated by GEM5 Performance Robot]
commit: 664723e
workflow: On-Demand SPEC Test (Tier 1.5)

Ideal BTB Performance

Overall Score

	PR	Master	Diff(%)
Score	19.90	20.15	-1.24 🔴