From b6b7e1362b26f21f9c279df05a9efdcc1c124a60 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 16 Aug 2023 23:13:17 -0400 Subject: [PATCH] i#5538 skip: Fix skip-1-instr and skip-0-instr cases Fixes a boundary case of skipping 1 instruction when the scheduler has already read an instruction record but not yet passed it to the user. Fixes a boundary case of back-to-back regions of interest. Adds test cases. Issue: #5538 --- clients/drcachesim/reader/reader.cpp | 2 - clients/drcachesim/reader/reader.h | 2 + clients/drcachesim/scheduler/scheduler.cpp | 5 + clients/drcachesim/scheduler/scheduler.h | 7 +- .../drcachesim/tests/scheduler_unit_tests.cpp | 117 +++++++++++++++--- 5 files changed, 108 insertions(+), 25 deletions(-) diff --git a/clients/drcachesim/reader/reader.cpp b/clients/drcachesim/reader/reader.cpp index 09a945954bb..1cd157ab44a 100644 --- a/clients/drcachesim/reader/reader.cpp +++ b/clients/drcachesim/reader/reader.cpp @@ -388,8 +388,6 @@ reader_t::pre_skip_instructions() reader_t & reader_t::skip_instructions(uint64_t instruction_count) { - if (instruction_count == 0) - return *this; // We do not support skipping with instr bundles. if (bundle_idx_ != 0) { ERRMSG("Skipping with instr bundles is not supported.\n"); diff --git a/clients/drcachesim/reader/reader.h b/clients/drcachesim/reader/reader.h index 943dca44375..54292464707 100644 --- a/clients/drcachesim/reader/reader.h +++ b/clients/drcachesim/reader/reader.h @@ -124,6 +124,8 @@ class reader_t : public std::iterator, // observed. This generally should call pre_skip_instructions() to observe the // headers, perform any fast skipping, and then should call // skip_instructions_with_timestamp() to properly duplicate the prior timestamp. + // Skipping 0 instructions is supported and will skip ahead to right before the + // next instruction. virtual reader_t & skip_instructions(uint64_t instruction_count); diff --git a/clients/drcachesim/scheduler/scheduler.cpp b/clients/drcachesim/scheduler/scheduler.cpp index 0daadbbe1c6..ffd5e8d2ee7 100644 --- a/clients/drcachesim/scheduler/scheduler.cpp +++ b/clients/drcachesim/scheduler/scheduler.cpp @@ -1269,6 +1269,11 @@ scheduler_tmpl_t::advance_region_of_interest( cur_range = input.regions_of_interest[input.cur_region]; } + if (cur_instr >= cur_range.start_instruction) { + // We're already there (back-to-back regions). We do not insert a separator. + input.in_cur_region = true; + return sched_type_t::STATUS_OK; + } // If we're within one and already skipped, just exit to avoid re-requesting a skip // and making no progress (we're on the inserted timetamp + cpuid and our cur instr // count isn't yet the target). diff --git a/clients/drcachesim/scheduler/scheduler.h b/clients/drcachesim/scheduler/scheduler.h index a6e41d0d1f8..d1133275bee 100644 --- a/clients/drcachesim/scheduler/scheduler.h +++ b/clients/drcachesim/scheduler/scheduler.h @@ -200,9 +200,10 @@ template class scheduler_tmpl_t { * though the input were constructed by concatenating these ranges together. A * #TRACE_MARKER_TYPE_WINDOW_ID marker is inserted between * ranges (with a value equal to the range ordinal) to notify the client of the - * discontinuity (but not before the first range), with a - * #dynamorio::drmemtrace::TRACE_TYPE_THREAD_EXIT record inserted after the final - * range. These ranges must be non-overlapping and in increasing order. + * discontinuity (but not before the first range nor between back-to-back regions + * with no separation), with a #dynamorio::drmemtrace::TRACE_TYPE_THREAD_EXIT + * record inserted after the final range. These ranges must be non-overlapping + * and in increasing order. */ std::vector regions_of_interest; }; diff --git a/clients/drcachesim/tests/scheduler_unit_tests.cpp b/clients/drcachesim/tests/scheduler_unit_tests.cpp index 96b84ab09be..e2db2866aef 100644 --- a/clients/drcachesim/tests/scheduler_unit_tests.cpp +++ b/clients/drcachesim/tests/scheduler_unit_tests.cpp @@ -424,11 +424,13 @@ test_regions_bare() make_instr(1), make_instr(2), // Region 1 is just this instr. make_instr(3), - make_instr(4), - make_instr(5), - make_instr(6), // Region 2 starts here. - make_instr(7), // Region 2 ends here. - make_instr(8), + make_instr(4), // Region 2 is just this instr. + make_instr(5), // Region 3 is just this instr. + make_instr(6), + make_instr(7), + make_instr(8), // Region 4 starts here. + make_instr(9), // Region 4 ends here. + make_instr(10), make_exit(1), /* clang-format on */ }; @@ -439,7 +441,9 @@ test_regions_bare() std::vector regions; // Instr counts are 1-based. regions.emplace_back(2, 2); - regions.emplace_back(6, 7); + regions.emplace_back(4, 4); + regions.emplace_back(5, 5); + regions.emplace_back(8, 9); scheduler_t scheduler; std::vector sched_inputs; @@ -470,17 +474,92 @@ test_regions_bare() break; case 2: assert(type_is_instr(memref.instr.type)); - assert(memref.instr.addr == 6); + assert(memref.instr.addr == 4); break; case 3: + // No window separator for back-to-back regions. assert(type_is_instr(memref.instr.type)); - assert(memref.instr.addr == 7); + assert(memref.instr.addr == 5); break; - default: assert(ordinal == 4); assert(memref.exit.type == TRACE_TYPE_THREAD_EXIT); + case 4: + assert(memref.marker.type == TRACE_TYPE_MARKER); + assert(memref.marker.marker_type == TRACE_MARKER_TYPE_WINDOW_ID); + assert(memref.marker.marker_value == 3); + break; + case 5: + assert(type_is_instr(memref.instr.type)); + assert(memref.instr.addr == 8); + break; + case 6: + assert(type_is_instr(memref.instr.type)); + assert(memref.instr.addr == 9); + break; + default: assert(ordinal == 7); assert(memref.exit.type == TRACE_TYPE_THREAD_EXIT); } ++ordinal; } - assert(ordinal == 5); + assert(ordinal == 8); +} + +// Tests regions without timestamps with an instr at the very front of the trace. +static void +test_regions_bare_no_marker() +{ + std::cerr << "\n----------------\nTesting bare regions with no marker\n"; + std::vector memrefs = { + /* clang-format off */ + make_thread(1), + make_pid(1), + make_instr(1), + make_instr(2), // The region skips the 1st instr. + make_instr(3), + make_instr(4), + make_exit(1), + /* clang-format on */ + }; + std::vector readers; + readers.emplace_back(std::unique_ptr(new mock_reader_t(memrefs)), + std::unique_ptr(new mock_reader_t()), 1); + + std::vector regions; + // Instr counts are 1-based. + regions.emplace_back(2, 0); + + scheduler_t scheduler; + std::vector sched_inputs; + sched_inputs.emplace_back(std::move(readers)); + sched_inputs[0].thread_modifiers.push_back(scheduler_t::input_thread_info_t(regions)); + // Without timestmaps we can't use the serial options. + if (scheduler.init(sched_inputs, 1, + scheduler_t::scheduler_options_t( + scheduler_t::MAP_TO_ANY_OUTPUT, scheduler_t::DEPENDENCY_IGNORE, + scheduler_t::SCHEDULER_DEFAULTS, + /*verbosity=*/4)) != scheduler_t::STATUS_SUCCESS) + assert(false); + int ordinal = 0; + auto *stream = scheduler.get_stream(0); + memref_t memref; + for (scheduler_t::stream_status_t status = stream->next_record(memref); + status != scheduler_t::STATUS_EOF; status = stream->next_record(memref)) { + assert(status == scheduler_t::STATUS_OK); + switch (ordinal) { + case 0: + assert(type_is_instr(memref.instr.type)); + assert(memref.instr.addr == 2); + break; + case 1: + assert(type_is_instr(memref.instr.type)); + assert(memref.instr.addr == 3); + break; + case 2: + assert(type_is_instr(memref.instr.type)); + assert(memref.instr.addr == 4); + break; + default: assert(ordinal == 3); assert(memref.exit.type == TRACE_TYPE_THREAD_EXIT); + } + ++ordinal; + } + assert(ordinal == 4); } static void @@ -622,7 +701,7 @@ test_regions_start() sched_inputs.emplace_back(std::move(readers)); sched_inputs[0].thread_modifiers.push_back(scheduler_t::input_thread_info_t(regions)); if (scheduler.init(sched_inputs, 1, - scheduler_t::make_scheduler_serial_options(/*verbosity=*/4)) != + scheduler_t::make_scheduler_serial_options(/*verbosity=*/5)) != scheduler_t::STATUS_SUCCESS) assert(false); int ordinal = 0; @@ -631,32 +710,29 @@ test_regions_start() for (scheduler_t::stream_status_t status = stream->next_record(memref); status != scheduler_t::STATUS_EOF; status = stream->next_record(memref)) { assert(status == scheduler_t::STATUS_OK); + // Because we skipped, even if not very far, we do not see the page marker. switch (ordinal) { case 0: - assert(memref.marker.type == TRACE_TYPE_MARKER); - assert(memref.marker.marker_type == TRACE_MARKER_TYPE_PAGE_SIZE); - break; - case 1: assert(memref.marker.type == TRACE_TYPE_MARKER); assert(memref.marker.marker_type == TRACE_MARKER_TYPE_TIMESTAMP); break; - case 2: + case 1: assert(memref.marker.type == TRACE_TYPE_MARKER); assert(memref.marker.marker_type == TRACE_MARKER_TYPE_CPU_ID); break; - case 3: + case 2: assert(type_is_instr(memref.instr.type)); assert(memref.instr.addr == 1); break; - case 4: + case 3: assert(type_is_instr(memref.instr.type)); assert(memref.instr.addr == 2); break; - default: assert(ordinal == 5); assert(memref.exit.type == TRACE_TYPE_THREAD_EXIT); + default: assert(ordinal == 4); assert(memref.exit.type == TRACE_TYPE_THREAD_EXIT); } ++ordinal; } - assert(ordinal == 6); + assert(ordinal == 5); } static void @@ -703,6 +779,7 @@ test_regions() { test_regions_timestamps(); test_regions_bare(); + test_regions_bare_no_marker(); test_regions_start(); test_regions_too_far(); }