diff --git a/.github/workflows/sanitizers.yml b/.github/workflows/sanitizers.yml new file mode 100644 index 000000000..df71af2e5 --- /dev/null +++ b/.github/workflows/sanitizers.yml @@ -0,0 +1,38 @@ +name: sanitizers + +on: [push, pull_request] +jobs: + build-and-test: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + compiler: [gcc10, clang10] + # Since Leak is usually part of Address, we do not run it separately in + # CI. Keeping Address and Undefined separate for easier debugging + sanitizer: [Thread, + Address, + Undefined] + include: + # Memory sanitizer is not available for gcc + - compiler: clang10 + sanitizer: MemoryWithOrigin + steps: + - uses: actions/checkout@v2 + - uses: cvmfs-contrib/github-action-cvmfs@v2 + - uses: aidasoft/run-lcg-view@v2 + with: + release-platform: LCG_99/x86_64-centos7-${{ matrix.compiler }}-opt + run: | + set -x + mkdir build + cd build + cmake -DCMAKE_BUILD_TYPE=Debug \ + -DUSE_SANITIZER=${{ matrix.sanitizer }} \ + -DCMAKE_CXX_STANDARD=17 \ + -DCMAKE_CXX_FLAGS=" -fdiagnostics-color=always " \ + -DUSE_EXTERNAL_CATCH2=OFF \ + -DENABLE_SIO=ON \ + -G Ninja .. + ninja -k0 + ctest --output-on-failure diff --git a/CMakeLists.txt b/CMakeLists.txt index 599c58791..b25fd3878 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -56,6 +56,9 @@ include(cmake/podioBuild.cmake) podio_set_compiler_flags() podio_set_rpath() +set(USE_SANITIZER "" + CACHE STRING "Compile with a sanitizer. Options are Address, Memory, MemoryWithOrigin, Undefined, Thread, Leak, Address;Undefined") +ADD_SANITIZER_FLAGS() #--- Declare options ----------------------------------------------------------- option(CREATE_DOC "Whether or not to create doxygen doc target." OFF) diff --git a/cmake/podioBuild.cmake b/cmake/podioBuild.cmake index 6012b1e05..075aab337 100644 --- a/cmake/podioBuild.cmake +++ b/cmake/podioBuild.cmake @@ -94,3 +94,62 @@ macro(podio_set_compiler_flags) endif() endmacro(podio_set_compiler_flags) + +#--- add sanitizer flags, depending on the setting of USE_SANITIZER and the +#--- availability of the different sanitizers +macro(ADD_SANITIZER_FLAGS) + if(USE_SANITIZER) + if(USE_SANITIZER MATCHES "Address;Undefined" OR USE_SANITIZE MATCHES "Undefined;Address") + message(STATUS "Building with Address and Undefined behavior sanitizers") + add_compile_options("-fsanitize=address,undefined") + add_link_options("-fsanitize=address,undefined") + + elseif(USE_SANITIZER MATCHES "Address") + message(STATUS "Building with Address sanitizer") + add_compile_options("-fsanitize=address") + add_link_options("-fsanitize=address") + + elseif(USE_SANITIZER MATCHES "Undefined") + message(STATUS "Building with Undefined behaviour sanitizer") + add_compile_options("-fsanitize=undefined") + add_link_options("-fsanitize=undefined") + + elseif(USE_SANITIZER MATCHES "Thread") + message(STATUS "Building with Thread sanitizer") + add_compile_options("-fsanitize=thread") + add_link_options("-fsanitize=thread") + + elseif(USE_SANITIZER MATCHES "Leak") + # Usually already part of Address sanitizer + message(STATUS "Building with Leak sanitizer") + add_compile_options("-fsanitize=leak") + add_link_options("-fsanitize=leak") + + elseif(USE_SANITIZER MATCHES "Memory(WithOrigins)?") + if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + message(FATAL_ERROR "GCC does not have memory sanitizer support") + endif() + message(STATUS "Building with memory sanitizer") + add_compile_options("-fsanitize=memory") + add_link_options("-fsanitize=memory") + if(USE_SANITIZER MATCHES "MemoryWithOrigins") + message(STATUS "Adding origin tracking for memory sanitizer") + add_compile_options("-fsanitize-memory-track-origins") + endif() + + else() + message(FATAL_ERROR "Unsupported value for USE_SANITIZER: ${USE_SANITIZER}") + endif() + + # Set a few more flags if we have succesfully configured a sanitizer + # For nicer stack-traces in the output + add_compile_options("-fno-omit-frame-pointer") + # append_flag( CMAKE_CXX_FLAGS CMAKE_C_FLAGS) + + # Make it even easier to interpret stack-traces + if(uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG") + add_compile_options("-O0") + endif() + + endif(USE_SANITIZER) +endmacro(ADD_SANITIZER_FLAGS) diff --git a/include/podio/MaybeSharedPtr.h b/include/podio/MaybeSharedPtr.h new file mode 100644 index 000000000..fce6d3c62 --- /dev/null +++ b/include/podio/MaybeSharedPtr.h @@ -0,0 +1,152 @@ +#ifndef PODIO_MAYBESHAREDPTR_H +#define PODIO_MAYBESHAREDPTR_H + +#include + +namespace podio { + +namespace detail { + /// Tag struct to create MaybeSharedPtr instances that initially own their + /// managed pointer and hence will be created with a control block (ownership of + /// the managed pointer may still change later!) + struct MarkOwnedTag {}; +} + +constexpr static auto MarkOwned [[maybe_unused]] = detail::MarkOwnedTag{}; + +/// "Semi-smart" pointer class for pointers that at some point during their +/// lifetime might hand over management to another entitity. E.g. Objects that +/// are added to a collection will hand over the management of their Obj* to +/// collection. In such a case two things need to be considered: +/// - Other Objects with the same Obj* instance should not delete the managed +/// Obj*, even if the last Object goes out of scope +/// - Even if the managed Obj* is gone (e.g. collection has gone out of scope or +/// was cleared), the remaining Object instances should still be able to +/// gracefully destruct, even if they are at this point merely an "empty husk" +/// The MaybeSharedPtr achieves this by having an optional control block that +/// controls the lifetime of itself and potentially the managed Obj*. +template +class MaybeSharedPtr { +public: + /// Constructor from raw pointer. Assumes someone else manages the pointer + /// already + explicit MaybeSharedPtr(T* p) : m_ptr(p) {} + + /// Constructor from a raw pointer assuming ownership in the process + explicit MaybeSharedPtr(T* p, detail::MarkOwnedTag) : m_ptr(p), m_ctrlBlock(new ControlBlock()) {} + + /// Copy constructor + MaybeSharedPtr(const MaybeSharedPtr& other) : m_ptr(other.m_ptr), m_ctrlBlock(other.m_ctrlBlock) { + // Increase the reference count if there is a control block + m_ctrlBlock && m_ctrlBlock->count++; + } + + /// Assignment operator + MaybeSharedPtr& operator=(MaybeSharedPtr other) { + swap(*this, other); + return *this; + } + + /// Move constructor + MaybeSharedPtr(MaybeSharedPtr&& other) : m_ptr(other.m_ptr), m_ctrlBlock(other.m_ctrlBlock) { + other.m_ptr = nullptr; + other.m_ctrlBlock = nullptr; + } + + /// Destructor + ~MaybeSharedPtr() { + // Only if we have a control block, do we assume that we have any + // responsibility in cleaning things up + if (m_ctrlBlock && --m_ctrlBlock->count == 0) { + // When the reference count reaches 0 we have to clean up control block in + // any case, but first we have to find out whether we also need to clean + // up the "managed" pointer + if (m_ctrlBlock->owned) { + delete m_ptr; + } + delete m_ctrlBlock; + } + } + + /// Get a raw pointer to the managed pointer. Do not change anything + /// concerning the management of the pointer + T* get() const { return m_ptr; } + + /// Get a raw pointer to the managed pointer and assume ownership. + T* release() { + // From now on we only need to keep track of the control block + m_ctrlBlock->owned = false; + return m_ptr; + } + + operator bool() const { return m_ptr; } + + T* operator->() { return m_ptr; } + const T* operator->() const { return m_ptr; } + + T& operator*() { return *m_ptr; } + const T& operator*() const { return *m_ptr; } + + template + friend void swap(MaybeSharedPtr& a, MaybeSharedPtr& b); + + // avoid a bit of typing for having all the necessary combinations of + // comparison operators +#define DECLARE_COMPARISON_OPERATOR(op) \ + template \ + friend bool operator op (const MaybeSharedPtr& lhs, const MaybeSharedPtr& rhs); \ + template \ + friend bool operator op (const MaybeSharedPtr& lhs, const U* rhs); \ + template \ + friend bool operator op (const U* lhs, const MaybeSharedPtr& rhs); \ + + DECLARE_COMPARISON_OPERATOR(==) + DECLARE_COMPARISON_OPERATOR(!=) + DECLARE_COMPARISON_OPERATOR(<) +#undef DECLARE_COMPARISON_OPERATOR + +private: + /// Simple control structure that controls the behavior of the + /// MaybeSharedPtr destructor. Keeps track of how many references of the + /// ControlBlock are currently still alive and whether the managed pointer + /// should be destructed alongside the ControlBlock, once the reference count + /// reaches 0. + struct ControlBlock { + std::atomic count{1}; ///< reference count + std::atomic owned{true}; ///< ownership flag for the managed pointer. true == we manage the pointer + }; + + T* m_ptr{nullptr}; + ControlBlock* m_ctrlBlock{nullptr}; +}; + +template +void swap(MaybeSharedPtr& a, MaybeSharedPtr& b) { + using std::swap; + swap(a.m_ptr, b.m_ptr); + swap(a.m_ctrlBlock, b.m_ctrlBlock); +} + +// helper macro for avoiding a bit of typing/repetition +#define DEFINE_COMPARISON_OPERATOR(op) \ +template \ +bool operator op (const MaybeSharedPtr& lhs, const MaybeSharedPtr& rhs) { \ + return lhs.m_ptr op rhs.m_ptr; \ +} \ +template \ +bool operator op (const MaybeSharedPtr& lhs, const U* rhs) { \ + return lhs.m_ptr op rhs; \ +} \ +template \ +bool operator op (const U* lhs, const MaybeSharedPtr& rhs) { \ + return lhs op rhs.m_ptr; \ +} \ + +DEFINE_COMPARISON_OPERATOR(==) +DEFINE_COMPARISON_OPERATOR(!=) +DEFINE_COMPARISON_OPERATOR(<) +#undef DEFINE_COMPARISON_OPERATOR + +} + +#endif // PODIO_MAYBESHAREDPTR_H diff --git a/include/podio/ObjBase.h b/include/podio/ObjBase.h deleted file mode 100644 index 4aaeb24a8..000000000 --- a/include/podio/ObjBase.h +++ /dev/null @@ -1,47 +0,0 @@ -#ifndef OBJBASE_H -#define OBJBASE_H - -#include -#include -#include "podio/ObjectID.h" - -namespace podio { - - class ObjBase { - public: - - /// Constructor from ObjectID and initial object-count - ObjBase(ObjectID id_, unsigned i) : id(id_) , ref_counter(i) {}; - - /// checks whether object is "untracked" by a collection - /// if yes, increases reference count - void acquire() { - if (id.index == podio::ObjectID::untracked) ++ref_counter; - }; - - /// checks whether object is "untracked" by a collection - /// if yes, decrease reference count and delete itself if count===0 - int release(){ - if (id.index != podio::ObjectID::untracked){ return 1;}; - if (--ref_counter == 0) { - delete this; - } - return 0; - }; - - /// destructor - virtual ~ObjBase(){}; - - public: - /// ID of the object - podio::ObjectID id; - - private: - /// reference counter - std::atomic ref_counter; - - }; - -} // namespace - -#endif diff --git a/include/podio/SIOReader.h b/include/podio/SIOReader.h index 845f0a946..b528c667d 100644 --- a/include/podio/SIOReader.h +++ b/include/podio/SIOReader.h @@ -7,6 +7,7 @@ #include #include #include +#include #include "podio/ICollectionProvider.h" @@ -79,7 +80,10 @@ namespace podio { void readMetaDataRecord(std::shared_ptr mdBlock); void createBlocks(); - typedef std::pair Input; + /// collection, name, and flag to indictate whether it has been requested by + /// the EventStore. The latter is necessary, because collections which have + /// not been requested are still the responsibility of us + typedef std::tuple Input; std::vector m_inputs{}; CollectionIDTable* m_table{nullptr}; // will be owned by the EventStore int m_eventNumber{0}; diff --git a/python/templates/Collection.cc.jinja2 b/python/templates/Collection.cc.jinja2 index d291eb7d4..7fd01309c 100644 --- a/python/templates/Collection.cc.jinja2 +++ b/python/templates/Collection.cc.jinja2 @@ -32,11 +32,11 @@ Const{{ class.bare_type }} {{ collection_type }}::at(unsigned int index) const { } {{ class.bare_type }} {{ collection_type }}::operator[](unsigned int index) { - return {{ class.bare_type }}(m_storage.entries[index]); + return {{ class.bare_type }}(podio::MaybeSharedPtr(m_storage.entries[index])); } {{ class.bare_type }} {{ collection_type }}::at(unsigned int index) { - return {{ class.bare_type }}(m_storage.entries.at(index)); + return {{ class.bare_type }}(podio::MaybeSharedPtr(m_storage.entries.at(index))); } size_t {{ collection_type }}::size() const { @@ -65,7 +65,7 @@ void {{ collection_type }}::setSubsetCollection(bool setSubset) { {% endif %} obj->id = {int(m_storage.entries.size() - 1), m_collectionID}; - return {{ class.bare_type }}(obj); + return {{ class.bare_type }}(podio::MaybeSharedPtr<{{ class.bare_type }}Obj>(obj)); } void {{ collection_type }}::clear() { @@ -97,7 +97,7 @@ bool {{ collection_type }}::setReferences(const podio::ICollectionProvider* {% i return true; //TODO: check success } -void {{ collection_type }}::push_back(Const{{ class.bare_type }} object) { +void {{ collection_type }}::push_back({{ class.bare_type }} object) { // We have to do different things here depending on whether this is a // subset collection or not. A normal collection cannot collect objects // that are already part of another collection, while a subset collection @@ -107,24 +107,30 @@ void {{ collection_type }}::push_back(Const{{ class.bare_type }} object) { if (obj->id.index == podio::ObjectID::untracked) { const auto size = m_storage.entries.size(); obj->id = {(int)size, m_collectionID}; - m_storage.entries.push_back(obj); + m_storage.entries.push_back(obj.release()); {% if OneToManyRelations or VectorMembers %} - m_storage.createRelations(obj); + m_storage.createRelations(obj.get()); {% endif %} } else { throw std::invalid_argument("Object already in a collection. Cannot add it to a second collection"); } } else { - const auto obj = object.m_obj; - if (obj->id.index < 0) { - throw std::invalid_argument("Object needs to be tracked by another collection in order for it to be storable in a subset collection"); - } - m_storage.entries.push_back(obj); - // No need to handle any relations here, since this is already done by the - // "owning" collection + push_back(Const{{ class.bare_type }}(object)); } } +void {{ collection_type }}::push_back(Const{{ class.bare_type }} object) { + if (!m_isSubsetColl) { + throw std::invalid_argument("Can only add Const objects to subset collections!"); + } + auto obj = object.m_obj; + if (obj->id.index < 0) { + throw std::invalid_argument("Object needs to be tracked by another collection in order for it to be storable in a subset collection"); + } + m_storage.entries.push_back(obj); +} + + podio::CollectionBuffers {{ collection_type }}::getBuffers() { return m_storage.getCollectionBuffers(m_isSubsetColl); } diff --git a/python/templates/Collection.h.jinja2 b/python/templates/Collection.h.jinja2 index 9a36eea19..543a52916 100644 --- a/python/templates/Collection.h.jinja2 +++ b/python/templates/Collection.h.jinja2 @@ -85,7 +85,10 @@ public: /// Append object to the collection - void push_back(Const{{class.bare_type}} object); + void push_back({{class.bare_type}} object); + + /// Append a const reference to a subset collection + void push_back(Const{{ class.bare_type }} object); void prepareForWrite() override final; void prepareAfterRead() override final; @@ -160,7 +163,7 @@ template {% endfor %} m_storage.createRelations(obj); {% endif %} - return {{ class.bare_type }}(obj); + return {{ class.bare_type }}(podio::MaybeSharedPtr<{{ class.bare_type }}Obj>(obj)); } {% for member in Members %} diff --git a/python/templates/CollectionData.cc.jinja2 b/python/templates/CollectionData.cc.jinja2 index 9e4254658..cde9dee87 100644 --- a/python/templates/CollectionData.cc.jinja2 +++ b/python/templates/CollectionData.cc.jinja2 @@ -30,9 +30,8 @@ {{ class_type }}::~{{ class_type }}() { delete m_data; -{% if OneToManyRelations or OneToOneRelations %} for (auto& pointer : m_refCollections) delete pointer; -{% endif %} + {% for relation in OneToManyRelations + OneToOneRelations %} delete m_rel_{{ relation.name }}; {% endfor %} diff --git a/python/templates/ConstObject.cc.jinja2 b/python/templates/ConstObject.cc.jinja2 index c2f0d9bf8..8cfef982f 100644 --- a/python/templates/ConstObject.cc.jinja2 +++ b/python/templates/ConstObject.cc.jinja2 @@ -12,7 +12,22 @@ {{ utils.namespace_open(class.namespace) }} -{{ macros.constructors_destructors(class.bare_type, Members, prefix='Const') }} +{{ macros.common_constructors_destructors(class.bare_type, Members, prefix='Const') }} + +{% with full_type = 'Const' + class.bare_type %} +{{ full_type }}::{{ full_type }}() { + static auto defaultObj = {{ class.bare_type }}Obj{}; + m_obj = &defaultObj; +} + +{{ class.bare_type }} {{ full_type }}::clone() const { + return {{ class.bare_type }}(podio::MaybeSharedPtr(new {{ class.bare_type}}Obj(*m_obj), podio::MarkOwned)); +} + +{{ full_type }}::{{ full_type }}({{ class.bare_type }}Obj* obj) : m_obj(obj) {} + +{% endwith %} + {{ macros.member_getters(class, Members, use_get_syntax, prefix='Const') }} {{ macros.single_relation_getters(class, OneToOneRelations, use_get_syntax, prefix='Const') }} {{ macros.multi_relation_handling(class, OneToManyRelations + VectorMembers, use_get_syntax, prefix='Const') }} diff --git a/python/templates/ConstObject.h.jinja2 b/python/templates/ConstObject.h.jinja2 index 2ac2a9d42..4aa9acec6 100644 --- a/python/templates/ConstObject.h.jinja2 +++ b/python/templates/ConstObject.h.jinja2 @@ -10,6 +10,7 @@ {% for include in includes %} {{ include }} {% endfor %} +#include "podio/MaybeSharedPtr.h" #include "podio/ObjectID.h" {{ utils.forward_decls(forward_declarations) }} @@ -24,7 +25,9 @@ class Const{{ class.bare_type }} { friend class {{ class.bare_type }}ConstCollectionIterator; public: -{{ macros.constructors_destructors(class.bare_type, Members, prefix='Const') }} +{{ macros.common_constructors_destructors(class.bare_type, Members, prefix='Const') }} + /// constructor from existing {{ type }}Obj + explicit Const{{ class.bare_type }}({{ class.bare_type }}Obj* obj); public: @@ -34,8 +37,12 @@ public: {{ utils.if_present(ConstExtraCode, "declaration") }} {{ macros.common_object_funcs(class.bare_type, prefix='Const') }} + /// disconnect from {{ type }}Obj instance + void unlink() { m_obj = nullptr; } + private: - {{ class.bare_type }}Obj* m_obj; + {{ class.bare_type }}Obj* m_obj{nullptr}; + // podio::MaybeSharedPtr<{{ class.bare_type }}Obj> m_obj; }; {{ utils.namespace_close(class.namespace) }} diff --git a/python/templates/Obj.cc.jinja2 b/python/templates/Obj.cc.jinja2 index 8c89b8f42..ef3e58341 100644 --- a/python/templates/Obj.cc.jinja2 +++ b/python/templates/Obj.cc.jinja2 @@ -16,7 +16,7 @@ {{ utils.namespace_open(class.namespace) }} {% with obj_type = class.bare_type + 'Obj' %} {{ obj_type }}::{{ obj_type }}() : -{% raw %} ObjBase{{podio::ObjectID::untracked, podio::ObjectID::untracked}, 0}{% endraw %}, + id({podio::ObjectID::untracked, podio::ObjectID::untracked}), data(){{ single_relations_initialize(OneToOneRelations) }} {%- for relation in OneToManyRelations + VectorMembers %}, m_{{ relation.name }}(new std::vector<{{ relation.relation_type }}>()) @@ -25,11 +25,11 @@ { } {{ obj_type }}::{{ obj_type }}(const podio::ObjectID id_, {{ class.bare_type }}Data data_) : - ObjBase{id_, 0}, data(data_) + id(id_), data(data_) { } {{ obj_type }}::{{ obj_type }}(const {{ obj_type }}& other) : -{% raw %} ObjBase{{podio::ObjectID::untracked, podio::ObjectID::untracked}, 0}{% endraw %}, + id({podio::ObjectID::untracked, podio::ObjectID::untracked}), data(other.data){{ single_relations_initialize(OneToOneRelations) }} {%- for relation in OneToManyRelations + VectorMembers %}, m_{{ relation.name }}(new std::vector<{{ relation.relation_type }}>(*(other.m_{{ relation.name }}))) diff --git a/python/templates/Obj.h.jinja2 b/python/templates/Obj.h.jinja2 index 2dd6a6a70..91cb6b911 100644 --- a/python/templates/Obj.h.jinja2 +++ b/python/templates/Obj.h.jinja2 @@ -10,7 +10,7 @@ {{ include }} {% endfor %} -#include "podio/ObjBase.h" +#include "podio/ObjectID.h" {% if OneToManyRelations or VectorMembers %} #include {%- endif %} @@ -22,7 +22,7 @@ class {{ class.bare_type }}; class Const{{ class.bare_type }}; {% with obj_type = class.bare_type + 'Obj' %} -class {{ class.bare_type }}Obj : public podio::ObjBase { +class {{ class.bare_type }}Obj { public: /// constructor {{ obj_type }}(); @@ -40,6 +40,7 @@ public: {% endif %} public: + podio::ObjectID id; {{ class.bare_type }}Data data; {% for relation in OneToOneRelations %} {{ relation.relation_type }}* m_{{ relation.name }}{nullptr}; diff --git a/python/templates/Object.cc.jinja2 b/python/templates/Object.cc.jinja2 index 5eedfbcdf..89e1aa6e9 100644 --- a/python/templates/Object.cc.jinja2 +++ b/python/templates/Object.cc.jinja2 @@ -12,8 +12,25 @@ {{ utils.namespace_open(class.namespace) }} -{{ macros.constructors_destructors(class.bare_type, Members) }} -{{ class.bare_type }}::operator Const{{ class.bare_type }}() const { return Const{{ class.bare_type }}(m_obj); } +{{ macros.common_constructors_destructors(class.bare_type, Members) }} + +{{ class.bare_type }}::{{ class.bare_type }}() : m_obj(new {{ class.bare_type }}Obj(), podio::MarkOwned) {} + +{{ class.bare_type }} {{ class.bare_type }}::clone() const { + return {{ class.bare_type }}{podio::MaybeSharedPtr(new {{ class.bare_type }}Obj(*m_obj), podio::MarkOwned)}; +} + +{% if Members %} +{{ class.bare_type }}::{{ class.bare_type }}({{ Members | map(attribute='signature') | join(', ') }}) : m_obj(new {{ class.bare_type }}Obj(), podio::MarkOwned) { +{% for member in Members %} + m_obj->data.{{ member.name }} = {{ member.name }}; +{% endfor %} +} +{% endif %} + +{{ class.bare_type }}::{{ class.bare_type }}(podio::MaybeSharedPtr<{{ class.bare_type }}Obj> obj) : m_obj(obj) {} + +{{ class.bare_type }}::operator Const{{ class.bare_type }}() const { return Const{{ class.bare_type }}(m_obj.get()); } {{ macros.member_getters(class, Members, use_get_syntax) }} {{ macros.single_relation_getters(class, OneToOneRelations, use_get_syntax) }} diff --git a/python/templates/Object.h.jinja2 b/python/templates/Object.h.jinja2 index 61dedfa89..e9d17b590 100644 --- a/python/templates/Object.h.jinja2 +++ b/python/templates/Object.h.jinja2 @@ -12,6 +12,7 @@ {{ include }} {% endfor %} #include "podio/ObjectID.h" +#include "podio/MaybeSharedPtr.h" #include {{ utils.forward_decls(forward_declarations) }} @@ -27,7 +28,12 @@ class {{ class.bare_type }} { public: -{{ macros.constructors_destructors(class.bare_type, Members) }} +{{ macros.common_constructors_destructors(class.bare_type, Members) }} +{% if Members %} + /// Constructor initializing data members + {{ class.bare_type }}({{ Members | map(attribute='signature') | join(', ') }}); +{% endif %} + /// conversion to const object operator Const{{ class.bare_type }}() const; @@ -41,9 +47,15 @@ public: {{ utils.if_present(ExtraCode, "declaration") }} {{ utils.if_present(ConstExtraCode, "declaration") }} {{ macros.common_object_funcs(class.bare_type) }} + /// disconnect from {{ type }}Obj instance + void unlink() { m_obj = podio::MaybeSharedPtr<{{ class.bare_type }}Obj>(nullptr); } private: - {{ class.bare_type }}Obj* m_obj; + /// constructor from existing {{ type }}Obj + explicit {{ class.bare_type }}(podio::MaybeSharedPtr<{{ class.bare_type }}Obj> obj); + + // {{ class.bare_type }}Obj* m_obj; + podio::MaybeSharedPtr<{{ class.bare_type }}Obj> m_obj; }; std::ostream& operator<<(std::ostream& o, const Const{{ class.bare_type }}& value); diff --git a/python/templates/macros/declarations.jinja2 b/python/templates/macros/declarations.jinja2 index 4f83957c6..9b6baed74 100644 --- a/python/templates/macros/declarations.jinja2 +++ b/python/templates/macros/declarations.jinja2 @@ -54,16 +54,10 @@ {% endmacro %} -{% macro constructors_destructors(type, members, prefix='') %} +{% macro common_constructors_destructors(type, members, prefix='') %} {% set full_type = prefix + type %} /// default constructor {{ full_type }}(); -{% if members %} - {{ full_type }}({{ members | map(attribute='signature') | join(', ') }}); -{% endif %} - - /// constructor from existing {{ type }}Obj - {{ full_type }}({{ type }}Obj* obj); /// copy constructor {{ full_type }}(const {{ full_type }}& other); @@ -72,10 +66,10 @@ {{ full_type }}& operator=({{ full_type }} other); /// support cloning (deep-copy) - {{ full_type }} clone() const; + {{ type }} clone() const; /// destructor - ~{{ full_type }}(); + ~{{ full_type }}() = default; {% endmacro %} @@ -83,8 +77,6 @@ {% set full_type = prefix + type %} /// check whether the object is actually available bool isAvailable() const; - /// disconnect from {{ type }}Obj instance - void unlink() { m_obj = nullptr; } {% set inverse_type = type if prefix else 'Const' + type %} bool operator==(const {{ full_type }}& other) const { return m_obj == other.m_obj; } diff --git a/python/templates/macros/implementations.jinja2 b/python/templates/macros/implementations.jinja2 index 47be23093..5ca84c5d0 100644 --- a/python/templates/macros/implementations.jinja2 +++ b/python/templates/macros/implementations.jinja2 @@ -1,38 +1,12 @@ -{% macro constructors_destructors(type, members, prefix='') %} +#include "podio/MaybeSharedPtr.h" +{% macro common_constructors_destructors(type, members, prefix='') %} {% set full_type = prefix + type %} -{{ full_type }}::{{ full_type }}() : m_obj(new {{ type }}Obj()) { - m_obj->acquire(); -} - -{% if members %} -{{ full_type }}::{{ full_type }}({{ members | map(attribute='signature') | join(', ') }}) : m_obj(new {{ type }}Obj()) { - m_obj->acquire(); -{% for member in members %} - m_obj->data.{{ member.name }} = {{ member.name }}; -{% endfor %} -} -{% endif %} - -{{ full_type}}::{{ full_type }}(const {{ full_type }}& other) : m_obj(other.m_obj) { - m_obj->acquire(); -} +{{ full_type}}::{{ full_type }}(const {{ full_type }}& other) : m_obj(other.m_obj) {} {{ full_type }}& {{ full_type }}::operator=({{ full_type }} other) { swap(*this, other); return *this; } - -{{ full_type }}::{{ full_type }}( {{ type }}Obj* obj) : m_obj(obj) { - if (m_obj) m_obj->acquire(); -} - -{{ full_type }} {{ full_type }}::clone() const { - return {new {{ type }}Obj(*m_obj)}; -} - -{{ full_type }}::~{{ full_type }}() { - if (m_obj) m_obj->release(); -} {%- endmacro %} diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index eaba99ba8..1997576a3 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -1,8 +1,9 @@ {% macro iterator_declaration(class, prefix='') %} {% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %} +{% set ptr_init = 'nullptr' if prefix else 'podio::MaybeSharedPtr<' + class.bare_type + 'Obj>{nullptr}' %} class {{ iterator_type }} { public: - {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object(nullptr), m_collection(collection) {} + {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object({{ ptr_init }}), m_collection(collection) {} {{ iterator_type }}(const {{ iterator_type }}&) = delete; {{ iterator_type }}& operator=(const {{ iterator_type }}&) = delete; @@ -25,14 +26,15 @@ private: {% macro iterator_definitions(class, prefix='') %} +{% set ptr_type = '' if prefix else 'podio::MaybeSharedPtr<' + class.bare_type + 'Obj>' %} {% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %} {{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() { - m_object.m_obj = (*m_collection)[m_index]; + m_object.m_obj = {{ ptr_type }}((*m_collection)[m_index]); return m_object; } {{ prefix }}{{ class.bare_type }}* {{ iterator_type }}::operator->() { - m_object.m_obj = (*m_collection)[m_index]; + m_object.m_obj = {{ ptr_type }}((*m_collection)[m_index]); return &m_object; } diff --git a/src/SIOReader.cc b/src/SIOReader.cc index d9ed6aefd..eb25a9fb4 100644 --- a/src/SIOReader.cc +++ b/src/SIOReader.cc @@ -27,11 +27,14 @@ namespace podio { } auto p = std::find_if(begin(m_inputs), end(m_inputs), - [&name](const SIOReader::Input& t){ return t.second == name;}); + [&name](const SIOReader::Input& t){ return std::get<1>(t) == name;}); if (p != end(m_inputs)) { - p->first->prepareAfterRead(); - return p->first; + auto coll = std::get<0>(*p); + coll->prepareAfterRead(); + std::get<2>(*p) = true; + + return coll; } return nullptr; @@ -95,7 +98,7 @@ namespace podio { compressor.uncompress( m_rec_buffer.span(), m_unc_buffer ) ; sio::api::read_blocks( m_unc_buffer.span(), m_blocks ) ; - for (auto& [collection, name] : m_inputs) { + for (auto& [collection, name, read] : m_inputs) { collection->setID(m_table->collectionID(name)); } @@ -115,6 +118,10 @@ namespace podio { void SIOReader::endOfEvent() { ++m_eventNumber; m_blocks.clear(); + // Delete all collections which have not been used + for (const auto& [coll, name, used] : m_inputs) { + if (!used) delete coll; + } m_inputs.clear(); } @@ -127,7 +134,7 @@ namespace podio { for (size_t i = 0; i < m_typeNames.size(); ++i) { auto blk = podio::SIOBlockFactory::instance().createBlock(m_typeNames[i], m_table->names()[i], m_subsetCollectionBits[i]); m_blocks.push_back(blk); - m_inputs.emplace_back(blk->getCollection(), m_table->names()[i]); + m_inputs.emplace_back(blk->getCollection(), m_table->names()[i], false); } } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 5b6e13e7b..5cca86de3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -63,9 +63,13 @@ else() endif() CREATE_PODIO_TEST(unittest.cpp "Catch2::Catch2;Catch2::Catch2WithMain") -include(Catch) -catch_discover_tests(unittest WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}) +if(NOT USE_SANITIZER MATCHES "Memory(WithOrigin)?") + # This can fail with memory sanitizer because it actually runs an executable. + # Hence, we enable this only for non memory-sanitizer runs + include(Catch) + catch_discover_tests(unittest WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}) +endif() if (TARGET TestDataModelSioBlocks) set(sio_dependent_tests write_sio.cpp read_sio.cpp read_and_write_sio.cpp write_timed_sio.cpp read_timed_sio.cpp) diff --git a/tests/relation_range.cpp b/tests/relation_range.cpp index ad72b479e..07667f6ce 100644 --- a/tests/relation_range.cpp +++ b/tests/relation_range.cpp @@ -119,7 +119,7 @@ void doTestExampleMC(ExampleMCCollection const& collection) } try { - const auto parent = parents.at(3); + const auto parent [[maybe_unused]] = parents.at(3); throw std::runtime_error("Trying to access out of bounds in a RelationRange::at should throw"); } catch (const std::out_of_range& err) { ASSERT_EQUAL(err.what(), std::string("index out of bounds for RelationRange"), diff --git a/tests/unittest.cpp b/tests/unittest.cpp index c938be093..d18d94190 100755 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -20,7 +20,9 @@ #include "datamodel/ExampleWithOneRelation.h" #include "datamodel/ExampleWithOneRelationCollection.h" -TEST_CASE("AutoDelete") { +TEST_CASE("AutoDelete", "[memory management]") { + // NOTE: This is a unit test without any asserts as it should always work, + // even in an ASan build auto store = podio::EventStore(); auto hit1 = EventInfo(); auto hit2 = EventInfo(); @@ -89,6 +91,37 @@ TEST_CASE("Clearing"){ REQUIRE(success); } +TEST_CASE("Clearing in same scope", "[memory management]") { + // NOTE: This is a unit test without any asserts as it should always work, + // even in an ASan build + // Make sure that there is no heap-use-after-free even when the collection has + // taken over management of the Obj* even if clear is called in the same scope + // See: https://github.com/AIDASoft/podio/issues/174 + auto clusters = ExampleClusterCollection(); + auto cluster = clusters.create(); + clusters.clear(); +} + +TEST_CASE("Clearing order doesn't matter", "[memory management]") { + // NOTE: This is a unit test without any asserts as it should always work, + // even in an ASan build + // Make sure that the order in which collections are cleared do not cause any + // heap-use-after-free problems + // See: https://github.com/AIDASoft/podio/issues/174 + + auto clusters = ExampleClusterCollection(); + auto oneRelations = ExampleWithOneRelationCollection(); + { + // introduce a new scope to emulate framework usage + auto cluster = clusters.create(); + auto rel = oneRelations.create(); + rel.cluster(cluster); + } + + clusters.clear(); // Clear the related-to collection first + oneRelations.clear(); +} + TEST_CASE("Cloning"){ bool success = true; auto hit = ExampleHit(); @@ -126,7 +159,6 @@ TEST_CASE("Cyclic"){ } TEST_CASE("Invalid_refs") { - bool success = false; auto store = podio::EventStore(); auto& hits = store.create("hits"); auto hit1 = hits.create(0xcaffeeULL,0.,0.,0.,0.); @@ -135,12 +167,7 @@ TEST_CASE("Invalid_refs") { auto cluster = clusters.create(); cluster.addHits(hit1); cluster.addHits(hit2); - try { - clusters.prepareForWrite(); //should fail! - } catch (std::runtime_error&){ - success = true; - } - REQUIRE(success); + REQUIRE_THROWS_AS(clusters.prepareForWrite(), std::runtime_error); } TEST_CASE("Looping") {