-
Notifications
You must be signed in to change notification settings - Fork 83
Fix typed proxy access to generic skeleton event storage #394
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
base: main
Are you sure you want to change the base?
Changes from all commits
29563be
642fdd7
a73daf3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,11 @@ | |
| #define SCORE_MW_COM_IMPL_BINDINGS_LOLA_PROXY_EVENT_H | ||
|
|
||
| #include "score/mw/com/impl/bindings/lola/event_data_storage.h" | ||
| #include "score/mw/com/impl/bindings/lola/event_meta_info.h" | ||
| #include "score/mw/com/impl/bindings/lola/proxy_event_common.h" | ||
|
|
||
| #include "score/language/safecpp/safe_math/safe_math.h" | ||
| #include "score/memory/shared/pointer_arithmetic_util.h" | ||
| #include "score/mw/com/impl/proxy_event_binding.h" | ||
| #include "score/mw/com/impl/sample_reference_tracker.h" | ||
| #include "score/mw/com/impl/subscription_state.h" | ||
|
|
@@ -26,6 +30,7 @@ | |
| #include <score/assert.hpp> | ||
| #include <score/optional.hpp> | ||
|
|
||
| #include <cstdint> | ||
| #include <exception> | ||
| #include <iostream> | ||
| #include <limits> | ||
|
|
@@ -64,8 +69,11 @@ class ProxyEvent final : public ProxyEventBinding<SampleType> | |
| ProxyEvent(Proxy& parent, const ElementFqId element_fq_id, const std::string_view event_name) | ||
| : ProxyEventBinding<SampleType>{}, | ||
| proxy_event_common_{parent, element_fq_id, event_name}, | ||
| samples_{parent.GetEventDataStorage<SampleType>(element_fq_id)} | ||
| meta_info_{parent.GetEventMetaInfo(element_fq_id)}, | ||
| aligned_sample_size_{memory::shared::CalculateAlignedSize(sizeof(SampleType), alignof(SampleType))}, | ||
| event_slots_raw_array_{nullptr} | ||
| { | ||
| InitialiseEventSlotsRawArray(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could return const std::uint8_t* and be used to directly initialize event_slots_raw_array_. |
||
| } | ||
|
|
||
| ProxyEvent(const ProxyEvent&) = delete; | ||
|
|
@@ -130,13 +138,41 @@ class ProxyEvent final : public ProxyEventBinding<SampleType> | |
| }; | ||
|
|
||
| private: | ||
| void InitialiseEventSlotsRawArray() noexcept; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should only mark destructors / move ops as noexcept |
||
|
|
||
| Result<std::size_t> GetNewSamplesImpl(Callback&& receiver, TrackerGuardFactory& tracker) noexcept; | ||
| Result<std::size_t> GetNumNewSamplesAvailableImpl() const noexcept; | ||
|
|
||
| ProxyEventCommon proxy_event_common_; | ||
| const EventDataStorage<SampleType>& samples_; | ||
| const EventMetaInfo& meta_info_; | ||
| const std::size_t aligned_sample_size_; | ||
| const std::uint8_t* event_slots_raw_array_; | ||
| }; | ||
|
|
||
| template <typename SampleType> | ||
| inline void ProxyEvent<SampleType>::InitialiseEventSlotsRawArray() noexcept | ||
| { | ||
| auto& event_data_control_local = proxy_event_common_.GetConsumerEventDataControlLocal(); | ||
|
|
||
| const auto event_slots_raw_array_size = | ||
| safe_math::Multiply(aligned_sample_size_, event_data_control_local.GetMaxSampleSlots()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could also use the kAbortOnError return mode in safe_math |
||
| if (!event_slots_raw_array_size.has_value()) | ||
| { | ||
| score::mw::log::LogFatal("lola") << "Could not calculate the event slots raw array size. Terminating."; | ||
| std::terminate(); | ||
| } | ||
|
|
||
| const void* const event_slots_raw_array = meta_info_.event_slots_raw_array_.get(event_slots_raw_array_size.value()); | ||
|
|
||
| SCORE_LANGUAGE_FUTURECPP_PRECONDITION_PRD_MESSAGE(nullptr != event_slots_raw_array, "Null event slot array"); | ||
| SCORE_LANGUAGE_FUTURECPP_PRECONDITION_PRD_MESSAGE(meta_info_.data_type_info_.size == sizeof(SampleType), | ||
| "Event sample size mismatch"); | ||
| SCORE_LANGUAGE_FUTURECPP_PRECONDITION_PRD_MESSAGE(meta_info_.data_type_info_.alignment == alignof(SampleType), | ||
| "Event sample alignment mismatch"); | ||
|
|
||
| event_slots_raw_array_ = static_cast<const std::uint8_t*>(event_slots_raw_array); | ||
| } | ||
|
|
||
| template <typename SampleType> | ||
| inline Result<std::size_t> ProxyEvent<SampleType>::GetNumNewSamplesAvailable() const noexcept | ||
| { | ||
|
|
@@ -191,10 +227,23 @@ inline Result<std::size_t> ProxyEvent<SampleType>::GetNewSamplesImpl(Callback&& | |
| const auto slot_indices = proxy_event_common_.GetNewSamplesSlotIndices(max_sample_count); | ||
|
|
||
| auto& event_data_control_local = proxy_event_common_.GetConsumerEventDataControlLocal(); | ||
| SCORE_LANGUAGE_FUTURECPP_PRECONDITION_PRD_MESSAGE(nullptr != event_slots_raw_array_, "Null event slot array"); | ||
|
|
||
| for (auto slot_index_it = slot_indices.begin; slot_index_it != slot_indices.end; ++slot_index_it) | ||
| { | ||
| const SampleType& sample_data{samples_.at(static_cast<std::size_t>(*slot_index_it))}; | ||
| // NOLINTBEGIN(cppcoreguidelines-pro-bounds-pointer-arithmetic) The pointer event_slots_raw_array_ points to | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a todo here stating that this is just a temporary fix since the size of EventDataStorage may incorrect and the long term solution will be to type erase the binding layer. |
||
| // the first byte of the type-erased event sample storage in shared memory. Samples may originate from either a | ||
| // typed SkeletonEvent or a GenericSkeletonEvent, therefore slot lookup must use the stable EventMetaInfo raw | ||
| // storage address and SampleType stride instead of interpreting the shared-memory DynamicArray object type. | ||
| const auto* const object_start_address = &event_slots_raw_array_[aligned_sample_size_ * (*slot_index_it)]; | ||
| // NOLINTEND(cppcoreguidelines-pro-bounds-pointer-arithmetic) | ||
|
|
||
| // Suppress "AUTOSAR C++14 M5-2-8" rule finding: "An object with integer type or pointer to void type shall | ||
| // not be converted to an object with pointer type.". | ||
| // The raw storage address is provided through EventMetaInfo. The regular typed proxy validates the expected | ||
| // type at construction time and calculates the slot offset with sizeof(SampleType)/alignof(SampleType). | ||
| // coverity[autosar_cpp14_m5_2_8_violation] | ||
| const SampleType& sample_data{*reinterpret_cast<const SampleType*>(object_start_address)}; | ||
| const EventSlotStatus event_slot_status{event_data_control_local[*slot_index_it]}; | ||
| const EventSlotStatus::EventTimeStamp sample_timestamp{event_slot_status.GetTimeStamp()}; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -693,6 +693,33 @@ TYPED_TEST(LolaProxyEventDeathTest, FailOnEventNotFound) | |
| } | ||
|
|
||
| using LoLaTypedProxyEventTestFixture = LolaProxyEventFixture<ProxyEventStruct>; | ||
| TEST_F(LoLaTypedProxyEventTestFixture, ReadsSamplesFromEventMetaInfoRawStorage) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test name should clearly state what is the expectation of the test. e.g. In the title you say it should read samples from EventMetaInfoRawStorage but I don't see that tested in the test. Most likely that's just an implementation detail and we simply need to check that GetNewSamples works? |
||
| { | ||
| RecordProperty("Verifies", "SCR-6225206"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this RecordProperty added? |
||
| RecordProperty("Description", | ||
| "Checks that a typed ProxyEvent reads samples through EventMetaInfo raw storage."); | ||
| RecordProperty("TestType", "Regression test"); | ||
| RecordProperty("DerivationTechnique", "Analysis of bug 311"); | ||
|
|
||
| const std::size_t max_sample_count_subscription{5U}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All our tests should follow the given, when, then structure (e.g. https://martinfowler.com/bliki/GivenWhenThen.html) |
||
| this->GivenAProxyEvent(this->element_fq_id_, this->event_name_) | ||
| .ThatIsSubscribedWithMaxSamples(max_sample_count_subscription) | ||
| .WithSkeletonEventData({{kDummySampleValue, kDummyInputTimestamp}}); | ||
|
|
||
| const std::size_t max_samples{1U}; | ||
| TestSampleType received_sample{0U}; | ||
| const auto receiver = [&received_sample](impl::SamplePtr<TestSampleType> sample, | ||
| const tracing::ITracingRuntime::TracePointDataId) { | ||
| received_sample = *sample; | ||
| }; | ||
|
|
||
| const auto num_callbacks_result = this->GetNewSamples(receiver, max_samples); | ||
|
|
||
| ASSERT_TRUE(num_callbacks_result.has_value()); | ||
| EXPECT_EQ(num_callbacks_result.value(), 1U); | ||
| EXPECT_EQ(received_sample, kDummySampleValue); | ||
| } | ||
|
|
||
| TEST_F(LoLaTypedProxyEventTestFixture, SampleConstness) | ||
| { | ||
| RecordProperty("Verifies", "SCR-6340729"); | ||
|
|
@@ -703,8 +730,8 @@ TEST_F(LoLaTypedProxyEventTestFixture, SampleConstness) | |
| this->GivenAProxyEvent(this->element_fq_id_, this->event_name_); | ||
|
|
||
| ProxyEventAttorney<TestSampleType> proxy_event_attorney{*test_proxy_event_}; | ||
| using SamplesMemberType = typename std::remove_reference<decltype(proxy_event_attorney.GetSamplesMember())>::type; | ||
| static_assert(std::is_const<SamplesMemberType>::value, "Proxy should hold const slot data."); | ||
| using MetaInfoMemberType = typename std::remove_reference<decltype(proxy_event_attorney.GetMetaInfoMember())>::type; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add in a new test instead of modifying the current one. |
||
| static_assert(std::is_const<MetaInfoMemberType>::value, "Proxy should hold const event meta info."); | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| #include "score/mw/com/impl/runtime.h" | ||
| #include "score/mw/com/impl/skeleton_event_binding.h" | ||
|
|
||
| #include "score/language/safecpp/safe_math/safe_math.h" | ||
| #include "score/memory/shared/managed_memory_resource.h" | ||
| #include "score/memory/shared/new_delete_delegate_resource.h" | ||
| #include "score/memory/shared/shared_memory_factory.h" | ||
|
|
@@ -232,7 +233,35 @@ void* SkeletonMemoryManager::CreateGenericEventDataInCreatedSharedMemory( | |
| SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(inserted_meta_info.second, | ||
| "Couldn't register/emplace event-meta-info in data-section."); | ||
|
|
||
| return data_storage; | ||
| return event_data_raw_array; | ||
| } | ||
|
|
||
| void* SkeletonMemoryManager::RetrieveGenericEventDataFromOpenedSharedMemory( | ||
| const ElementFqId element_fq_id, | ||
| const SkeletonEventProperties& element_properties) noexcept | ||
| { | ||
| SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(storage_ != nullptr, "Service data storage is not available."); | ||
|
|
||
| const auto event_meta_info_it = storage_->events_metainfo_.find(element_fq_id); | ||
| SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(event_meta_info_it != storage_->events_metainfo_.cend(), | ||
| "Could not find element fq id in meta info map"); | ||
|
|
||
| const auto sample_size = event_meta_info_it->second.data_type_info_.size; | ||
| const auto sample_alignment = event_meta_info_it->second.data_type_info_.alignment; | ||
| const auto aligned_sample_size = | ||
| memory::shared::CalculateAlignedSize(sample_size, static_cast<std::size_t>(sample_alignment)); | ||
| const auto total_event_slots_size = safe_math::Multiply(aligned_sample_size, element_properties.number_of_slots); | ||
| if (!total_event_slots_size.has_value()) | ||
| { | ||
| score::mw::log::LogFatal("lola") << "Could not calculate the event slots raw array size. Terminating."; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are the others assertions and this is a terminate? You could also use the kAbortOnError return mode in safe_math |
||
| std::terminate(); | ||
| } | ||
|
|
||
| void* const event_slots_raw_array = | ||
| event_meta_info_it->second.event_slots_raw_array_.get(total_event_slots_size.value()); | ||
| SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(event_slots_raw_array != nullptr, | ||
| "Could not get generic EventDataStorage raw array"); | ||
| return event_slots_raw_array; | ||
| } | ||
|
|
||
| auto SkeletonMemoryManager::RetrieveEventControlsFromOpenedSharedMemory(const ElementFqId element_fq_id) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,9 +94,9 @@ class ProxyEventAttorney | |
|
|
||
| ProxyEventAttorney(ProxyEvent<T>& proxy_event) noexcept : proxy_event_{proxy_event} {} | ||
|
|
||
| auto& GetSamplesMember() | ||
| auto& GetMetaInfoMember() | ||
| { | ||
| return proxy_event_.samples_; | ||
| return proxy_event_.meta_info_; | ||
| } | ||
|
|
||
| private: | ||
|
|
@@ -206,7 +206,7 @@ class ProxyMockedMemoryFixture : public ::testing::Test | |
| EventControl* event_control_{nullptr}; | ||
| std::optional<ProviderEventDataControlLocalView<>> provider_event_data_control_local_{}; | ||
| std::optional<ConsumerEventDataControlLocalView<>> consumer_event_data_control_local_{}; | ||
| EventDataStorage<SampleType>* event_data_storage_{nullptr}; | ||
| void* event_slots_raw_array_{nullptr}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't remove event_data_storage_. We should rather emulate the production code in which event_slots_raw_array_ is a type erased view into the event_data_storage_ |
||
| RollbackSynchronization rollback_synchronization_{}; | ||
|
|
||
| std::shared_ptr<MessagePassingServiceMock> mock_service_{std::make_shared<MessagePassingServiceMock>()}; | ||
|
|
@@ -226,6 +226,7 @@ class LolaProxyEventResources : public ProxyMockedMemoryFixture | |
| void ExpectReregisterEventNotification(score::cpp::optional<pid_t> pid = {}); | ||
| void ExpectUnregisterEventNotification(score::cpp::optional<pid_t> pid = {}); | ||
|
|
||
|
|
||
| SlotIndexType PutData(const std::uint32_t value = 42, const EventSlotStatus::EventTimeStamp timestamp = 1); | ||
|
|
||
| const std::size_t max_num_slots_{5U}; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,10 +20,13 @@ | |
| #include "score/mw/com/impl/bindings/lola/service_data_storage.h" | ||
| #include "score/mw/com/impl/bindings/lola/skeleton_event_properties.h" | ||
|
|
||
| #include "score/memory/shared/pointer_arithmetic_util.h" | ||
|
|
||
| #include "score/memory/shared/i_shared_memory_resource.h" | ||
| #include "score/memory/shared/offset_ptr.h" | ||
| #include "score/memory/shared/shared_memory_resource_heap_allocator_mock.h" | ||
|
|
||
| #include <cstddef> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <tuple> | ||
|
|
@@ -48,14 +51,14 @@ struct FakeMockedServiceData | |
| /// \param id Event ID as used inside the Lola event structures. | ||
| /// \param max_num_slots Number of data slots. | ||
| /// \param max_subscribers maximum number of subscribers | ||
| /// \return A tuple that points to the newly initialized event-specific data structures. | ||
| /// \return A tuple containing the event control and the raw event sample storage pointer registered in | ||
| /// EventMetaInfo. | ||
| template <typename SampleType> | ||
| std::tuple<EventControl*, EventDataStorage<SampleType>*> AddEvent(ElementFqId id, | ||
| SkeletonEventProperties event_properties); | ||
| std::tuple<EventControl*, void*> AddEvent(ElementFqId id, SkeletonEventProperties event_properties); | ||
| }; | ||
|
|
||
| template <typename SampleType> | ||
| inline std::tuple<EventControl*, EventDataStorage<SampleType>*> FakeMockedServiceData::AddEvent( | ||
| inline std::tuple<EventControl*, void*> FakeMockedServiceData::AddEvent( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should be changed. Same point as: https://github.com/eclipse-score/communication/pull/394/changes#r3288235207 |
||
| const ElementFqId id, | ||
| const SkeletonEventProperties event_properties) | ||
| { | ||
|
|
@@ -71,20 +74,26 @@ inline std::tuple<EventControl*, EventDataStorage<SampleType>*> FakeMockedServic | |
| *control_memory)); | ||
| auto& event_control = std::get<EventControl>(*inserted_control); | ||
|
|
||
| EventDataStorage<SampleType>* event_data_slots = | ||
| data_memory->construct<EventDataStorage<SampleType>>(event_properties.number_of_slots, *data_memory); | ||
| const auto aligned_sample_size = | ||
| memory::shared::CalculateAlignedSize(sizeof(SampleType), alignof(SampleType)); | ||
| const auto total_data_size_bytes = aligned_sample_size * event_properties.number_of_slots; | ||
| const auto num_max_align_elements = | ||
| (total_data_size_bytes + sizeof(std::max_align_t) - 1U) / sizeof(std::max_align_t); | ||
|
|
||
| auto* const event_data_slots = data_memory->construct<EventDataStorage<std::max_align_t>>( | ||
| num_max_align_elements, memory::shared::PolymorphicOffsetPtrAllocator<std::max_align_t>(*data_memory)); | ||
| const memory::shared::OffsetPtr<void> rel_event_data_buffer{static_cast<void*>(event_data_slots)}; | ||
| data_storage->events_.emplace(id, rel_event_data_buffer); | ||
|
|
||
| const DataTypeMetaInfo sample_meta_info{sizeof(SampleType), alignof(SampleType)}; | ||
| auto* event_data_raw_array = event_data_slots->data(); | ||
| void* const event_data_raw_array = event_data_slots->data(); | ||
| const auto inserted_meta_info = | ||
| data_storage->events_metainfo_.emplace(std::piecewise_construct, | ||
| std::forward_as_tuple(id), | ||
| std::forward_as_tuple(sample_meta_info, event_data_raw_array)); | ||
| SCORE_LANGUAGE_FUTURECPP_ASSERT(inserted_meta_info.second); | ||
|
|
||
| return std::make_tuple(&event_control, event_data_slots); | ||
| return std::make_tuple(&event_control, event_data_raw_array); | ||
| } | ||
|
|
||
| } // namespace score::mw::com::impl::lola | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should also be added as deps in the build file if they're not already there.