Refactoring of UsedChunkList

[FerdinandSpitzschnueffler]

Brief feature description

This whole list is error prone, uses the STL even though it does not have to use it (we should use here a cxx::vector) and it is not tested.

Detailed information

• [x] separate declaration and implementation
• [x] add unit tests
• [x] use cxx::vector instead of std::array see https://github.com/eclipse-iceoryx/iceoryx/issues/623#issuecomment-803986280
• [x] store sharedChunks? (see https://github.com/eclipse-iceoryx/iceoryx/pull/618#discussion_r597591727) see https://github.com/eclipse-iceoryx/iceoryx/issues/623#issuecomment-803986280
• [ ] change signature of remove to mepoo::SharedChunk remove(const mepoo::ChunkHeader& chunkHeader)
• [ ] decide if the UsedChunkList should behave like a Set and reject to take the same data a second time

[elBoberido]

It seems we desperately need a design document before this refactoring since there are some constraints which renders some of your point invalid. First and foremost, RouDi always needs to be able to access the used chunks in order to release them after an applications terminates. Since the application could terminate at any instruction, a container like a cxx::vector might be in a corrupted state and crash RouDi when accessing it. Therefor it must be possible to access the raw memory when RouDi cleans up. The std::array could be replaced by a C-style array though. Storing a SharedChunk should also not be done since we would have the same problems like in the SOFI with a possible Frankenstein object when the the copy assignment or dtor is only partially run. There is one big issue though. The UsedChunkList was designed to safely let RouDi cleanup the used chunks of an application which walked the plank. With the introduction of the relative_ptr this got broken, since the relative_ptr itself can become a Frankenstein object.

I suggest to make the relative_ptr a struct with a size of 8 byte again, e.g. by using a bitfield for the data members

struct relativ_ptr_data {
    constexpr relativ_ptr_data () noexcept : segment(MAX_SEGMENT), offset(MAX_OFFSET) {}
    constexpr relativ_ptr_data (uint16_t segment, uint64_t offset) noexcept : segment(segment), offset(offset) {}

    uint16_t segment : 16;
    uint64_t offset : 48;

    void reset() {
        constexpr relativ_ptr_data NULLPTR_EQUIVALENT{};
        *this = NULLPTR_EQUIVALENT;
    }

    static constexpr uint64_t MAX_SEGMENT{std::numeric_limits<uint16_t>::max()};
    static constexpr uint64_t MAX_OFFSET{(1ULL << 48) - 1};
};

Beside the size of 8 bytes which prevents torn writes when this is copied, the two member must simultaneously be reset in order to prevent the torn writes in this situation. I would also not add custom copy/move ctors/assignment operators to keep this as close to an uint64_t as possible.

This code snippet (https://godbolt.org/z/E56K3nbGr) shows that the performance impact would be negligible. There is an additional shift when the offset is accessed, but we have only 8 byte instead of 16, so passing this around got cheaper.

I suggest to refactor the relative_ptr in this way, but only after the release since the current implementation served well for over a year and can be assumed proven. We need to introduce this in the UsedChunkList before the release since the current implementation has a defect and later on this can become the base for relative_ptr.

@MatthiasKillat what do you think regarding the relative_ptr?

Edit: It seems when the a uint64_t is used instead of a bitfield and the segment with uint16_t is located at the lower bits and the offset at the upper 48 bits, the compiler generates the same code as for a bitfield. Since this would also work for Windows, this should be preferred