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.

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

i#6662 public traces: disable read/write counts in invariant_checker #7140

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

i#6662 public traces: disable read/write counts in invariant_checker #7140

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f593a5
i#6662 public traces: disable read/write counts in invariant_checker
edeiana Dec 16, 2024
91e2497
Merge branch 'master' into i6662-invariant_checker-fix
edeiana Dec 16, 2024
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
62 changes: 20 additions & 42 deletions clients/drcachesim/tools/invariant_checker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,26 +809,10 @@ invariant_checker_t::parallel_shard_memref(void *shard_data, const memref_t &mem
instr_writes_memory(noalloc_instr);
edeiana marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, not all instructions that then perform a read or a write have a
corresponding load or a store category, leading to false positive
"Too many reads/writes" invariants firing.

Wait a minute: this does not sound right. Every instruction doing a load or store should have that category: that's the point of the category. I thought the problem was the other way around, instructions with variable number of loads/stores: e.g., masked or predicated scatter/gather.

e.g., no store category for this instruction that performs a write operation:

13123537 10416044: 7446 ifetch 4 byte(s) @ 0x0000aaaaefe73500 00000831 0e0d0d04 load [4byte] %rv11 %rv12 %rv13 -> %rv11
13123537 10416044: 7446 0000000f
13123538 10416044: 7446 read 4 byte(s) @ 0x0000115cffc33ba0 by PC 0x0000aaaaefe73500
13123539 10416044: 7446 write 4 byte(s) @ 0x0000115cffc33ba0 by PC 0x0000aaaaefe73500

This is a bug, isn't it? What's the original opcode? Sounds like a category bug in DR's decoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it's a bug in the categories of some AARCH64 instructions that propagates into their corresponding REGDEPS instructions.
Bug filed: #7146.
Holding on this change until we know we can solve it.

cur_instr_info.decoding.is_predicated =
instr_is_predicated(noalloc_instr);
// DR_ISA_REGDEPS instructions don't have an opcode, hence we use
// their category to determine whether they perform at least one read
// or write.
if (instr_get_isa_mode(noalloc_instr) == DR_ISA_REGDEPS) {
cur_instr_info.decoding.num_memory_read_access =
TESTANY(DR_INSTR_CATEGORY_LOAD,
instr_get_category(noalloc_instr))
? 1
: 0;
cur_instr_info.decoding.num_memory_write_access =
TESTANY(DR_INSTR_CATEGORY_STORE,
instr_get_category(noalloc_instr))
? 1
: 0;
} else {
cur_instr_info.decoding.num_memory_read_access =
instr_num_memory_read_access(noalloc_instr);
cur_instr_info.decoding.num_memory_write_access =
instr_num_memory_write_access(noalloc_instr);
}
cur_instr_info.decoding.num_memory_read_access =
instr_num_memory_read_access(noalloc_instr);
cur_instr_info.decoding.num_memory_write_access =
instr_num_memory_write_access(noalloc_instr);
if (type_is_instr_branch(memref.instr.type) &&
// DR_ISA_REGDEPS instructions don't have a branch target saved as
// instr.src[0], so we cannot retrieve this information.
Expand Down Expand Up @@ -1213,37 +1197,31 @@ invariant_checker_t::parallel_shard_memref(void *shard_data, const memref_t &mem
report_if_false(
shard,
shard->expected_read_records_ >
0 IF_X86(|| relax_expected_read_count_check_for_kernel(shard)),
0 IF_X86(||
relax_expected_read_count_check_for_kernel(shard)) ||
// We cannot check the shard->expected_read_records_ counter in
// OFFLINE_FILE_TYPE_ARCH_REGDEPS traces because we cannot
// determine the exact number of loads a DR_ISA_REGDEPS
// instruction performs.
TESTANY(OFFLINE_FILE_TYPE_ARCH_REGDEPS, shard->file_type_),
"Too many read records");
if (shard->expected_read_records_ > 0 &&
// We cannot decrease the shard->expected_read_records_ counter in
// OFFLINE_FILE_TYPE_ARCH_REGDEPS traces because we cannot determine
// the exact number of loads a DR_ISA_REGDEPS instruction performs.
// If a DR_ISA_REGDEPS instruction has a DR_INSTR_CATEGORY_LOAD among
// its categories, we simply set
// cur_instr_info.decoding.num_memory_read_access to 1. We rely on the
// next instruction to re-set shard->expected_read_records_
// accordingly.
!TESTANY(OFFLINE_FILE_TYPE_ARCH_REGDEPS, shard->file_type_)) {
if (shard->expected_read_records_ > 0) {
shard->expected_read_records_--;
}
} else {
// Skip D-filtered traces which don't have every load or store records.
report_if_false(
shard,
shard->expected_write_records_ >
0 IF_X86(|| relax_expected_write_count_check_for_kernel(shard)),
0 IF_X86(
|| relax_expected_write_count_check_for_kernel(shard)) ||
// We cannot check the shard->expected_write_records_ counter in
// OFFLINE_FILE_TYPE_ARCH_REGDEPS traces because we cannot
// determine the exact number of stores a DR_ISA_REGDEPS
// instruction performs.
TESTANY(OFFLINE_FILE_TYPE_ARCH_REGDEPS, shard->file_type_),
"Too many write records");
if (shard->expected_write_records_ > 0 &&
// We cannot decrease the shard->expected_write_records_ counter in
// OFFLINE_FILE_TYPE_ARCH_REGDEPS traces because we cannot determine
// the exact number of stores a DR_ISA_REGDEPS instruction performs.
// If a DR_ISA_REGDEPS instruction has a DR_INSTR_CATEGORY_STORE among
// its categories, we simply set
// cur_instr_info.decoding.num_memory_write_access to 1. We rely on
// the next instruction to re-set shard->expected_write_records_
// accordingly.
!TESTANY(OFFLINE_FILE_TYPE_ARCH_REGDEPS, shard->file_type_)) {
if (shard->expected_write_records_ > 0) {
shard->expected_write_records_--;
}
}
Expand Down
Loading