iox-#2414 Reset index after move construction

[yuanxingyang]

Notes for Reviewer

Pre-Review Checklist for the PR Author

1. Code follows the coding style of CONTRIBUTING.md
2. Tests follow the best practice for testing
3. Changelog updated in the unreleased section including API breaking changes
4. Branch follows the naming format (iox-123-this-is-a-branch)
5. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
6. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
7. Relevant issues are linked
8. Add sensible notes for the reviewer
9. All checks have passed (except task-list-completed)
10. Assign PR to reviewer

Checklist for the PR Reviewer

• Consider a second reviewer for complex new features or larger refactorings
• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• Unit tests have been written for new behavior
• Public API changes are documented via doxygen
• Copyright owner are updated in the changed files
• All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Closes 'variant' type index is not reset after move #2414

[elfenpiff]

@yuanxingyang Thank you for your contribution!

Please follow the contribution rules: https://github.com/eclipse-iceoryx/iceoryx/blob/main/CONTRIBUTING.md

1. Sign the eclipse contributor agreement with the same account you committed
2. Create an issue first
3. Adjust every commit message so that it has the prefix iox-#??? where ??? is the issue number

When this is done I am happy to merge your contribution

[elBoberido]

@yuanxingyang the process is a bit cumbersome for an initial contributor. The reason for all the 'paperwork' is because we are an Eclipse project (therefore the ECA) and we aim for safety certification (therefore the issue for traceability).

To ease your first contribution, I created #2414 for you. Please use this issue number for your commit. You can change the commit message with git commit --amend. For more regular contributions, the git hooks can be used:
https://github.com/eclipse-iceoryx/iceoryx/blob/main/tools/git-hooks/Readme.md

If possible, it would be great to create a tests for the bugfix.

[elBoberido]

@yuanxingyang it seems some tests are now failing. I guess it's because they were based on assumption which don't hold anymore.

Oh, and please also consider adding your copyright to the files you think have non trivial changes.

[yuanxingyang]

@yuanxingyang it seems some tests are now failing. I guess it's because they were based on assumption which don't hold anymore.

Oh, and please also consider adding your copyright to the files you think have non trivial changes.

In the latest commit, I fixed some issues with the test cases. Since the implementation of EXPECT relies on variant, I also modified some test cases related to EXPECT. Please help review the latest test changes to see if they are reasonable

I am not considering adding my own copyright notice; the modified code is fully shared with everyone

[elBoberido]

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?

[elfenpiff]

@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.

[elBoberido]

@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.

[yuanxingyang]

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

[yuanxingyang]

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.

[elBoberido]

@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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.69%. Comparing base (f33d582) to head (963b232).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2413      +/-   ##
==========================================
+ Coverage   78.68%   78.69%   +0.01%     
==========================================
  Files         440      440              
  Lines       16986    16989       +3     
  Branches     2361     2361              
==========================================
+ Hits        13365    13370       +5     
  Misses       2740     2740              
+ Partials      881      879       -2     

Flag	Coverage Δ
unittests	78.49% <100.00%> (+0.01%)	⬆️
unittests_timing	15.37% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...yx_hoofs/vocabulary/include/iox/detail/variant.inl	94.33% <100.00%> (+0.16%)	⬆️

... and 1 file with indirect coverage changes