From 2fd68cbb50e22a0dd2528de24c07979a6c4a33d4 Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Thu, 28 Nov 2024 11:39:55 +0100 Subject: [PATCH 1/3] Add dedicated test case for only reading partial frames Reading only part of the collections stored in a frame should not lead to leaked memory (see e.g. #500). The tests that are added in this commit are targetting that, specifically by running them with sanitizers enabled. --- tests/CTestCustom.cmake | 1 + tests/read_partial.h | 39 ++++++++++++++++++++++++++ tests/root_io/CMakeLists.txt | 11 +++++++- tests/root_io/read_partial_rntuple.cpp | 7 +++++ tests/root_io/read_partial_root.cpp | 7 +++++ tests/sio_io/CMakeLists.txt | 3 ++ tests/sio_io/read_partial_sio.cpp | 7 +++++ 7 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 tests/read_partial.h create mode 100644 tests/root_io/read_partial_rntuple.cpp create mode 100644 tests/root_io/read_partial_root.cpp create mode 100644 tests/sio_io/read_partial_sio.cpp diff --git a/tests/CTestCustom.cmake b/tests/CTestCustom.cmake index bce6fc5fd..102b6c2c0 100644 --- a/tests/CTestCustom.cmake +++ b/tests/CTestCustom.cmake @@ -18,6 +18,7 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ write_python_frame_root read_python_frame_root read_and_write_frame_root + read_partioal_root param_reading_rdataframe diff --git a/tests/read_partial.h b/tests/read_partial.h new file mode 100644 index 000000000..72fb9d27c --- /dev/null +++ b/tests/read_partial.h @@ -0,0 +1,39 @@ +#ifndef PODIO_TESTS_READ_PARTIAL_H // NOLINT(llvm-header-guard): folder structure not suitable +#define PODIO_TESTS_READ_PARTIAL_H // NOLINT(llvm-header-guard): folder structure not suitable + +#include "podio/Frame.h" + +#include +#include +#include +#include + +void read_partial_collections(const podio::Frame& event, const std::vector& collsToRead) { + for (const auto& name : collsToRead) { + event.get(name); + } +} + +/** + * This is a test case that will always work in normal builds and will only fail + * in builds with sanitizers enabled, where they will start to fail in case of + * e.g. memory leaks. + */ +template +int read_partial_frames(const std::string& filename) { + auto reader = ReaderT(); + try { + reader.openFile(filename); + } catch (const std::runtime_error& e) { + std::cerr << "File " << filename << " could not be opened. aborting." << std::endl; + return 1; + } + + for (auto i = 0u; i < reader.getEntries(podio::Category::Event); ++i) { + const auto event = podio::Frame(reader.readEntry(podio::Category::Event, i)); + read_partial_collections(event, {"mcparticles", "info", "hits", "clusters"}); + } + + return 0; +} +#endif // PODIO_TESTS_READ_PARTIAL_H diff --git a/tests/root_io/CMakeLists.txt b/tests/root_io/CMakeLists.txt index 9de085a4f..98ffb73dd 100644 --- a/tests/root_io/CMakeLists.txt +++ b/tests/root_io/CMakeLists.txt @@ -8,6 +8,7 @@ set(root_dependent_tests read_and_write_frame_root.cpp write_interface_root.cpp read_interface_root.cpp + read_partial_root.cpp ) if(ENABLE_RNTUPLE) set(root_dependent_tests @@ -17,6 +18,7 @@ if(ENABLE_RNTUPLE) read_python_frame_rntuple.cpp write_interface_rntuple.cpp read_interface_rntuple.cpp + read_partial_rntuple.cpp ) endif() if(ENABLE_DATASOURCE) @@ -39,13 +41,20 @@ set_tests_properties( read_frame_root read_frame_root_multiple read_and_write_frame_root + read_partial_root PROPERTIES DEPENDS write_frame_root ) if(ENABLE_RNTUPLE) - set_property(TEST read_rntuple PROPERTY DEPENDS write_rntuple) + set_tests_properties( + read_rntuple + read_partial_rntuple + + PROPERTIES + DEPENDS write_rntuple + ) set_property(TEST read_interface_rntuple PROPERTY DEPENDS write_interface_rntuple) endif() diff --git a/tests/root_io/read_partial_rntuple.cpp b/tests/root_io/read_partial_rntuple.cpp new file mode 100644 index 000000000..c4ae0b962 --- /dev/null +++ b/tests/root_io/read_partial_rntuple.cpp @@ -0,0 +1,7 @@ +#include "read_partial.h" + +#include "podio/RNTupleReader.h" + +int main() { + return read_partial_frames("example_rntuple.root"); +} diff --git a/tests/root_io/read_partial_root.cpp b/tests/root_io/read_partial_root.cpp new file mode 100644 index 000000000..c4d547a31 --- /dev/null +++ b/tests/root_io/read_partial_root.cpp @@ -0,0 +1,7 @@ +#include "read_partial.h" + +#include "podio/ROOTReader.h" + +int main() { + return read_partial_frames("example_frame.root"); +} diff --git a/tests/sio_io/CMakeLists.txt b/tests/sio_io/CMakeLists.txt index 7f53917fe..9ebaae49a 100644 --- a/tests/sio_io/CMakeLists.txt +++ b/tests/sio_io/CMakeLists.txt @@ -5,7 +5,9 @@ set(sio_dependent_tests read_python_frame_sio.cpp write_interface_sio.cpp read_interface_sio.cpp + read_partial_sio.cpp ) + set(sio_libs podio::podioSioIO podio::podioIO) foreach( sourcefile ${sio_dependent_tests} ) CREATE_PODIO_TEST(${sourcefile} "${sio_libs}") @@ -14,6 +16,7 @@ endforeach() set_tests_properties( read_frame_sio read_and_write_frame_sio + read_partial_sio PROPERTIES DEPENDS diff --git a/tests/sio_io/read_partial_sio.cpp b/tests/sio_io/read_partial_sio.cpp new file mode 100644 index 000000000..f2b027421 --- /dev/null +++ b/tests/sio_io/read_partial_sio.cpp @@ -0,0 +1,7 @@ +#include "read_partial.h" + +#include "podio/SIOReader.h" + +int main() { + return read_partial_frames("example_frame.sio"); +} From 2246d8b24d051e8a5a7e2ca1bc1a7f5f387b235e Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Thu, 28 Nov 2024 13:52:17 +0100 Subject: [PATCH 2/3] Don't run ROOT based tests for UBSan on clang --- tests/CTestCustom.cmake | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/CTestCustom.cmake b/tests/CTestCustom.cmake index 102b6c2c0..8904af293 100644 --- a/tests/CTestCustom.cmake +++ b/tests/CTestCustom.cmake @@ -18,7 +18,7 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ write_python_frame_root read_python_frame_root read_and_write_frame_root - read_partioal_root + read_partial_root param_reading_rdataframe @@ -108,6 +108,7 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ read_rntuple write_interface_rntuple read_interface_rntuple + read_partial_rntuple ) endif() From 77ea46fdc9931924a5eeeaec0d00396de356644d Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Thu, 28 Nov 2024 14:02:52 +0100 Subject: [PATCH 3/3] Suppress Thread sanitizer warnings from ROOT --- cmake/podioTest.cmake | 1 + tests/root_io/thread_sanitizer_suppressions.txt | 2 ++ 2 files changed, 3 insertions(+) create mode 100644 tests/root_io/thread_sanitizer_suppressions.txt diff --git a/cmake/podioTest.cmake b/cmake/podioTest.cmake index 4c4d8f24d..ffc5ee3d3 100644 --- a/cmake/podioTest.cmake +++ b/cmake/podioTest.cmake @@ -15,6 +15,7 @@ function(PODIO_SET_TEST_ENV test) ENABLE_SIO=${ENABLE_SIO} PODIO_BUILD_BASE=${PROJECT_BINARY_DIR} LSAN_OPTIONS=suppressions=${PROJECT_SOURCE_DIR}/tests/root_io/leak_sanitizer_suppressions.txt + TSAN_OPTIONS=suppressions=${PROJECT_SOURCE_DIR}/tests/root_io/thread_sanitizer_suppressions.txt ) set_property(TEST ${test} PROPERTY ENVIRONMENT "${test_environment}" diff --git a/tests/root_io/thread_sanitizer_suppressions.txt b/tests/root_io/thread_sanitizer_suppressions.txt new file mode 100644 index 000000000..8bf74c401 --- /dev/null +++ b/tests/root_io/thread_sanitizer_suppressions.txt @@ -0,0 +1,2 @@ +# Ignore race conditions in libROOTNTuple +race:ROOTNTuple