Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions configs/common/Options.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ def addNoISAOptions(parser, configure_xiangshan=False):
parser.add_argument("--l1-to-l2-pf-hint", action="store_true")
parser.add_argument("--l2-to-l3-pf-hint", action="store_true")

parser.add_argument("--memtrace_path", type=str, default=None) # Memtrace file path for restoring the microarchitectural state in L3 Cache

parser.add_argument("--num-dirs", type=int, default=1)
parser.add_argument("--num-l2caches", type=int, default=1)
parser.add_argument("--num-l3caches", type=int, default=1)
Expand Down
3 changes: 2 additions & 1 deletion configs/common/Simulation.py
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,8 @@ def run_vanilla(options, root, testsys, cpu_class):

checkpoint_dir = None
root.apply_config(options.param)
m5.instantiate(checkpoint_dir)
# add path for generic_rv_cpt and memtrace_path
m5.instantiate(pmem_file_path=options.generic_rv_cpt,mem_trace_file=options.memtrace_path,ckpt_dir=checkpoint_dir)
Comment on lines +824 to +825
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

rg -nP '\bm5\.instantiate\s*\(' -A 2

Repository: OpenXiangShan/GEM5

Length of output: 12263


🏁 Script executed:

sed -n '65,100p' src/python/m5/simulate.py

Repository: OpenXiangShan/GEM5

Length of output: 1254


🏁 Script executed:

sed -n '648,656p' configs/common/Simulation.py

Repository: OpenXiangShan/GEM5

Length of output: 496


Fix the m5.instantiate call at line 652 to match the new signature.

The new signature at line 825 requires two positional arguments (pmem_file_path and mem_trace_file), but line 652 in the same file calls m5.instantiate(checkpoint_dir) with only one argument. This will fail at runtime with a TypeError. Line 652 must be updated to pass both required arguments (or use keyword arguments like line 825).

🤖 Prompt for AI Agents
In `@configs/common/Simulation.py` around lines 824 - 825, Update the earlier call
to m5.instantiate so it matches the new signature used later: pass both
pmem_file_path and mem_trace_file (or use keyword args) in addition to
checkpoint_dir; locate the call to m5.instantiate(checkpoint_dir) and change it
to include options.generic_rv_cpt and options.memtrace_path (e.g.,
m5.instantiate(pmem_file_path=options.generic_rv_cpt,
mem_trace_file=options.memtrace_path, ckpt_dir=checkpoint_dir)) so the function
receives the required two positional/keyword arguments.


# Handle the max tick settings now that tick frequency was resolved
# during system instantiation
Expand Down
3 changes: 3 additions & 0 deletions configs/common/xiangshan.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,9 @@ def config_xiangshan_inputs(args: argparse.Namespace, sys):
sys.workload.raw_bootloader = True
else:
sys.gcpt_restorer_file = gcpt_restorer
if args.memtrace_path is not None:
sys.restore_from_memtrace = True
sys.memtrace_file = args.memtrace_path
Comment on lines +230 to +232
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Validate memtrace_path before wiring it into the system.

This currently accepts any string and defers failure to later stages. Add an early file-existence check for clearer errors and safer setup flow.

✅ Suggested patch
     if args.memtrace_path is not None:
+        if not os.path.isfile(args.memtrace_path):
+            fatal(f"Invalid --memtrace-path: file not found: {args.memtrace_path}")
         sys.restore_from_memtrace = True
         sys.memtrace_file = args.memtrace_path
📝 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.

Suggested change
if args.memtrace_path is not None:
sys.restore_from_memtrace = True
sys.memtrace_file = args.memtrace_path
if args.memtrace_path is not None:
if not os.path.isfile(args.memtrace_path):
fatal(f"Invalid --memtrace-path: file not found: {args.memtrace_path}")
sys.restore_from_memtrace = True
sys.memtrace_file = args.memtrace_path
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@configs/common/xiangshan.py` around lines 230 - 232, Validate
args.memtrace_path before assigning to sys: check that the path exists and is a
regular file (e.g., via os.path.exists or pathlib.Path.is_file) and only set
sys.restore_from_memtrace = True and sys.memtrace_file = args.memtrace_path when
the check passes; if the check fails, emit a clear error (raise/exit or
processLogger.error) mentioning the invalid args.memtrace_path so the setup
fails fast and with an actionable message.

# enable h checkpoint
if args.enable_h_gcpt:
sys.enable_h_gcpt = True
Expand Down
10 changes: 10 additions & 0 deletions src/mem/cache/cache_blk.hh
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,16 @@ class CacheBlk : public TaggedEntry
}
}

// Public versions of some protected setters for use in restore L3 microarchitectural state
/** Set the task id value. */
void setTaskId_pub(const uint32_t task_id) { _taskId = task_id; }

/** Set the source requestor id. */
void setSrcRequestorId_pub(const uint32_t id) { _srcRequestorId = id; }

/** Set the current tick as this block's insertion tick. */
void setTickInserted_pub() { _tickInserted = curTick(); }

/** Set the XS metadata. */
void setXsMetadata(const Request::XsMetadata &xs_meta)
{
Expand Down
2 changes: 2 additions & 0 deletions src/mem/cache/replacement_policies/base.hh
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ class Base : public SimObject
virtual void reset(const std::shared_ptr<ReplacementData>&
replacement_data) const = 0;

virtual void reset4memtrace(const std::shared_ptr<ReplacementData>&
replacement_data,int priority) const = 0;
/**
* Find replacement victim among candidates.
*
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/bip_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ class BIP : public LRU
*/
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;
void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
};

} // namespace replacement_policy
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/brrip_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ class BRRIP : public Base
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
Comment on lines +146 to +148
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Implement reset4memtrace to initialize BRRIP state.

Leaving the memtrace reset hook empty means BRRIP replacement metadata won’t be initialized during memtrace warmup, which undermines the feature and can skew victim selection. At minimum, fall back to the normal reset() behavior, or map priority to an RRPV value.

🛠️ Suggested fallback
-    void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
-    {     
-    }   
+    void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
+    {
+        reset(replacement_data);
+        (void)priority;
+    }
📝 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.

Suggested change
void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
reset(replacement_data);
(void)priority;
}
🤖 Prompt for AI Agents
In `@src/mem/cache/replacement_policies/brrip_rp.hh` around lines 145 - 147, The
reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data, int
priority) const override is currently empty; implement it to initialize BRRIP
metadata during memtrace warmup by either delegating to the existing reset()
method (e.g., call reset(replacement_data)) or by mapping the provided priority
to an RRPV value and setting each line's RRPV in the ReplacementData/BRRIP state
accordingly (ensure you update the same structures used by get/set RRPV methods
so victim selection behaves correctly).

/**
* Find replacement victim using rrpv.
*
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/dueling_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ class Dueling : public Base
const PacketPtr pkt) override;
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;
void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
ReplaceableEntry* getVictim(const ReplacementCandidates& candidates) const
override;
std::shared_ptr<ReplacementData> instantiateEntry() override;
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/fifo_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ class FIFO : public Base
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
/**
* Find replacement victim using insertion timestamps.
*
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/lfu_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ class LFU : public Base
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
/**
* Find replacement victim using reference frequency.
*
Expand Down
9 changes: 9 additions & 0 deletions src/mem/cache/replacement_policies/lru_rp.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,15 @@ LRU::reset(const std::shared_ptr<ReplacementData>& replacement_data) const
replacement_data)->lastTouchTick = curTick();
}

void
LRU::reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const
{
// Set last touch timestamp
// Here we use priority to set lastTouchTick, the lower the priority, the older the lastTouchTick
std::static_pointer_cast<LRUReplData>(
replacement_data)->lastTouchTick = curTick() - priority;
}
Comment on lines +73 to +80
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for Tick typedef and using declarations
echo "=== Tick type definitions ==="
rg -n "typedef\s+.*Tick|using\s+Tick" -S 2>/dev/null || echo "No matches found"

echo -e "\n=== reset4memtrace call sites ==="
rg -n "reset4memtrace\(" -S 2>/dev/null || echo "No matches found"

echo -e "\n=== File structure context ==="
fd -t f "lru_rp\.(cc|hh)" | head -5

Repository: OpenXiangShan/GEM5

Length of output: 4146


🏁 Script executed:

#!/bin/bash
# Look for LRUReplData definition to understand lastTouchTick type
rg -n "struct\s+LRUReplData|class\s+LRUReplData" -A 10 -S 2>/dev/null || echo "No struct/class found"

Repository: OpenXiangShan/GEM5

Length of output: 847


🏁 Script executed:

#!/bin/bash
# Check the header file for reset4memtrace declaration and comments
fd -t f "lru_rp\.hh" -exec cat {} \;

Repository: OpenXiangShan/GEM5

Length of output: 249


🏁 Script executed:

#!/bin/bash
# Read the LRU header file
cat src/mem/cache/replacement_policies/lru_rp.hh

Repository: OpenXiangShan/GEM5

Length of output: 4365


🏁 Script executed:

#!/bin/bash
# Check the call sites to understand what priority values are passed
echo "=== sector_tags.cc reset4memtrace call ==="
rg -n -B 5 -A 2 "reset4memtrace\(blk->replacementData,priority\)" src/mem/cache/tags/sector_tags.cc

echo -e "\n=== base_set_assoc.hh reset4memtrace call ==="
rg -n -B 5 -A 2 "reset4memtrace\(blk->replacementData,priority\)" src/mem/cache/tags/base_set_assoc.hh

Repository: OpenXiangShan/GEM5

Length of output: 512


🏁 Script executed:

#!/bin/bash
# Check how priority is calculated and what values it can have
rg -n "priority\s*=" src/mem/cache/tags/sector_tags.cc | head -20
rg -n "priority\s*=" src/mem/cache/tags/base_set_assoc.hh | head -20

Repository: OpenXiangShan/GEM5

Length of output: 44


🏁 Script executed:

#!/bin/bash
# Find where updateRp is called with priority parameter
rg -n "updateRp\(" -B 3 -A 1 | head -60

Repository: OpenXiangShan/GEM5

Length of output: 2056


🏁 Script executed:

#!/bin/bash
# Check for any documentation or comments about priority in memtrace context
rg -n "memtrace|priority" src/mem/cache/tags/sector_tags.cc -B 2 -A 2 | head -80

Repository: OpenXiangShan/GEM5

Length of output: 217


🏁 Script executed:

#!/bin/bash
# Look for the definition/implementation of updateRp callers
rg -n "\.updateRp\(" src/mem/cache -r --type cc --type hh | head -30

Repository: OpenXiangShan/GEM5

Length of output: 88


🏁 Script executed:

#!/bin/bash
# Find where memtrace_priority is defined and used
rg -n "memtrace_priority" src/mem/cache -B 3 -A 3

Repository: OpenXiangShan/GEM5

Length of output: 1181


🏁 Script executed:

#!/bin/bash
# Check the base.cc file around line 397 for more context
sed -n '380,415p' src/mem/cache/tags/base.cc

Repository: OpenXiangShan/GEM5

Length of output: 1547


🏁 Script executed:

#!/bin/bash
# Search for Tick underflow or saturation patterns in the codebase
rg -n "saturate|underflow|std::min|std::max.*Tick" src/ -t cc -t hh | head -40

Repository: OpenXiangShan/GEM5

Length of output: 88


🏁 Script executed:

#!/bin/bash
# Get more context around where memtrace_priority is used
sed -n '360,410p' src/mem/cache/tags/base.cc

Repository: OpenXiangShan/GEM5

Length of output: 2166


🏁 Script executed:

#!/bin/bash
# Check if there are any bounds checks on priority/rank before use
rg -n "priority|rank" src/mem/cache/tags/base.cc -B 2 -A 2 | head -100

Repository: OpenXiangShan/GEM5

Length of output: 870


🏁 Script executed:

#!/bin/bash
# Check how other implementations of reset4memtrace handle the priority parameter
rg -n "reset4memtrace" -A 5 src/mem/cache/replacement_policies/ | head -80

Repository: OpenXiangShan/GEM5

Length of output: 6363


🏁 Script executed:

#!/bin/bash
# Check if there's any validation on the rank value before use
sed -n '360,380p' src/mem/cache/tags/base.cc

Repository: OpenXiangShan/GEM5

Length of output: 831


🏁 Script executed:

#!/bin/bash
# Look for any documentation about memtrace file format or constraints
rg -n "memtrace|rank.*file|trace.*format" src/mem/cache/tags/base.cc -B 2 -A 2

Repository: OpenXiangShan/GEM5

Length of output: 1166


Guard against tick underflow when applying memtrace priority.

The priority parameter is parsed from a memtrace file via std::stoi(rank) without validation. If priority is negative or exceeds curTick(), the subtraction curTick() - priority causes unsigned integer underflow, inverting LRU ordering during warmup. Clamp or validate priority to prevent silent corruption of cache replacement state.

🤖 Prompt for AI Agents
In `@src/mem/cache/replacement_policies/lru_rp.cc` around lines 73 - 80, In
LRU::reset4memtrace, guard against unsigned underflow when computing
lastTouchTick = curTick() - priority by validating/clamping the parsed memtrace
priority: ensure priority is non-negative and not greater than curTick() before
subtracting. Locate the method LRU::reset4memtrace and the use of
std::static_pointer_cast<LRUReplData>(replacement_data)->lastTouchTick, and
replace the direct subtraction with a clamped value (e.g., use min(max(priority,
0), curTick())) or equivalent logic so lastTouchTick never underflows.


ReplaceableEntry*
LRU::getVictim(const ReplacementCandidates& candidates) const
{
Expand Down
2 changes: 2 additions & 0 deletions src/mem/cache/replacement_policies/lru_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ class LRU : public Base
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override;

/**
* Find replacement victim using LRU timestamps.
*
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/mru_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ class MRU : public Base
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
/**
* Find replacement victim using access timestamps.
*
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/random_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ class Random : public Base
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
/**
* Find replacement victim at random.
*
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/second_chance_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ class SecondChance : public FIFO
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
/**
* Find replacement victim using insertion timestamps and second chance
* bit.
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/ship_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ class SHiP : public BRRIP
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
/**
* Instantiate a replacement data entry.
*
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/tree_plru_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,9 @@ class TreePLRU : public Base
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
/**
* Find replacement victim using TreePLRU bits. It is assumed that all
* candidates share the same replacement data tree.
Expand Down
3 changes: 3 additions & 0 deletions src/mem/cache/replacement_policies/xs_drrip_rp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,9 @@ class XSDRRIP : public Base
void reset(const std::shared_ptr<ReplacementData>& replacement_data) const
override;

void reset4memtrace(const std::shared_ptr<ReplacementData>& replacement_data,int priority) const override
{
}
Comment on lines +207 to +209
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

XSDRRIP::reset4memtrace as no-op breaks replacement-state restoration.

This policy carries nontrivial state (rrpv, valid, dueling behavior). Leaving the memtrace hook empty means restored priority/rank is ignored, so post-restore behavior diverges from intended cache state.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/mem/cache/replacement_policies/xs_drrip_rp.hh` around lines 207 - 209,
XSDRRIP::reset4memtrace is currently a no-op which prevents restoration of
replacement state; implement it to read the saved ReplacementData and restore
the policy-specific fields (e.g., rrpv, valid bit, and any dueling/leader state
or counters) into the target set/way so the in-memory structure reflects the
saved priority/rank; specifically, inside XSDRRIP::reset4memtrace extract the
policy blob from the replacement_data argument and assign its rrpv and valid
fields and restore dueling-related flags/counters back into the object's per-way
state (and update any aggregated counters if needed) so restored entries behave
identically after memtrace replay.

/**
* Find victim for replacement
*/
Expand Down
192 changes: 192 additions & 0 deletions src/mem/cache/tags/base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,16 @@
#include "sim/sim_exit.hh"
#include "sim/system.hh"

#include <iostream>
#include <zlib.h>
#include <cstdio>
#include <fstream>
#include <vector>
#include <stdexcept>
#include <cstdint>
#include <iomanip>
#include "sim/root.hh"

namespace gem5
{

Expand Down Expand Up @@ -311,4 +321,186 @@ BaseTags::BaseTagStats::preDumpStats()
tags.computeStats();
}


// restore L3 cache microarchitecture states based on memtrace
void
BaseTags::warmupState(const std::string &pmem_file,const std::string &memtrace_file)
{
std::ifstream file(memtrace_file); // the file contains the microarchitecture states from memtrace
if(!file.is_open())
{
std::cout << "File open failed:" << memtrace_file << std::endl;
};
std::string line;
std::string taskid;
std::string requestorid;
std::string rank;
int line_max = this->size / this->blkSize; //compute the number of cache lines
volatile int offset_num=0;
int num = this->blkMask;
while(num) //compute tag+set bits
{
offset_num += num & 1;
num >>= 1;
}
Comment on lines +339 to +345
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Remove incorrect use of volatile qualifier.

The volatile keyword on offset_num and set_num is incorrect here. volatile is for memory-mapped I/O or signal handlers, not for regular computation. This may also prevent compiler optimizations unnecessarily.

Proposed fix
-     volatile int offset_num=0;
+     int offset_num = 0;
      int num = this->blkMask;
      ...
-     volatile int set_num=0;
+     int set_num = 0;
      while(num)
      ...
-     volatile int tag_num= total_num-set_num-offset_num;
+     int tag_num = total_num - set_num - offset_num;

Also applies to: 354-360

🤖 Prompt for AI Agents
In `@src/mem/cache/tags/base.cc` around lines 340 - 346, Remove the incorrect
volatile qualifier from the local counters used for bit counting: change
volatile int offset_num to just int offset_num in the tag computation loop
(inside the function that computes tag+set bits) and likewise remove volatile
from set_num in the corresponding set-bit loop (the block around the code
referenced at lines 354-360). These are ordinary local arithmetic variables
(used in the tag/set bit calculation) and should be plain ints to allow normal
optimizations; keep the existing loop logic and only delete the volatile keyword
where offset_num and set_num are declared.


int total_num =64; //compute set bits

int assoc = this->indexingPolicy->getAssoc();

num = size/assoc;
num = num/blkSize;
volatile int set_num=0;
while(num)
{
num >>= 1;
set_num++;
}
set_num--;
volatile int tag_num= total_num-set_num-offset_num;

std::vector<char> decompressed_data = decompress_gz_to_memory(pmem_file);
for(int line_num=0;line_num<line_max;line_num++)
{
std::getline(file,line);
std::getline(file,taskid);
std::getline(file,requestorid);
std::getline(file,rank);
int memtrace_priority = std::stoi(rank);
char myvalid = line[0];
char myhit = line[1];
Comment on lines +365 to +371
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing bounds checks before accessing line characters.

The code accesses line[0] and line[1] without verifying the line has sufficient length. If std::getline reads an empty line or the file ends unexpectedly, this will cause undefined behavior.

Proposed fix
         std::getline(file,line);
         std::getline(file,taskid);
         std::getline(file,requestorid);
         std::getline(file,rank);
+        if (line.size() < 2) {
+            panic("Invalid memtrace line format at line %d", line_num);
+        }
         int memtrace_priority = std::stoi(rank);
         char myvalid = line[0];
         char myhit = line[1];
🤖 Prompt for AI Agents
In `@src/mem/cache/tags/base.cc` around lines 366 - 372, The code reads several
lines and then does char myvalid = line[0] and char myhit = line[1] without
checking length; update the parsing logic in the routine that calls std::getline
(the block that computes memtrace_priority and declares myvalid/myhit) to
validate that 'line' has at least two characters (e.g., if (line.size() < 2) {
/* handle error: log, set defaults, skip record or continue */ }) before
indexing, and ensure any fallback behavior (default values, skipping the entry,
or returning an error) is applied consistently so you avoid undefined behavior
when getline returns an empty or short string.

if( myhit == '1')
{
std::string mytag = line.substr(3,tag_num); // paddr
int myset = line_num / assoc; // paddr
std::bitset<32> setbin(myset);
std::string myset_str = setbin.to_string();

myset_str=myset_str.substr(myset_str.size()-set_num,set_num);
std::string myaddr = mytag+myset_str;
myaddr.append(offset_num,'0');//paddr

const Addr p_addr = std::stoull(myaddr, nullptr, 2);
const Addr h_addr = p_addr - 0x100000000 + 0x80000000;// host
Addr tag = std::stoull(mytag, nullptr, 2);
uint32_t set = indexingPolicy->myextractSet(p_addr);
const bool is_secure = false;

std::size_t blk_size_bits = blkSize*8;


// Find replacement victim
std::vector<CacheBlk*> evict_blks;
CacheBlk *victim = this->findVictim(p_addr, is_secure, blk_size_bits,
evict_blks);
this->updateRp(victim,memtrace_priority);//replacement policy state update
victim->insert(tag, is_secure);
victim->setSrcRequestorId_pub(static_cast<uint16_t>(std::stoul(requestorid)));
victim->setTaskId_pub(static_cast<uint32_t>(std::stoul(taskid)));
victim->setTickInserted_pub();
victim->setCoherenceBits(CacheBlk::WritableBit);
victim->setCoherenceBits(CacheBlk::ReadableBit);

Addr offset = p_addr & Addr (blkSize - 1);
unsigned size = this->blkSize;
char result_buffer[size + 1];
size_t bytes_read = query_in_memory(decompressed_data, h_addr, result_buffer, size);
std::memcpy(victim->data + offset, result_buffer, size);
Comment on lines +404 to +408
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Variable shadowing, non-standard VLA, and potential buffer overflow.

Multiple serious issues:

  1. Variable shadowing: unsigned size = this->blkSize shadows the class member size.

  2. Non-standard VLA: char result_buffer[size + 1] uses a variable-length array which is not standard C++ (it's a GCC extension).

  3. Buffer overflow risk: The code copies size bytes starting at offset into victim->data + offset. If offset > 0, this writes past the end of the cache block data.

Proposed fix
-    Addr offset = p_addr & Addr (blkSize - 1);
-    unsigned size = this->blkSize;
-    char result_buffer[size + 1];
-    size_t bytes_read = query_in_memory(decompressed_data, h_addr, result_buffer, size);
-    std::memcpy(victim->data + offset, result_buffer, size);
+    // For warmup, we load the full cache line (offset should be 0 for aligned addresses)
+    Addr offset = p_addr & Addr(blkSize - 1);
+    assert(offset == 0 && "Expected aligned address for cache warmup");
+    std::vector<char> result_buffer(blkSize);
+    size_t bytes_read = query_in_memory(decompressed_data, h_addr, result_buffer.data(), blkSize);
+    std::memcpy(victim->data, result_buffer.data(), bytes_read);
📝 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.

Suggested change
Addr offset = p_addr & Addr (blkSize - 1);
unsigned size = this->blkSize;
char result_buffer[size + 1];
size_t bytes_read = query_in_memory(decompressed_data, h_addr, result_buffer, size);
std::memcpy(victim->data + offset, result_buffer, size);
// For warmup, we load the full cache line (offset should be 0 for aligned addresses)
Addr offset = p_addr & Addr(blkSize - 1);
assert(offset == 0 && "Expected aligned address for cache warmup");
std::vector<char> result_buffer(blkSize);
size_t bytes_read = query_in_memory(decompressed_data, h_addr, result_buffer.data(), blkSize);
std::memcpy(victim->data, result_buffer.data(), bytes_read);
🤖 Prompt for AI Agents
In `@src/mem/cache/tags/base.cc` around lines 405 - 409, Local variable shadowing
and a non-standard VLA cause correctness and safety bugs: rename the local
unsigned to e.g. block_size = this->blkSize, replace the VLA with a standard
container (std::vector<char> result_buffer(block_size + 1)), call
query_in_memory(...) into result_buffer.data(), use the returned bytes_read and
compute size_t copy_len = std::min(bytes_read, static_cast<size_t>(block_size) -
offset) then std::memcpy(victim->data + offset, result_buffer.data(), copy_len);
ensure you never copy more than block_size - offset to avoid overrunning the
cache block (and handle the case bytes_read < copy_len if you need to
zero-fill).

victim->setWhenReady(curTick());
}
else if(myvalid != '1')
{
break;
}
}
file.close();
}


std::vector<char>
BaseTags::decompress_gz_to_memory(const std::string& gz_path) {
std::vector<char> decompressed_data;
const size_t CHUNK_SIZE = 32 * 1024;
std::vector<char> in_buffer(CHUNK_SIZE);
std::vector<char> out_buffer(CHUNK_SIZE * 2);
std::ifstream gz_file(gz_path, std::ios_base::binary);
if (!gz_file.is_open()) {
throw std::runtime_error("File open failed: " + gz_path);
}
z_stream strm;
strm.zalloc = Z_NULL;
strm.zfree = Z_NULL;
strm.opaque = Z_NULL;
int ret = inflateInit2(&strm, MAX_WBITS | 16);
if (ret != Z_OK) {
throw std::runtime_error("inflateInit2 falied: " + std::to_string(ret));
}

while (true) {
gz_file.read(in_buffer.data(), in_buffer.size());
std::streamsize bytes_read = gz_file.gcount();

strm.avail_in = static_cast<uInt>(bytes_read);
strm.next_in = reinterpret_cast<Bytef*>(in_buffer.data());

if (strm.avail_in == 0 && strm.avail_out == 0) {
break;
}
Comment on lines +446 to +448
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n src/mem/cache/tags/base.cc | sed -n '440,465p'

Repository: OpenXiangShan/GEM5

Length of output: 1175


🏁 Script executed:

cat -n src/mem/cache/tags/base.cc | sed -n '420,480p'

Repository: OpenXiangShan/GEM5

Length of output: 2523


The condition strm.avail_in == 0 && strm.avail_out == 0 checks the wrong state at the wrong time.

Line 447 evaluates strm.avail_out before it is set within the do-while loop on line 452. More critically, the do-while loop on line 451–470 continues while (strm.avail_out == 0), meaning it exits when avail_out > 0. On subsequent iterations of the outer loop, avail_out retains a value > 0 from the previous iteration, making the strm.avail_out == 0 check ineffective. The actual EOF condition is already handled correctly on line 472–474 with if (bytes_read == 0).

🤖 Prompt for AI Agents
In `@src/mem/cache/tags/base.cc` around lines 447 - 449, The condition checking
both strm.avail_in == 0 && strm.avail_out == 0 is incorrect and uses stale
avail_out from prior iterations; remove or replace that check and rely on the
existing EOF handling via bytes_read (the bytes_read == 0 branch) and the
do-while loop that continues while strm.avail_out == 0. Specifically, in the
outer loop surrounding the do-while that inspects strm.avail_out, eliminate the
redundant avail_out check (or change it to only test strm.avail_in == 0 if you
need to detect input exhaustion) so control flow depends on bytes_read and the
inner loop condition, referencing the variables strm.avail_in, strm.avail_out,
bytes_read and the outer loop that currently contains the faulty if.


do {
strm.avail_out = static_cast<uInt>(out_buffer.size());
strm.next_out = reinterpret_cast<Bytef*>(out_buffer.data());

ret = inflate(&strm, Z_NO_FLUSH);

if (ret == Z_STREAM_ERROR) {
inflateEnd(&strm);
gz_file.close();
throw std::runtime_error("inflate failed: " + std::to_string(ret));
}
Comment on lines +454 to +460
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect zlib return-code handling in decompress loop
rg -nP 'inflate\(|Z_STREAM_ERROR|Z_DATA_ERROR|Z_MEM_ERROR|Z_NEED_DICT|Z_STREAM_END' src/mem/cache/tags/base.cc -C2

Repository: OpenXiangShan/GEM5

Length of output: 434


🏁 Script executed:

cat -n src/mem/cache/tags/base.cc | sed -n '450,485p'

Repository: OpenXiangShan/GEM5

Length of output: 1487


🌐 Web query:

zlib inflate() return codes Z_OK Z_STREAM_ERROR Z_DATA_ERROR Z_MEM_ERROR Z_NEED_DICT

💡 Result:

inflate() return codes (zlib):

  • Z_OK (0): Some progress was made (consumed input and/or produced output), but the stream is not finished yet. [1]
  • Z_STREAM_ERROR (-2): The z_stream state is invalid/inconsistent (e.g., next_in/next_out is Z_NULL, or state was corrupted/overwritten). [1]
  • Z_DATA_ERROR (-3): Input is corrupted or not in the expected format (e.g., bad zlib/gzip wrapper, invalid deflate stream, or checksum/check value mismatch). strm->msg may contain a more specific error string. [1]
  • Z_MEM_ERROR (-4): Not enough memory to perform decompression (e.g., allocating internal state/window). [1]
  • Z_NEED_DICT (2): A preset dictionary is required to continue; strm->adler is set to the Adler-32 of the required dictionary. Call inflateSetDictionary() with the correct dictionary, then continue inflating. [1]

Source: zlib manual for inflate(). [1]

References

Citations:


Handle all inflate failure codes immediately in the loop, not only Z_STREAM_ERROR.

The inflate() function can return Z_STREAM_ERROR, Z_DATA_ERROR, Z_MEM_ERROR, or Z_NEED_DICT during decompression. Lines 456–460 only check for Z_STREAM_ERROR, allowing corrupted or erroneous decompression to continue until the final check at line 476. These errors should be caught immediately in the loop. Check the return value against all possible error codes: Z_DATA_ERROR (data corruption), Z_MEM_ERROR (allocation failure), and Z_NEED_DICT (dictionary requirement).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/mem/cache/tags/base.cc` around lines 454 - 460, The loop currently only
treats Z_STREAM_ERROR as an immediate failure after calling inflate(&strm,...);
update the check to treat Z_DATA_ERROR, Z_MEM_ERROR and Z_NEED_DICT as immediate
failures as well: if ret equals any of those error codes (or Z_STREAM_ERROR)
call inflateEnd(&strm), close gz_file, and throw a std::runtime_error that
includes the ret value and a short message; continue using the same symbols
(inflate, inflateEnd, strm, ret, gz_file) so the change is localized to the
existing loop error handling.


size_t bytes_decompressed = out_buffer.size() - strm.avail_out;
if (bytes_decompressed > 0) {
decompressed_data.insert(decompressed_data.end(),
out_buffer.begin(),
out_buffer.begin() + bytes_decompressed);
}

} while (strm.avail_out == 0);

if (bytes_read == 0) {
break;
}
}

if (ret != Z_STREAM_END) {
inflateEnd(&strm);
gz_file.close();
throw std::runtime_error("Decompression did not complete normally, the file may be corrupted. zlib return code: " + std::to_string(ret));
}

inflateEnd(&strm);
gz_file.close();
return decompressed_data;
}
size_t
BaseTags::query_in_memory(const std::vector<char>& data,
uint64_t target_address,
char* result,
size_t max_result_length) {
if (max_result_length == 0 || result == nullptr) {
return 0;
}

if (target_address >= data.size()) {
throw std::out_of_range("The destination address 0x "+ std::to_string(target_address) + " is out of the unzipped data range.");
}

size_t bytes_to_copy = std::min(max_result_length, data.size() - static_cast<size_t>(target_address));

std::memcpy(result, data.data() + target_address, bytes_to_copy);

return bytes_to_copy;
}

} // namespace gem5
Loading