From 9f38c51fbde844e39788fc0c732648d7832071ea Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Mon, 18 Sep 2023 16:00:42 -0400 Subject: [PATCH 1/4] i#5329: Fix tool.drcachesim.scattergather test failure. Avoids going over the atomic write size while waiting for enough entries to write to IPC pipe. If we find we went over, we just write till the last ok-to-split ref. Fixes: #5329 --- clients/drcachesim/tracer/output.cpp | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/clients/drcachesim/tracer/output.cpp b/clients/drcachesim/tracer/output.cpp index 99472c41f02..ca1eac392f9 100644 --- a/clients/drcachesim/tracer/output.cpp +++ b/clients/drcachesim/tracer/output.cpp @@ -731,6 +731,7 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr byte *pipe_end = pipe_start; if (!op_offline.get_value()) { byte *post_header = buf_base + header_size; + byte *last_ok_to_split_ref = nullptr; // Pipe split headers are just the tid. header_size = instru->sizeof_entry(); for (byte *mem_ref = post_header; mem_ref < buf_ptr; @@ -752,8 +753,23 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr DR_ASSERT(is_ok_to_split_before( instru->get_entry_type(pipe_start + header_size), instru->get_entry_size(pipe_start + header_size))); - pipe_start = atomic_pipe_write(drcontext, pipe_start, pipe_end, - get_local_window(data)); + // Check if we went over the edge waiting for enough entries to + // write. If we did, we simply write till the last ok-to-split ref. + if (pipe_end - pipe_start > ipc_pipe.get_atomic_write_size()) { + // If the followin assert triggers, we found too many entries + // without an ok-to-split point. + DR_ASSERT(last_ok_to_split_ref != nullptr); + pipe_start = + atomic_pipe_write(drcontext, pipe_start, last_ok_to_split_ref, + get_local_window(data)); + last_ok_to_split_ref = pipe_end; + } else { + pipe_start = atomic_pipe_write(drcontext, pipe_start, pipe_end, + get_local_window(data)); + last_ok_to_split_ref = nullptr; + } + } else { + last_ok_to_split_ref = pipe_end; } } } From 629b4d7430a619033c552e1fcbf39e9fe776f53d Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Mon, 18 Sep 2023 16:17:49 -0400 Subject: [PATCH 2/4] Remove redundant var and simplify logic. --- clients/drcachesim/tracer/output.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/clients/drcachesim/tracer/output.cpp b/clients/drcachesim/tracer/output.cpp index ca1eac392f9..c4e6473fc26 100644 --- a/clients/drcachesim/tracer/output.cpp +++ b/clients/drcachesim/tracer/output.cpp @@ -728,7 +728,6 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr size_t header_size) { byte *pipe_start = buf_base; - byte *pipe_end = pipe_start; if (!op_offline.get_value()) { byte *post_header = buf_base + header_size; byte *last_ok_to_split_ref = nullptr; @@ -744,7 +743,6 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr // it or one instr after. if (is_ok_to_split_before(instru->get_entry_type(mem_ref), instru->get_entry_size(mem_ref))) { - pipe_end = mem_ref; // We check the end of this entry + the max # of delay entries to // avoid splitting an instr from its subsequent bundle entry. // An alternative is to have the reader use per-thread state. @@ -755,21 +753,21 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr instru->get_entry_size(pipe_start + header_size))); // Check if we went over the edge waiting for enough entries to // write. If we did, we simply write till the last ok-to-split ref. - if (pipe_end - pipe_start > ipc_pipe.get_atomic_write_size()) { + if (mem_ref - pipe_start > ipc_pipe.get_atomic_write_size()) { // If the followin assert triggers, we found too many entries // without an ok-to-split point. DR_ASSERT(last_ok_to_split_ref != nullptr); pipe_start = atomic_pipe_write(drcontext, pipe_start, last_ok_to_split_ref, get_local_window(data)); - last_ok_to_split_ref = pipe_end; + last_ok_to_split_ref = mem_ref; } else { - pipe_start = atomic_pipe_write(drcontext, pipe_start, pipe_end, + pipe_start = atomic_pipe_write(drcontext, pipe_start, mem_ref, get_local_window(data)); last_ok_to_split_ref = nullptr; } } else { - last_ok_to_split_ref = pipe_end; + last_ok_to_split_ref = mem_ref; } } } @@ -783,7 +781,10 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr DR_ASSERT( is_ok_to_split_before(instru->get_entry_type(pipe_start + header_size), instru->get_entry_size(pipe_start + header_size))); - pipe_start = atomic_pipe_write(drcontext, pipe_start, pipe_end, + // If the followin assert triggers, we found too many entries without an + // ok-to-split point. + DR_ASSERT(last_ok_to_split_ref != nullptr); + pipe_start = atomic_pipe_write(drcontext, pipe_start, last_ok_to_split_ref, get_local_window(data)); } if ((buf_ptr - pipe_start) > (ssize_t)buf_hdr_slots_size) { From b304fc441ecb49858129b3f2d08d45a760c2bfc9 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Mon, 18 Sep 2023 16:28:36 -0400 Subject: [PATCH 3/4] Fix comment. --- clients/drcachesim/tracer/output.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clients/drcachesim/tracer/output.cpp b/clients/drcachesim/tracer/output.cpp index c4e6473fc26..f88d17939b3 100644 --- a/clients/drcachesim/tracer/output.cpp +++ b/clients/drcachesim/tracer/output.cpp @@ -754,7 +754,7 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr // Check if we went over the edge waiting for enough entries to // write. If we did, we simply write till the last ok-to-split ref. if (mem_ref - pipe_start > ipc_pipe.get_atomic_write_size()) { - // If the followin assert triggers, we found too many entries + // If the following assert triggers, we found too many entries // without an ok-to-split point. DR_ASSERT(last_ok_to_split_ref != nullptr); pipe_start = @@ -781,7 +781,7 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr DR_ASSERT( is_ok_to_split_before(instru->get_entry_type(pipe_start + header_size), instru->get_entry_size(pipe_start + header_size))); - // If the followin assert triggers, we found too many entries without an + // If the following assert triggers, we found too many entries without an // ok-to-split point. DR_ASSERT(last_ok_to_split_ref != nullptr); pipe_start = atomic_pipe_write(drcontext, pipe_start, last_ok_to_split_ref, From 4bfdb703dea040de7a1741cd8e87d7d15f3da501 Mon Sep 17 00:00:00 2001 From: Abhinav Anil Sharma Date: Mon, 18 Sep 2023 22:08:13 -0400 Subject: [PATCH 4/4] Use DR_ASSERT_MSG instead of DR_ASSERT --- clients/drcachesim/tracer/output.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/clients/drcachesim/tracer/output.cpp b/clients/drcachesim/tracer/output.cpp index f88d17939b3..6f28e14e0f8 100644 --- a/clients/drcachesim/tracer/output.cpp +++ b/clients/drcachesim/tracer/output.cpp @@ -754,9 +754,9 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr // Check if we went over the edge waiting for enough entries to // write. If we did, we simply write till the last ok-to-split ref. if (mem_ref - pipe_start > ipc_pipe.get_atomic_write_size()) { - // If the following assert triggers, we found too many entries - // without an ok-to-split point. - DR_ASSERT(last_ok_to_split_ref != nullptr); + DR_ASSERT_MSG( + last_ok_to_split_ref != nullptr, + "Found too many entries without an ok-to-split point"); pipe_start = atomic_pipe_write(drcontext, pipe_start, last_ok_to_split_ref, get_local_window(data)); @@ -781,9 +781,8 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr DR_ASSERT( is_ok_to_split_before(instru->get_entry_type(pipe_start + header_size), instru->get_entry_size(pipe_start + header_size))); - // If the following assert triggers, we found too many entries without an - // ok-to-split point. - DR_ASSERT(last_ok_to_split_ref != nullptr); + DR_ASSERT_MSG(last_ok_to_split_ref != nullptr, + "Found too many entries without an ok-to-split point"); pipe_start = atomic_pipe_write(drcontext, pipe_start, last_ok_to_split_ref, get_local_window(data)); }