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

@@ -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");

clients/drcachesim/reader/reader.h

@@ -124,6 +124,8 @@ class reader_t : public std::iterator<std::input_iterator_tag, memref_t>,
     // 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);
 

clients/drcachesim/scheduler/scheduler.cpp

@@ -1269,6 +1269,11 @@ scheduler_tmpl_t<RecordType, ReaderType>::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).

clients/drcachesim/scheduler/scheduler.h

@@ -200,9 +200,10 @@ template <typename RecordType, typename ReaderType> 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<range_t> regions_of_interest;
     };

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<scheduler_t::range_t> 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<scheduler_t::input_workload_t> 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<trace_entry_t> 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<scheduler_t::input_reader_t> readers;
+    readers.emplace_back(std::unique_ptr<mock_reader_t>(new mock_reader_t(memrefs)),
+                         std::unique_ptr<mock_reader_t>(new mock_reader_t()), 1);
+
+    std::vector<scheduler_t::range_t> regions;
+    // Instr counts are 1-based.
+    regions.emplace_back(2, 0);
+
+    scheduler_t scheduler;
+    std::vector<scheduler_t::input_workload_t> 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();
 }