Skip to content
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.

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

Calling cache_fifo::replace_which_way() directly results in a change in FIFO cache state #5456

Closed
bete0 opened this issue Apr 11, 2022 · 0 comments

Comments

@bete0
Copy link
Contributor

bete0 commented Apr 11, 2022

While working on a unit test for clients/drcachesim/simulator/cache_fifo.cpp (#5454), I discovered that calling cache_fifo::replace_which_way() right after accessing a cache line (by calling request()) would update the cache state.

For the cache replacement unit tests, we validate the expected way is replaced after an access by first sending a request() and then checking the way to be replaced on the next access matches the expected way as shown below (from #5454):

 void
 access_and_check_fifo(const addr_t addr,
                          const int expected_replacement_way_after_access)
 {
        memref_t ref;
        ref.data.type = TRACE_TYPE_READ;
        ref.data.size = 1;
        ref.data.addr = addr;
        request(ref);
        assert(replace_which_way(get_block_index(addr)) ==
                                 expected_replacement_way_after_access);
}

However calling replace_which_way() changes the state of the FIFO cache by updating the way to be replaced which results in the wrong way to be replaced on the next access.

This is not necessarily a bug in the FIFO cache replacement implementation but makes testing the implementation harder and potentially could result in unexpected behavior if cache_fifo::replace_which_way() is called directly somewhere else.

Steps to reproduce the behavior:

  1. Checkout PR i#4842 drcachesim unit test: Add cache_fifo unit test #5454, build and run drcachesim unit test and observe the test fails
  2. Update the test cases from https://github.com/DynamoRIO/dynamorio/pull/5454/files#diff-b7acbebc334f2800e156dcb14d7ad91ea8b243fee66b9cf3011430d2605081b9R165-R168 as follows and observe the test passes
  cache_fifo_test.access_and_check_fifo(ADDRESS_A, 1); // A x X X
  cache_fifo_test.access_and_check_fifo(ADDRESS_B, 3); // A B x X
  cache_fifo_test.access_and_check_fifo(ADDRESS_C, 1); // A B C x
  cache_fifo_test.access_and_check_fifo(ADDRESS_D, 3); // a B C D
bete0 added a commit that referenced this issue Apr 15, 2022
Adds a non-state changing method get_next_way_to_replace() that
just returns the victim 'way' without updating the cache state. This
makes unit testing the FIFO cache replacement policy convenient.

Issue: #5456
@bete0 bete0 closed this as completed May 24, 2022
bete0 added a commit that referenced this issue May 25, 2022
Adds get_next_way_to_replace() helper method that just returns the victim way
without a side effect on the cache state. This makes unit testing the cache
replacement policies convenient and avoids unexpected behavior during testing.

Adds cache_policy_test_t class template for testing multiple cache replacement policies.

Adds get_block_index() method that returns the block index for a given address.

Issue: #4842, #5456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant