From b1263d9dcd25c372efbd00476ae50dc0fb3c1ad2 Mon Sep 17 00:00:00 2001 From: root Date: Mon, 17 Mar 2025 20:07:50 +0000 Subject: [PATCH 1/2] test(etdump): Add FileDataSink test coverage Summary: - Extend test loops to cover FileDataSink scenarios (j=2 case) - Add temporary file cleanup in TearDown - Include file_data_sink.h, stdio.h and fstream in test sources - Update CMakeLists.txt to ensure proper linking Fixes #9165 --- devtools/CMakeLists.txt | 4 +- devtools/etdump/tests/etdump_test.cpp | 65 +++++++++++++++++++++------ 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/devtools/CMakeLists.txt b/devtools/CMakeLists.txt index 3970f74fe5..27d23b69b0 100644 --- a/devtools/CMakeLists.txt +++ b/devtools/CMakeLists.txt @@ -186,6 +186,8 @@ add_library( ${CMAKE_CURRENT_SOURCE_DIR}/etdump/emitter.cpp ${CMAKE_CURRENT_SOURCE_DIR}/etdump/data_sinks/buffer_data_sink.cpp ${CMAKE_CURRENT_SOURCE_DIR}/etdump/data_sinks/buffer_data_sink.h + ${CMAKE_CURRENT_SOURCE_DIR}/etdump/data_sinks/file_data_sink.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/etdump/data_sinks/file_data_sink.h ) target_link_libraries( @@ -233,5 +235,5 @@ install( if(BUILD_TESTING) # TODO: This is currently not working! - # add_subdirectory(etdump/tests) + add_subdirectory(etdump/tests) endif() diff --git a/devtools/etdump/tests/etdump_test.cpp b/devtools/etdump/tests/etdump_test.cpp index 4a732335d3..55a4db3a90 100644 --- a/devtools/etdump/tests/etdump_test.cpp +++ b/devtools/etdump/tests/etdump_test.cpp @@ -8,8 +8,10 @@ #include #include +#include #include +#include #include #include #include @@ -19,6 +21,7 @@ #include #include #include +#include using ::executorch::aten::ScalarType; using ::executorch::aten::Tensor; @@ -36,6 +39,7 @@ using ::executorch::runtime::Tag; using ::executorch::runtime::testing::TensorFactory; using ::executorch::etdump::BufferDataSink; +using ::executorch::etdump::FileDataSink; class ProfilerETDumpTest : public ::testing::Test { protected: @@ -45,16 +49,23 @@ class ProfilerETDumpTest : public ::testing::Test { const size_t buf_size = 512 * 1024; buf = (uint8_t*)malloc(buf_size * sizeof(uint8_t)); etdump_gen[1] = new ETDumpGen(Span(buf, buf_size)); + + std::array dummy_name; + dummy_name[L_tmpnam-1] = '\0'; + dump_file_path = std::string(dummy_name.data()) + "-dump"; } void TearDown() override { delete etdump_gen[0]; delete etdump_gen[1]; free(buf); + + std::remove(dump_file_path.c_str()); } ETDumpGen* etdump_gen[2]; uint8_t* buf = nullptr; + std::string dump_file_path; }; TEST_F(ProfilerETDumpTest, SingleProfileEvent) { @@ -177,7 +188,7 @@ TEST_F(ProfilerETDumpTest, AllocationEvents) { TEST_F(ProfilerETDumpTest, DebugEvent) { for (size_t i = 0; i < 2; i++) { - for (size_t j = 0; j < 2; j++) { + for (size_t j = 0; j < 3; j++) { etdump_gen[i]->create_event_block("test_block"); void* ptr = malloc(2048); @@ -199,16 +210,22 @@ TEST_F(ProfilerETDumpTest, DebugEvent) { // using span to record debug data Span buffer((uint8_t*)ptr, 2048); auto buffer_data_sink = BufferDataSink::create(ptr, 2048); + 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"); etdump_gen[i]->set_debug_buffer(buffer); } - // using data sink to record debug data - else { + // 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()); + } etdump_gen[i]->log_evalue(evalue_tensor); etdump_gen[i]->log_evalue( @@ -221,7 +238,7 @@ TEST_F(ProfilerETDumpTest, DebugEvent) { TEST_F(ProfilerETDumpTest, DebugEventTensorList) { for (size_t i = 0; i < 2; i++) { - for (size_t j = 0; j < 2; j++) { + for (size_t j = 0; j < 3; j++) { TensorFactory tf; executorch::aten::Tensor storage[2] = {tf.ones({3, 2}), tf.ones({3, 2})}; EValue evalue_1(storage[0]); @@ -238,15 +255,20 @@ TEST_F(ProfilerETDumpTest, DebugEventTensorList) { Span buffer((uint8_t*)ptr, 2048); auto buffer_data_sink = BufferDataSink::create(ptr, 2048); + auto file_data_sink = FileDataSink::create(dump_file_path.c_str()); // using span to record debug data if (j == 0) { etdump_gen[i]->set_debug_buffer(buffer); } - // using data sink to record debug data - else { + // 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 dats + else { + etdump_gen[i]->set_data_sink(&file_data_sink.get()); + } etdump_gen[i]->log_evalue(evalue); @@ -267,15 +289,20 @@ TEST_F(ProfilerETDumpTest, VerifyLogging) { Span buffer((uint8_t*)ptr, 2048); auto buffer_data_sink = BufferDataSink::create(ptr, 2048); + auto file_data_sink = FileDataSink::create(dump_file_path.c_str()); // using span to record debug data if (j == 0) { etdump_gen[i]->set_debug_buffer(buffer); } - // using data sink to record debug data - else { + // using buffer data sink to record debug data + else if (j == 1) { etdump_gen[i]->set_data_sink(&buffer_data_sink.get()); } + // using buffer data sink to record debug data + else { + etdump_gen[i]->set_data_sink(&file_data_sink.get()); + } etdump_gen[i]->log_evalue(evalue); etdump_gen[i]->log_evalue(evalue, LoggedEValueType::kProgramOutput); @@ -473,11 +500,12 @@ TEST_F(ProfilerETDumpTest, VerifyData) { TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { for (size_t i = 0; i < 2; i++) { - for (size_t j = 0; j < 2; j++) { + for (size_t j = 0; j < 3; j++) { void* ptr = malloc(2048); Span buffer((uint8_t*)ptr, 2048); auto buffer_data_sink = BufferDataSink::create(ptr, 2048); + auto file_data_sink = FileDataSink::create(dump_file_path.c_str()); etdump_gen[i]->create_event_block("test_block"); TensorFactory tf; @@ -493,10 +521,14 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { "Must set data sink before writing tensor-like data"); etdump_gen[i]->set_debug_buffer(buffer); } - // using data sink to record debug data - else { + // 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()); + } // Log a tensor etdump_gen[i]->log_intermediate_output_delegate( @@ -546,22 +578,27 @@ TEST_F(ProfilerETDumpTest, VerifyDelegateIntermediateLogging) { EValue evalue(tf.ones({3, 2})); for (size_t i = 0; i < 2; i++) { - for (size_t j = 0; j < 2; j++) { + for (size_t j = 0; j < 3; j++) { etdump_gen[i]->create_event_block("test_block"); void* ptr = malloc(2048); Span buffer((uint8_t*)ptr, 2048); ; auto buffer_data_sink = BufferDataSink::create(ptr, 2048); + auto file_data_sink = FileDataSink::create(dump_file_path.c_str()); // using span to record debug data if (j == 0) { etdump_gen[i]->set_debug_buffer(buffer); } - // using data sink to record debug data - else { + // 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()); + } // Event 0 etdump_gen[i]->log_intermediate_output_delegate( From 8e8c9786550bf6251e31e734c68cedec22fe0fb9 Mon Sep 17 00:00:00 2001 From: megankuo Date: Tue, 18 Mar 2025 19:23:14 +0000 Subject: [PATCH 2/2] fix(etdump): replace insecure tmpnam with TempFile utility - Migrate from tmpnam to executorch::extension::testing::TempFile for secure temporary file handling - Add missing namespace qualification for TempFile class - Use std::unique_ptr for proper RAII management - Remove manual file deletion since TempFile handles cleanup - Remove building sdk_etdump_test in devtools/CMakeLists.txt --- devtools/CMakeLists.txt | 2 +- devtools/etdump/tests/etdump_test.cpp | 17 ++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/devtools/CMakeLists.txt b/devtools/CMakeLists.txt index 27d23b69b0..9dd38d3678 100644 --- a/devtools/CMakeLists.txt +++ b/devtools/CMakeLists.txt @@ -235,5 +235,5 @@ install( if(BUILD_TESTING) # TODO: This is currently not working! - add_subdirectory(etdump/tests) + # add_subdirectory(etdump/tests) endif() diff --git a/devtools/etdump/tests/etdump_test.cpp b/devtools/etdump/tests/etdump_test.cpp index 55a4db3a90..8e93a54707 100644 --- a/devtools/etdump/tests/etdump_test.cpp +++ b/devtools/etdump/tests/etdump_test.cpp @@ -8,25 +8,26 @@ #include #include -#include +#include #include #include #include #include #include +#include #include #include #include #include #include #include -#include using ::executorch::aten::ScalarType; using ::executorch::aten::Tensor; using ::executorch::etdump::ETDumpGen; using ::executorch::etdump::ETDumpResult; +using ::executorch::extension::testing::TempFile; using ::executorch::runtime::AllocatorID; using ::executorch::runtime::ArrayRef; using ::executorch::runtime::BoxedEvalueList; @@ -50,21 +51,19 @@ class ProfilerETDumpTest : public ::testing::Test { buf = (uint8_t*)malloc(buf_size * sizeof(uint8_t)); etdump_gen[1] = new ETDumpGen(Span(buf, buf_size)); - std::array dummy_name; - dummy_name[L_tmpnam-1] = '\0'; - dump_file_path = std::string(dummy_name.data()) + "-dump"; + temp_file = std::make_unique(std::string()); + dump_file_path = temp_file->path(); } void TearDown() override { delete etdump_gen[0]; delete etdump_gen[1]; free(buf); - - std::remove(dump_file_path.c_str()); } ETDumpGen* etdump_gen[2]; uint8_t* buf = nullptr; + std::unique_ptr temp_file; std::string dump_file_path; }; @@ -267,7 +266,7 @@ TEST_F(ProfilerETDumpTest, DebugEventTensorList) { } // using file data sink to record debug dats else { - etdump_gen[i]->set_data_sink(&file_data_sink.get()); + etdump_gen[i]->set_data_sink(&file_data_sink.get()); } etdump_gen[i]->log_evalue(evalue); @@ -301,7 +300,7 @@ TEST_F(ProfilerETDumpTest, VerifyLogging) { } // using buffer data sink to record debug data else { - etdump_gen[i]->set_data_sink(&file_data_sink.get()); + etdump_gen[i]->set_data_sink(&file_data_sink.get()); } etdump_gen[i]->log_evalue(evalue);