Skip to content

Commit

Permalink
fix: Make DataHandle type check more robust (#3768)
Browse files Browse the repository at this point in the history
It seems that the current implementation is prone to symbol hiding. I worked around this by inserting another non-templated layer in the class hierarchy which can do the type check more reliably.

The origin of the problem could be `-fvisibility=hidden` being toggled on all of my `ActsPythonBindings` compilation commands. It might be that this is another case where this flag leaks in via CMake from another library. I also did not want to change the visibility of the symbol manually as this would require some macros to be compiler agnostic.

Some resources
- https://www.qt.io/blog/quality-assurance/one-way-dynamic_cast-across-library-boundaries-can-fail-and-how-to-fix-it
- https://developers.redhat.com/articles/2021/10/27/compiler-option-hidden-visibility-and-weak-symbol-walk-bar#
  • Loading branch information
andiwand authored Oct 22, 2024
1 parent 54c27eb commit f537267
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 33 deletions.
1 change: 1 addition & 0 deletions Examples/Framework/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ add_library(
src/Framework/WhiteBoard.cpp
src/Framework/RandomNumbers.cpp
src/Framework/Sequencer.cpp
src/Framework/DataHandle.cpp
src/Utilities/EventDataTransforms.cpp
src/Utilities/Paths.cpp
src/Utilities/Options.cpp
Expand Down
57 changes: 25 additions & 32 deletions Examples/Framework/include/ActsExamples/Framework/DataHandle.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,9 @@

#pragma once

#include "Acts/Utilities/ThrowAssert.hpp"
#include "ActsExamples/Framework/SequenceElement.hpp"
#include "ActsExamples/Framework/WhiteBoard.hpp"

#include <iostream>
#include <stdexcept>
#include <typeinfo>

Expand Down Expand Up @@ -54,14 +52,33 @@ class DataHandleBase {
std::optional<std::string> m_key{};
};

template <typename T>
class ReadDataHandle;
class WriteDataHandleBase : public DataHandleBase {
protected:
WriteDataHandleBase(SequenceElement* parent, const std::string& name)
: DataHandleBase{parent, name} {}

public:
void initialize(const std::string& key);

bool isCompatible(const DataHandleBase& other) const final;
};

class ReadDataHandleBase : public DataHandleBase {
protected:
ReadDataHandleBase(SequenceElement* parent, const std::string& name)
: DataHandleBase{parent, name} {}

public:
void initialize(const std::string& key);

bool isCompatible(const DataHandleBase& other) const final;
};

template <typename T>
class WriteDataHandle final : public DataHandleBase {
class WriteDataHandle final : public WriteDataHandleBase {
public:
WriteDataHandle(SequenceElement* parent, const std::string& name)
: DataHandleBase{parent, name} {
: WriteDataHandleBase{parent, name} {
m_parent->registerWriteHandle(*this);
}

Expand All @@ -77,37 +94,17 @@ class WriteDataHandle final : public DataHandleBase {
wb.add(m_key.value(), std::move(value));
}

void initialize(const std::string& key) {
if (key.empty()) {
throw std::invalid_argument{"Write handle '" + fullName() +
"' cannot receive empty key"};
}
m_key = key;
}

bool isCompatible(const DataHandleBase& other) const override {
return dynamic_cast<const ReadDataHandle<T>*>(&other) != nullptr;
}

const std::type_info& typeInfo() const override { return typeid(T); };
};

template <typename T>
class ReadDataHandle final : public DataHandleBase {
class ReadDataHandle final : public ReadDataHandleBase {
public:
ReadDataHandle(SequenceElement* parent, const std::string& name)
: DataHandleBase{parent, name} {
: ReadDataHandleBase{parent, name} {
m_parent->registerReadHandle(*this);
}

void initialize(const std::string& key) {
if (key.empty()) {
throw std::invalid_argument{"Read handle '" + fullName() +
"' cannot receive empty key"};
}
m_key = key;
}

const T& operator()(const AlgorithmContext& ctx) const {
return (*this)(ctx.eventStore);
}
Expand All @@ -120,10 +117,6 @@ class ReadDataHandle final : public DataHandleBase {
return wb.get<T>(m_key.value());
}

bool isCompatible(const DataHandleBase& other) const override {
return dynamic_cast<const WriteDataHandle<T>*>(&other) != nullptr;
}

const std::type_info& typeInfo() const override { return typeid(T); };
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
#include <optional>
#include <stdexcept>
#include <string>
#include <typeinfo>
#include <unordered_map>
#include <utility>
#include <vector>
Expand Down
39 changes: 39 additions & 0 deletions Examples/Framework/src/Framework/DataHandle.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// This file is part of the ACTS project.
//
// Copyright (C) 2016 CERN for the benefit of the ACTS project
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

#include "ActsExamples/Framework/DataHandle.hpp"

namespace ActsExamples {

void WriteDataHandleBase::initialize(const std::string& key) {
if (key.empty()) {
throw std::invalid_argument{"Write handle '" + fullName() +
"' cannot receive empty key"};
}
m_key = key;
}

bool WriteDataHandleBase::isCompatible(const DataHandleBase& other) const {
return dynamic_cast<const ReadDataHandleBase*>(&other) != nullptr &&
typeInfo() == other.typeInfo();
}

void ReadDataHandleBase::initialize(const std::string& key) {
if (key.empty()) {
throw std::invalid_argument{"Read handle '" + fullName() +
"' cannot receive empty key"};
}
m_key = key;
}

bool ReadDataHandleBase::isCompatible(const DataHandleBase& other) const {
return dynamic_cast<const WriteDataHandleBase*>(&other) != nullptr &&
typeInfo() == other.typeInfo();
}

} // namespace ActsExamples
4 changes: 4 additions & 0 deletions Examples/Python/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ target_link_libraries(
ActsExamplesDetectorTelescope
ActsExamplesUtilities
ActsExamplesAmbiguityResolution
ActsExamplesTruthTracking
ActsExamplesDigitization
ActsExamplesPropagation
ActsExamplesMaterialMapping
)

set(py_files
Expand Down

0 comments on commit f537267

Please sign in to comment.