Skip to content

[Fix]: Tiny possible issues #283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
1a1a11a merged 2 commits into from
Aug 1, 2025
Merged

[Fix]: Tiny possible issues #283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59df2ae
Fix tiny possible issues
haochengxia Aug 1, 2025
9592379
Add default param string for consistency
haochengxia Aug 1, 2025
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
8 changes: 4 additions & 4 deletions libCacheSim/cache/eviction/ARC.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,14 +222,14 @@ static cache_obj_t *ARC_find(cache_t *cache, const request_t *req,

cache_obj_t *obj = cache_find_base(cache, req, update_cache);

if (!update_cache) {
return obj->ARC.ghost ? NULL : obj;
}

if (obj == NULL) {
return NULL;
}
Comment on lines 225 to 227
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Aug 1, 2025

Choose a reason for hiding this comment

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

critical

This reordering is a critical fix. The previous logic could lead to a null pointer dereference if cache_find_base returned NULL and update_cache was false, as it would attempt to access obj->ARC.ghost before checking if obj was NULL. This change correctly handles this by checking for NULL first.


if (!update_cache) {
return obj->ARC.ghost ? NULL : obj;
}

params->curr_obj_in_L1_ghost = false;
params->curr_obj_in_L2_ghost = false;

Expand Down
9 changes: 8 additions & 1 deletion libCacheSim/cache/eviction/LRUProb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ typedef struct LRU_Prob_params {
int threshold;
} LRU_Prob_params_t;

static const char *DEFAULT_CACHE_PARAMS = "prob=0.5";

// ***********************************************************************
// **** ****
// **** function declarations ****
Expand Down Expand Up @@ -77,8 +79,10 @@ cache_t *LRU_Prob_init(const common_cache_params_t ccache_params,
cache->eviction_params =
(LRU_Prob_params_t *)malloc(sizeof(LRU_Prob_params_t));
LRU_Prob_params_t *params = (LRU_Prob_params_t *)(cache->eviction_params);
params->prob = 0.5;
params->q_head = NULL;
params->q_tail = NULL;
Comment on lines +82 to +83
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Aug 1, 2025

Choose a reason for hiding this comment

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

high

Good catch! Initializing q_head and q_tail to NULL is crucial. malloc does not guarantee zero-initialized memory, so without this change, these pointers would hold indeterminate values, likely causing undefined behavior or crashes later.


LRU_Prob_parse_params(cache, DEFAULT_CACHE_PARAMS);
if (cache_specific_params != NULL) {
LRU_Prob_parse_params(cache, cache_specific_params);
}
Expand Down Expand Up @@ -169,6 +173,7 @@ static cache_obj_t *LRU_Prob_find(cache_t *cache, const request_t *req,
*/
static cache_obj_t *LRU_Prob_insert(cache_t *cache, const request_t *req) {
LRU_Prob_params_t *params = (LRU_Prob_params_t *)cache->eviction_params;

cache_obj_t *obj = cache_insert_base(cache, req);
prepend_obj_to_head(&params->q_head, &params->q_tail, obj);

Expand Down Expand Up @@ -203,6 +208,8 @@ static cache_obj_t *LRU_Prob_to_evict(cache_t *cache, const request_t *req) {
static void LRU_Prob_evict(cache_t *cache, const request_t *req) {
LRU_Prob_params_t *params = (LRU_Prob_params_t *)cache->eviction_params;
cache_obj_t *obj_to_evict = params->q_tail;
DEBUG_ASSERT(params->q_tail != NULL);
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Aug 1, 2025

Choose a reason for hiding this comment

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

medium

Adding this DEBUG_ASSERT is a good defensive measure. It ensures the invariant that LRU_Prob_evict is not called on an empty queue, which could cause a crash when obj_to_evict (which would be NULL) is passed to remove_obj_from_list.


remove_obj_from_list(&params->q_head, &params->q_tail, obj_to_evict);
cache_remove_obj_base(cache, obj_to_evict, true);
}
Expand Down
6 changes: 5 additions & 1 deletion libCacheSim/cache/eviction/SR_LRU.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,13 @@ static cache_obj_t *SR_LRU_find(cache_t *cache, const request_t *req,
params->C_demoted -= 1;
}

if (cache_hit_R || cache_hit_SR) {
if (cache_hit_R || (cache_hit_SR && likely(update_cache))) {
// if not update_cache obj_SR will not be updated to obj_R
DEBUG_ASSERT(obj_R != NULL);
return obj_R;
} else if (cache_hit_SR) {
// if not update_cache, directly return obj_SR
return obj_SR;
Comment on lines +211 to +217
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Aug 1, 2025

Choose a reason for hiding this comment

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

high

This is an important correctness fix. The previous logic would incorrectly modify the cache state (promoting an object from SR to R) even when update_cache was false. This change correctly handles the case where update_cache is false by returning the object from the SR segment without promotion.

For better readability and maintainability, you could consider restructuring it to handle each case more explicitly.

if (cache_hit_R) {
    DEBUG_ASSERT(obj_R != NULL);
    return obj_R;
  }

  if (cache_hit_SR && likely(update_cache)) {
    DEBUG_ASSERT(obj_R != NULL);
    return obj_R;
  } else if (cache_hit_SR) {
    return obj_SR;
  }
  return NULL;

}
return NULL;
}
Expand Down
Loading