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.

Sign up for GitHub

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

iox-#2414 Reset index after move construction #2413

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+59 −14
Open

iox-#2414 Reset index after move construction #2413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 doc/website/release-notes/iceoryx-unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@
- Depend on @ncurses when building with Bazel [#2372](https://github.com/eclipse-iceoryx/iceoryx/issues/2372)
- Fix windows performance issue [#2377](https://github.com/eclipse-iceoryx/iceoryx/issues/2377)
- Max Client and Server cannot be configured independently of Max Publisher and Subscriber [#2394](https://github.com/eclipse-iceoryx/iceoryx/issues/2394)
- Fix issue where 'variant' index is not reset after move [#2414](https://github.com/eclipse-iceoryx/iceoryx/issues/2414)

**Refactoring:**

Expand Down
14 changes: 6 additions & 8 deletions iceoryx_hoofs/test/moduletests/test_vocabulary_expected.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,8 +321,7 @@ TEST_F(expected_test, CreateWithValueAndMoveCtorLeadsToMovedSource)

// NOLINTJUSTIFICATION we explicitly want to test the defined state of a moved expected
// NOLINTBEGIN(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_TRUE(sutSource.has_value());
EXPECT_TRUE(sutSource.value().m_moved);
ASSERT_FALSE(sutSource.has_value());
// NOLINTEND(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_TRUE(sutDestination.has_value());
EXPECT_FALSE(sutDestination.value().m_moved);
Expand All @@ -340,8 +339,8 @@ TEST_F(expected_test, CreateWithErrorAndMoveCtorLeadsToMovedSource)

// NOLINTJUSTIFICATION we explicitly want to test the defined state of a moved expected
// NOLINTBEGIN(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_TRUE(sutSource.has_error());
EXPECT_TRUE(sutSource.error().m_moved);
ASSERT_FALSE(sutSource.has_error());
ASSERT_FALSE(sutSource.has_value());
elBoberido marked this conversation as resolved.
Show resolved Hide resolved
// NOLINTEND(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_TRUE(sutDestination.has_error());
EXPECT_FALSE(sutDestination.error().m_moved);
Expand All @@ -359,8 +358,7 @@ TEST_F(expected_test, CreateWithValueAndMoveAssignmentLeadsToMovedSource)

// NOLINTJUSTIFICATION we explicitly want to test the defined state of a moved expected
// NOLINTBEGIN(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_TRUE(sutSource.has_value());
EXPECT_TRUE(sutSource.value().m_moved);
ASSERT_FALSE(sutSource.has_value());
// NOLINTEND(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_TRUE(sutDestination.has_value());
EXPECT_FALSE(sutDestination.value().m_moved);
Expand All @@ -378,8 +376,8 @@ TEST_F(expected_test, CreateWithErrorAndMoveAssignmentLeadsToMovedSource)

// NOLINTJUSTIFICATION we explicitly want to test the defined state of a moved expected
// NOLINTBEGIN(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_TRUE(sutSource.has_error());
EXPECT_TRUE(sutSource.error().m_moved);
ASSERT_FALSE(sutSource.has_error());
ASSERT_FALSE(sutSource.has_value());
elBoberido marked this conversation as resolved.
Show resolved Hide resolved
// NOLINTEND(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_TRUE(sutDestination.has_error());
EXPECT_FALSE(sutDestination.error().m_moved);
Expand Down
55 changes: 49 additions & 6 deletions iceoryx_hoofs/test/moduletests/test_vocabulary_variant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ TEST_F(variant_Test, MoveCTorWithValueLeadsToSameValue)
EXPECT_THAT(*ignatz.get<int>(), Eq(123));
// NOLINTJUSTIFICATION check if move is invalidating the object
// NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
EXPECT_THAT(schlomo.index(), Eq(0U));
EXPECT_THAT(schlomo.index(), Eq(iox::INVALID_VARIANT_INDEX));
}

TEST_F(variant_Test, MoveCTorWithoutValueResultsInInvalidVariant)
Expand All @@ -352,6 +352,48 @@ TEST_F(variant_Test, MoveCTorWithoutValueResultsInInvalidVariant)
ASSERT_THAT(ignatz.index(), Eq(iox::INVALID_VARIANT_INDEX));
}

TEST_F(variant_Test, MoveCTorWithVariantLeadToSameValue)
{
::testing::Test::RecordProperty("TEST_ID", "dc2a2aff-1fcd-4679-9bfc-b2fb4d2ae928");
iox::variant<int, float, ComplexClass> schlomo;
schlomo = ComplexClass(2, 3.14F);
iox::variant<int, float, ComplexClass> ignatz(std::move(schlomo));
// NOLINTJUSTIFICATION check if move is invalidating the object
// NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_THAT(schlomo.index(), Eq(iox::INVALID_VARIANT_INDEX));
elBoberido marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_THAT(ignatz.get<ComplexClass>()->a, Eq(2));
EXPECT_THAT(ignatz.get<ComplexClass>()->b, Eq(3.14F));
}

TEST_F(variant_Test, MoveAssignmentWithDifferentTypeVariantLeadsToSameValue)
{
::testing::Test::RecordProperty("TEST_ID", "562a38c3-aac2-4b1f-be55-c2d1b49e6c53");
iox::variant<int, float, ComplexClass> schlomo;
schlomo = ComplexClass(2, 3.14F);
iox::variant<int, float, ComplexClass> ignatz(2.14F);
ignatz = std::move(schlomo);
// NOLINTJUSTIFICATION check if move is invalidating the object
// NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_THAT(schlomo.index(), Eq(iox::INVALID_VARIANT_INDEX));
EXPECT_THAT(ignatz.get<ComplexClass>()->a, Eq(2));
EXPECT_THAT(ignatz.get<ComplexClass>()->b, Eq(3.14F));
}

TEST_F(variant_Test, MoveAssignmentWithSameTypeVariantLeadsToSameValue)
{
::testing::Test::RecordProperty("TEST_ID", "e4a530af-05c0-49e5-ae04-f3512f299fbe");
iox::variant<int, float, ComplexClass> schlomo;
schlomo = ComplexClass(2, 3.14F);
iox::variant<int, float, ComplexClass> ignatz;
ignatz = ComplexClass(3, 4.14F);
ignatz = std::move(schlomo);
// NOLINTJUSTIFICATION check if move is invalidating the object
// NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
ASSERT_THAT(schlomo.index(), Eq(iox::INVALID_VARIANT_INDEX));
EXPECT_THAT(ignatz.get<ComplexClass>()->a, Eq(2));
EXPECT_THAT(ignatz.get<ComplexClass>()->b, Eq(3.14F));
}

TEST_F(variant_Test, MoveAssignmentWithValueLeadsToSameValue)
{
::testing::Test::RecordProperty("TEST_ID", "ee36df28-545f-42bc-9ef6-3699284f1a42");
Expand Down Expand Up @@ -429,12 +471,12 @@ TEST_F(variant_Test, CreatingSecondObjectViaMoveCTorResultsInTwoDTorCalls)
EXPECT_THAT(DTorTest::dtorWasCalled, Eq(false));
// NOLINTJUSTIFICATION check if move is invalidating the object
// NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
EXPECT_THAT(ignatz.index(), Eq(1U));
EXPECT_THAT(ignatz.index(), Eq(iox::INVALID_VARIANT_INDEX));
}
EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
DTorTest::dtorWasCalled = false;
}
EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
EXPECT_THAT(DTorTest::dtorWasCalled, Eq(false));
Comment on lines +474 to +479
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this leads to a semantic change in the variant. The moved from variant does not call the d'tor of the underlying value after it is moved. This should be fine though, although maybe unexpected.

@elfenpiff since you wrote the variant, your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elBoberido actually, this is not fine. C++ requires that the dtor is called after an object is moved otherwise we might have a leak here.

Objects must be always safely destructable after move. For instance when one has a threadsafe class, the contents of that class might be moved but since a mutex is unmovable, the mutex is still valid and is only cleaned up after the destructor is called.
This makes sense, since it is allowed to assign a moved class a new value. Simple example could be a threadsafe string. This thing could be moved, and it is legal to assign a completely new string to the moved version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuanxingyang it seems you need to find another solution to the problem you want to fix.

Since the fix was so simple, we did not ask about the problem, which we should have done in the first place. Please excuse this.

Can you describe your issue? With more information we might find a solution that works for you and does not break the contract.

Copy link
Author

@yuanxingyang yuanxingyang Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your response. I agree with your explanation. Returning to my original question, I made the following modifications（the following code is the simplified version that can reproduce the issue）:
diff --git a/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp b/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp
index ac4d659d7..3e7a3e9a3 100644
--- a/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp
+++ b/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp
@@ -41,6 +41,8 @@ expected<IpcRuntimeInterface, IpcRuntimeInterfaceError> IpcRuntimeInterface::cre

    MgmtShmCharacteristics mgmtShmCharacteristics;


+  iox::variant<IpcInterfaceUser> tmp;
+  tmp = IpcInterfaceUser(roudi::IPC_CHANNEL_ROUDI_NAME, domainId, ResourceType::ICEORYX_DEFINED);

When running ./iox-roudi and ./iox-cpp-publisher, the ./iox-cpp-publisher will crash. The backtrace is as follows:"

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from ./iox-cpp-publisher...
(gdb) run
Starting program: /home/ynx3sgh/work/projects/iceoryx/build/install/prefix/bin/iox-cpp-publisher
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
2025-01-21 16:41:44.770 [Warn ]: IPC channel still there, doing an unlink of 'iox1_0_u_iox-cpp-publisher'

Program received signal SIGSEGV, Segmentation fault.
0x00005555555ad587 in iox::internal::call_at_index<0ul, iox::runtime::IpcInterfaceUser>::destructor (index=0, ptr=0x7fffffec8810) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant_internal.hpp:177
177 reinterpret_cast<T*>(ptr)->~T();

(gdb) bt
#0  0x00005555555ad587 in iox::internal::call_at_index<0ul, iox::runtime::IpcInterfaceUser>::destructor (index=0, ptr=0x7fffffec8810)
    at /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant_internal.hpp:177
#1  0x00005555555acad5 in iox::variant<iox::runtime::IpcInterfaceUser>::call_element_destructor (this=0x7fffffec8810)
    at /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl:141
#2  0x00005555555ac0e0 in iox::variant<iox::runtime::IpcInterfaceUser>::~variant (this=0x7fffffec8810, __in_chrg=<optimised out>)
    at /home/ynx3sgh/work/projects/iceoryx/iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl:133
#3  0x00005555555aaf40 in iox::runtime::IpcRuntimeInterface::create (runtimeName=..., domainId=..., roudiWaitingTimeout=...)
    at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/ipc_runtime_interface.cpp:157
#4  0x000055555557f179 in operator() (__closure=0x7ffffff968f0) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime_impl.cpp:64
#5  0x000055555557f7d2 in iox::runtime::PoshRuntimeImpl::PoshRuntimeImpl (this=0x5555556768c0 <iox::runtime::PoshRuntime::defaultRuntimeFactory(iox::optional<iox::string<87ul> const*>)::buf>, name=..., 
    domainId=..., location=iox::runtime::RuntimeLocation::SEPARATE_PROCESS_FROM_ROUDI) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime_impl.cpp:116
#6  0x000055555557cb6c in operator()<iox::optional<iox::string<87> const*> > (__closure=0x7fffffffd1cf, name=...) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime.cpp:87
#7  0x000055555557cc30 in iox::runtime::PoshRuntime::defaultRuntimeFactory (name=...) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime.cpp:90
#8  0x000055555557cda6 in iox::runtime::PoshRuntime::getInstance (name=...) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime.cpp:107
#9  0x000055555557cd35 in iox::runtime::PoshRuntime::initRuntime (name=...) at /home/ynx3sgh/work/projects/iceoryx/iceoryx_posh/source/runtime/posh_runtime.cpp:102
#10 0x00005555555613db in main () at /home/ynx3sgh/work/projects/iceoryx/iceoryx_examples/icedelivery/iox_publisher.cpp:37

(gdb) quit

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably used the wrong method to assign a value to the variant. When I replaced = with emplace to assign a value, the crash no longer occurred.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuanxingyang I have the feeling that the rule of 5 was not followed and IpcInterfaceUser should not be cooyable at all. This is quite an old class, with it's origins at an R&D department. I'll have a look at it later on.

}

TEST_F(variant_Test, CreatingSecondObjectViaMoveAssignmentResultsInTwoDTorCalls)
Expand All @@ -450,13 +492,14 @@ TEST_F(variant_Test, CreatingSecondObjectViaMoveAssignmentResultsInTwoDTorCalls)
schlomo = std::move(ignatz);
// NOLINTJUSTIFICATION check if move is invalidating the object
// NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved,clang-analyzer-cplusplus.Move)
EXPECT_THAT(ignatz.index(), Eq(1U));
EXPECT_THAT(DTorTest::dtorWasCalled, Eq(false));
EXPECT_THAT(ignatz.index(), Eq(iox::INVALID_VARIANT_INDEX));
DTorTest::dtorWasCalled = false;
}
// schlomo is destroyed when it goes out of scope
EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
DTorTest::dtorWasCalled = false;
}
EXPECT_THAT(DTorTest::dtorWasCalled, Eq(true));
EXPECT_THAT(DTorTest::dtorWasCalled, Eq(false));
}

TEST_F(variant_Test, DirectValueAssignmentResultsInCorrectIndex)
Expand Down
3 changes: 3 additions & 0 deletions iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ inline constexpr variant<Types...>::variant(variant&& rhs) noexcept
if (m_type_index != INVALID_VARIANT_INDEX)
{
internal::call_at_index<0, Types...>::moveConstructor(m_type_index, &rhs.m_storage, &m_storage);
rhs.m_type_index = INVALID_VARIANT_INDEX;
elBoberido marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -114,13 +115,15 @@ inline constexpr variant<Types...>& variant<Types...>::operator=(variant&& rhs)
if (m_type_index != INVALID_VARIANT_INDEX)
{
internal::call_at_index<0, Types...>::moveConstructor(m_type_index, &rhs.m_storage, &m_storage);
rhs.m_type_index = INVALID_VARIANT_INDEX;
}
}
else
{
if (m_type_index != INVALID_VARIANT_INDEX)
{
internal::call_at_index<0, Types...>::move(m_type_index, &rhs.m_storage, &m_storage);
rhs.m_type_index = INVALID_VARIANT_INDEX;
}
}
}
Expand Down
Loading