From 4174f4891610b5d77c2b066688016e2870e03da2 Mon Sep 17 00:00:00 2001 From: Zuby Afzal Date: Sun, 30 Mar 2025 21:06:52 -0700 Subject: [PATCH 1/5] Refactor and add ET_EXPECT_DEATH tests to LogDelegateIntermediateOutput --- devtools/etdump/tests/etdump_test.cpp | 65 +++++++++++++++++++-------- 1 file changed, 46 insertions(+), 19 deletions(-) diff --git a/devtools/etdump/tests/etdump_test.cpp b/devtools/etdump/tests/etdump_test.cpp index 8e93a547074..d9e46fa89e6 100644 --- a/devtools/etdump/tests/etdump_test.cpp +++ b/devtools/etdump/tests/etdump_test.cpp @@ -497,36 +497,60 @@ TEST_F(ProfilerETDumpTest, VerifyData) { } } +// Helper to assert that logging without a data sink triggers ET_EXPECT_DEATH +static void et_expect_death_log_delegate( + ETDumpGen* gen, + TensorFactory& tf) { + ET_EXPECT_DEATH( + gen->log_intermediate_output_delegate( + "test_event_tensor", + static_cast(-1), + tf.ones({3, 2})), + "Must set data sink before writing tensor-like data"); +} + TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { + const size_t debug_buf_size = 2048; + const size_t etdump_buf_size = 512 * 1024; + for (size_t i = 0; i < 2; i++) { for (size_t j = 0; j < 3; j++) { - void* ptr = malloc(2048); - Span buffer((uint8_t*)ptr, 2048); + uint8_t* buf = nullptr; + void* ptr = malloc(debug_buf_size); + Span buffer((uint8_t*)ptr, debug_buf_size); - auto buffer_data_sink = BufferDataSink::create(ptr, 2048); + auto buffer_data_sink = BufferDataSink::create(ptr, debug_buf_size); auto file_data_sink = FileDataSink::create(dump_file_path.c_str()); etdump_gen[i]->create_event_block("test_block"); TensorFactory tf; - // using span to record debug data if (j == 0) { - // TODO(gasoonjia): add similar ET_EXPECT_DEATH on BufferDataSink branch - ET_EXPECT_DEATH( - etdump_gen[i]->log_intermediate_output_delegate( - "test_event_tensor", - static_cast(-1), - tf.ones({3, 2})), - "Must set data sink before writing tensor-like data"); + // Use span to record debug data + et_expect_death_log_delegate(etdump_gen[i], tf); etdump_gen[i]->set_debug_buffer(buffer); - } - // using buffer data sink to record debug data - else if (j == 1) { - etdump_gen[i]->set_data_sink(&buffer_data_sink.get()); - } - // using file data sink to record debug data - else { - etdump_gen[i]->set_data_sink(&file_data_sink.get()); + } else { + buf = (uint8_t*)malloc(etdump_buf_size * sizeof(uint8_t)); + + // Wrap buffer in span for ETDumGen constructor + Span span_buf(buf, etdump_buf_size); + + // Reset ETDumpGen to correctly trigger ET_EXPECT_DEATH + delete etdump_gen[i]; + + // Recreate ETDumpGen; use span buffer only for etdump_gen[1] + etdump_gen[i] = (i == 0) ? new ETDumpGen() : new ETDumpGen(span_buf); + + etdump_gen[i]->create_event_block("test_block"); + et_expect_death_log_delegate(etdump_gen[i], tf); + + if (j == 1) { + // Use buffer data sink to record debug data + etdump_gen[i]->set_data_sink(&buffer_data_sink.get()); + } else { + // Use file data sink to record debug data + etdump_gen[i]->set_data_sink(&file_data_sink.get()); + } } // Log a tensor @@ -568,6 +592,9 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { if (!etdump_gen[i]->is_static_etdump()) { free(result.buf); } + if (buf) { + free(buf); + } } } } From 9c6097a9d3b15966a0ad89eaea0ff499827f6f0f Mon Sep 17 00:00:00 2001 From: Zuby Afzal Date: Mon, 31 Mar 2025 05:41:05 -0700 Subject: [PATCH 2/5] Reuse class-level buf in LogDelegateIntermediateOutput test instead of reallocating --- devtools/etdump/tests/etdump_test.cpp | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/devtools/etdump/tests/etdump_test.cpp b/devtools/etdump/tests/etdump_test.cpp index d9e46fa89e6..5bce4a35161 100644 --- a/devtools/etdump/tests/etdump_test.cpp +++ b/devtools/etdump/tests/etdump_test.cpp @@ -512,10 +512,11 @@ static void et_expect_death_log_delegate( TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { const size_t debug_buf_size = 2048; const size_t etdump_buf_size = 512 * 1024; + ASSERT_NE(this->buf, nullptr); + Span span_buf = Span(this->buf, etdump_buf_size); for (size_t i = 0; i < 2; i++) { for (size_t j = 0; j < 3; j++) { - uint8_t* buf = nullptr; void* ptr = malloc(debug_buf_size); Span buffer((uint8_t*)ptr, debug_buf_size); @@ -526,21 +527,16 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { TensorFactory tf; if (j == 0) { - // Use span to record debug data et_expect_death_log_delegate(etdump_gen[i], tf); + + // Use span to record debug data etdump_gen[i]->set_debug_buffer(buffer); } else { - buf = (uint8_t*)malloc(etdump_buf_size * sizeof(uint8_t)); - - // Wrap buffer in span for ETDumGen constructor - Span span_buf(buf, etdump_buf_size); - // Reset ETDumpGen to correctly trigger ET_EXPECT_DEATH delete etdump_gen[i]; // Recreate ETDumpGen; use span buffer only for etdump_gen[1] etdump_gen[i] = (i == 0) ? new ETDumpGen() : new ETDumpGen(span_buf); - etdump_gen[i]->create_event_block("test_block"); et_expect_death_log_delegate(etdump_gen[i], tf); @@ -592,9 +588,6 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { if (!etdump_gen[i]->is_static_etdump()) { free(result.buf); } - if (buf) { - free(buf); - } } } } From 8dec5b7e94562867a9af8379e11bd6c297655dd8 Mon Sep 17 00:00:00 2001 From: Zuby Afzal Date: Mon, 31 Mar 2025 19:57:04 -0700 Subject: [PATCH 3/5] Add ET_EXPECT_DEATH tests to DebugEvent test --- devtools/etdump/tests/etdump_test.cpp | 61 ++++++++++++++++++--------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git a/devtools/etdump/tests/etdump_test.cpp b/devtools/etdump/tests/etdump_test.cpp index 5bce4a35161..9ba84ae53cf 100644 --- a/devtools/etdump/tests/etdump_test.cpp +++ b/devtools/etdump/tests/etdump_test.cpp @@ -186,11 +186,16 @@ TEST_F(ProfilerETDumpTest, AllocationEvents) { } TEST_F(ProfilerETDumpTest, DebugEvent) { + const size_t debug_buf_size = 2048; + const size_t etdump_buf_size = 512 * 1024; + ASSERT_NE(this->buf, nullptr); + Span span_buf = Span(this->buf, etdump_buf_size); + for (size_t i = 0; i < 2; i++) { for (size_t j = 0; j < 3; j++) { etdump_gen[i]->create_event_block("test_block"); - void* ptr = malloc(2048); + void* ptr = malloc(debug_buf_size); EValue evalue_int((int64_t)5); etdump_gen[i]->log_evalue(evalue_int); @@ -206,24 +211,37 @@ TEST_F(ProfilerETDumpTest, DebugEvent) { TensorFactory tf; EValue evalue_tensor(tf.ones({3, 2})); - // using span to record debug data - Span buffer((uint8_t*)ptr, 2048); - auto buffer_data_sink = BufferDataSink::create(ptr, 2048); + // Create span to record debug data + Span buffer((uint8_t*)ptr, debug_buf_size); + auto buffer_data_sink = BufferDataSink::create(ptr, debug_buf_size); auto file_data_sink = FileDataSink::create(dump_file_path.c_str()); if (j == 0) { ET_EXPECT_DEATH( etdump_gen[i]->log_evalue(evalue_tensor), "Must set data sink before writing tensor-like data"); + + // Set debug buffer with span etdump_gen[i]->set_debug_buffer(buffer); - } - // using buffer data sink to record debug data - else if (j == 1) { - etdump_gen[i]->set_data_sink(&buffer_data_sink.get()); - } - // using file data sink to record debug data - else { - etdump_gen[i]->set_data_sink(&file_data_sink.get()); + } else { + // Reset ETDumpGen to trigger ET_EXPECT_DEATH before setting data sink + delete etdump_gen[i]; + + // Recreate ETDumpGen; set span buffer only for etdump_gen[1] + etdump_gen[i] = (i == 0) ? new ETDumpGen() : new ETDumpGen(span_buf); + etdump_gen[i]->create_event_block("test_block"); + + ET_EXPECT_DEATH( + etdump_gen[i]->log_evalue(evalue_tensor), + "Must set data sink before writing tensor-like data"); + + if (j == 1) { + // Set buffer data sink + etdump_gen[i]->set_data_sink(&buffer_data_sink.get()); + } else { + // Set file data sink + etdump_gen[i]->set_data_sink(&file_data_sink.get()); + } } etdump_gen[i]->log_evalue(evalue_tensor); @@ -497,8 +515,8 @@ TEST_F(ProfilerETDumpTest, VerifyData) { } } -// Helper to assert that logging without a data sink triggers ET_EXPECT_DEATH -static void et_expect_death_log_delegate( +// Triggers ET_EXPECT_DEATH if log_intermediate_output_delegate has no data sink +static void expect_log_intermediate_delegate_death( ETDumpGen* gen, TensorFactory& tf) { ET_EXPECT_DEATH( @@ -527,24 +545,25 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { TensorFactory tf; if (j == 0) { - et_expect_death_log_delegate(etdump_gen[i], tf); + expect_log_intermediate_delegate_death(etdump_gen[i], tf); - // Use span to record debug data + // Set debug buffer with span etdump_gen[i]->set_debug_buffer(buffer); } else { - // Reset ETDumpGen to correctly trigger ET_EXPECT_DEATH + // Reset ETDumpGen to trigger ET_EXPECT_DEATH before setting data sink delete etdump_gen[i]; - // Recreate ETDumpGen; use span buffer only for etdump_gen[1] + // Recreate ETDumpGen; set span buffer only for etdump_gen[1] etdump_gen[i] = (i == 0) ? new ETDumpGen() : new ETDumpGen(span_buf); etdump_gen[i]->create_event_block("test_block"); - et_expect_death_log_delegate(etdump_gen[i], tf); + + expect_log_intermediate_delegate_death(etdump_gen[i], tf); if (j == 1) { - // Use buffer data sink to record debug data + // Set buffer data sink etdump_gen[i]->set_data_sink(&buffer_data_sink.get()); } else { - // Use file data sink to record debug data + // Set file data sink etdump_gen[i]->set_data_sink(&file_data_sink.get()); } } From d49509edd1ea5732576c4d251126fbb0c109e8f2 Mon Sep 17 00:00:00 2001 From: Zuby Afzal Date: Mon, 31 Mar 2025 20:42:41 -0700 Subject: [PATCH 4/5] Move create_event_block call under j == 0 condition --- devtools/etdump/tests/etdump_test.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/devtools/etdump/tests/etdump_test.cpp b/devtools/etdump/tests/etdump_test.cpp index 9ba84ae53cf..42ede636b1b 100644 --- a/devtools/etdump/tests/etdump_test.cpp +++ b/devtools/etdump/tests/etdump_test.cpp @@ -541,10 +541,10 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { auto buffer_data_sink = BufferDataSink::create(ptr, debug_buf_size); auto file_data_sink = FileDataSink::create(dump_file_path.c_str()); - etdump_gen[i]->create_event_block("test_block"); TensorFactory tf; if (j == 0) { + etdump_gen[i]->create_event_block("test_block"); expect_log_intermediate_delegate_death(etdump_gen[i], tf); // Set debug buffer with span @@ -556,7 +556,6 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { // Recreate ETDumpGen; set span buffer only for etdump_gen[1] etdump_gen[i] = (i == 0) ? new ETDumpGen() : new ETDumpGen(span_buf); etdump_gen[i]->create_event_block("test_block"); - expect_log_intermediate_delegate_death(etdump_gen[i], tf); if (j == 1) { From 86024d858ced508c8164b3bc71e36d0a7a71a312 Mon Sep 17 00:00:00 2001 From: Zuby Afzal Date: Mon, 31 Mar 2025 20:53:20 -0700 Subject: [PATCH 5/5] Make expect_log_intermediate_delegate_death a protected member function --- devtools/etdump/tests/etdump_test.cpp | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/devtools/etdump/tests/etdump_test.cpp b/devtools/etdump/tests/etdump_test.cpp index 42ede636b1b..98f77d27403 100644 --- a/devtools/etdump/tests/etdump_test.cpp +++ b/devtools/etdump/tests/etdump_test.cpp @@ -61,6 +61,19 @@ class ProfilerETDumpTest : public ::testing::Test { free(buf); } + // Triggers ET_EXPECT_DEATH if log_intermediate_output_delegate has no + // data sink + void expect_log_intermediate_delegate_death( + ETDumpGen* gen, + TensorFactory& tf) { + ET_EXPECT_DEATH( + gen->log_intermediate_output_delegate( + "test_event_tensor", + static_cast(-1), + tf.ones({3, 2})), + "Must set data sink before writing tensor-like data"); + } + ETDumpGen* etdump_gen[2]; uint8_t* buf = nullptr; std::unique_ptr temp_file; @@ -515,18 +528,6 @@ TEST_F(ProfilerETDumpTest, VerifyData) { } } -// Triggers ET_EXPECT_DEATH if log_intermediate_output_delegate has no data sink -static void expect_log_intermediate_delegate_death( - ETDumpGen* gen, - TensorFactory& tf) { - ET_EXPECT_DEATH( - gen->log_intermediate_output_delegate( - "test_event_tensor", - static_cast(-1), - tf.ones({3, 2})), - "Must set data sink before writing tensor-like data"); -} - TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { const size_t debug_buf_size = 2048; const size_t etdump_buf_size = 512 * 1024;