Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix minor memory leaks and enable more tests in CI with sanitizers #695

Merged
merged 8 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/sanitizers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ jobs:
-DUSE_EXTERNAL_CATCH2=OFF \
-DENABLE_SIO=ON \
-DENABLE_JULIA=ON \
-DENABLE_RNTUPLE=ON \
-G Ninja ..
echo "::endgroup::"
echo "::group::Build"
Expand Down
1 change: 1 addition & 0 deletions cmake/podioTest.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ function(PODIO_SET_TEST_ENV test)
PODIO_BASE=${PROJECT_SOURCE_DIR}
ENABLE_SIO=${ENABLE_SIO}
PODIO_BUILD_BASE=${PROJECT_BINARY_DIR}
LSAN_OPTIONS=suppressions=${PROJECT_SOURCE_DIR}/tests/root_io/leak_sanitizer_suppressions.txt
)
set_property(TEST ${test}
PROPERTY ENVIRONMENT "${test_environment}"
Expand Down
3 changes: 1 addition & 2 deletions include/podio/SIOBlockUserData.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,11 @@ class SIOBlockUserData : public podio::SIOBlock {
.createBuffers(podio::userDataCollTypeName<BasicType>(), sio::version::major_version(version), false)
.value();

auto* dataVec = new std::vector<BasicType>();
auto* dataVec = m_buffers.dataAsVector<BasicType>();
unsigned size(0);
device.data(size);
dataVec->resize(size);
podio::handlePODDataSIO(device, &(*dataVec)[0], size);
m_buffers.data = dataVec;
}

void write(sio::write_device& device) override {
Expand Down
2 changes: 1 addition & 1 deletion include/podio/SIOFrameData.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class SIOFrameData {

public:
SIOFrameData() = delete;
~SIOFrameData() = default;
~SIOFrameData();

SIOFrameData(const SIOFrameData&) = delete;
SIOFrameData& operator=(const SIOFrameData&) = delete;
Expand Down
2 changes: 2 additions & 0 deletions include/podio/detail/LinkCollectionData.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ class LinkCollectionData {
if (!isSubsetColl) {
m_data.reset(buffers.dataAsVector<LinkData>());
}

delete buffers.references;
}

LinkCollectionData(const LinkCollectionData&) = delete;
Expand Down
4 changes: 3 additions & 1 deletion python/templates/CollectionData.cc.jinja2
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ void {{ class_type }}::clear(bool isSubsetColl) {
}

// Normal collections manage a bit more and have to clean up a bit more
if (m_data) m_data->clear();
if (m_data) {
m_data->clear();
}
{% if OneToManyRelations or OneToOneRelations %}
for (auto& pointer : m_refCollections) { pointer->clear(); }
{% endif %}
Expand Down
9 changes: 9 additions & 0 deletions src/SIOFrameData.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,13 @@ void SIOFrameData::readIdTable() {
m_subsetCollectionBits = idTableBlock->getSubsetCollectionBits();
}

SIOFrameData::~SIOFrameData() {
for (size_t i = 1; i < m_blocks.size(); ++i) {
if (m_availableBlocks[i]) {
auto buffers = dynamic_cast<SIOBlock*>(m_blocks[i].get())->getBuffers();
buffers.deleteBuffers(buffers);
}
}
}

} // namespace podio
4 changes: 3 additions & 1 deletion src/UserDataCollection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ namespace {
podio::UserDataCollection<T>::schemaVersion,
podio::userDataCollTypeName<T>(),
[](podio::CollectionReadBuffers buffers, bool) {
return std::make_unique<UserDataCollection<T>>(std::move(*buffers.dataAsVector<T>()));
auto vec = std::move(*buffers.dataAsVector<T>());
delete static_cast<std::vector<T>*>(buffers.data);
return std::make_unique<UserDataCollection<T>>(std::move(vec));
},
[](podio::CollectionReadBuffers& buffers) {
buffers.data = podio::CollectionWriteBuffers::asVector<T>(buffers.data);
Expand Down
36 changes: 30 additions & 6 deletions tests/CTestCustom.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ
write_python_frame_sio
read_python_frame_sio

write_python_frame_rntuple
read_python_frame_rntuple

relation_range

pyunittest
Expand All @@ -45,10 +48,16 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ
podio-dump-legacy_sio_v00-16-06
podio-dump-legacy_sio-detailed_v00-16-06

podio-dump-rntuple
podio-dump-detailed-rntuple

datamodel_def_store_roundtrip_root
datamodel_def_store_roundtrip_root_extension
datamodel_def_store_roundtrip_sio
datamodel_def_store_roundtrip_sio_extension
datamodel_def_store_roundtrip_rntuple
datamodel_def_store_roundtrip_rntuple_extension


write_old_data_root
read_new_data_root
Expand Down Expand Up @@ -76,15 +85,30 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ
set(CTEST_CUSTOM_TESTS_IGNORE
${CTEST_CUSTOM_TESTS_IGNORE}

read_sio
read_and_write_sio
write_timed_sio
read_timed_sio
read_frame_sio
read_interface_sio
read_frame_legacy_sio
read_and_write_frame_sio
)
endif()

if("@USE_SANITIZER@" MATCHES "Thread")
set(CTEST_CUSTOM_TESTS_IGNORE
${CTEST_CUSTOM_TESTS_IGNORE}

read_rntuple
read_interface_rntuple
)
endif()

if("@USE_SANITIZER@" MATCHES "Undefined" AND "@CMAKE_CXX_COMPILER_ID@" STREQUAL "Clang")
set(CTEST_CUSTOM_TESTS_IGNORE
${CTEST_CUSTOM_TESTS_IGNORE}

write_rntuple
read_rntuple
write_interface_rntuple
read_interface_rntuple
)

endif()

endif()
2 changes: 2 additions & 0 deletions tests/root_io/leak_sanitizer_suppressions.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Ignore leaks from Cling
leak:Cling
2 changes: 2 additions & 0 deletions tests/unittests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -83,5 +83,7 @@ else()
PODIO_SIOBLOCK_PATH=${PROJECT_BINARY_DIR}/tests
ENVIRONMENT
LD_LIBRARY_PATH=${CMAKE_CURRENT_BINARY_DIR}:${PROJECT_BINARY_DIR}/src:${PROJECT_BINARY_DIR}/tests:$<TARGET_FILE_DIR:ROOT::Tree>:$<$<TARGET_EXISTS:SIO::sio>:$<TARGET_FILE_DIR:SIO::sio>>:$ENV{LD_LIBRARY_PATH}
ENVIRONMENT
LSAN_OPTIONS=suppressions=${PROJECT_SOURCE_DIR}/tests/root_io/leak_sanitizer_suppressions.txt
)
endif()
6 changes: 3 additions & 3 deletions tests/unittests/unittest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1380,16 +1380,16 @@ TEST_CASE("ROOTWriter check consistency", "[ASAN-FAIL][UBSAN-FAIL][basics][root]

#if PODIO_ENABLE_RNTUPLE

TEST_CASE("Relations after cloning with RNTuple", "[relations][basics]") {
TEST_CASE("Relations after cloning with RNTuple", "[THREAD-FAIL][UBSAN-FAIL][relations][basics]") {
runRelationAfterCloneCheck<podio::RNTupleReader, podio::RNTupleWriter>(
"unittests_relations_after_cloning_rntuple.root");
}

TEST_CASE("RNTupleWriter consistent frame contents", "[basics][root]") {
TEST_CASE("RNTupleWriter consistent frame contents", "[UBSAN-FAIL][basics][root]") {
runConsistentFrameTest<podio::RNTupleWriter>("unittests_frame_consistency_rntuple.root");
}

TEST_CASE("RNTupleWriter check consistency", "[basics][root]") {
TEST_CASE("RNTupleWriter check consistency", "[UBSAN-FAIL][basics][root]") {
runCheckConsistencyTest<podio::RNTupleWriter>("unittests_frame_check_consistency_rntuple.root");
}

Expand Down
2 changes: 1 addition & 1 deletion tools/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ if(BUILD_TESTING)

if (ENABLE_RNTUPLE)
CREATE_DUMP_TEST(podio-dump-rntuple "write_rntuple" ${PROJECT_BINARY_DIR}/tests/root_io/example_rntuple.root)
CREATE_DUMP_TEST(podio-dump-rntuple-detailed "write_rntuple" --detailed --category events --entries 1:3 ${PROJECT_BINARY_DIR}/tests/root_io/example_rntuple.root)
CREATE_DUMP_TEST(podio-dump-detailed-rntuple "write_rntuple" --detailed --category events --entries 1:3 ${PROJECT_BINARY_DIR}/tests/root_io/example_rntuple.root)
endif()

endif()