skarupke / flat_hash_map

A very fast hashtable
1.69k stars 186 forks source link

sherwood_v8_block::empty_block() oddities #14

Open markpapadakis opened 6 years ago

markpapadakis commented 6 years ago

I noticed that sometimes sherwood_v8_table::emplace() would just fail even if the instance was empty(no elements). It turns out, that this happens because sometimes sherwood_v8_table::empty_block() would return a different pointer than you would get if you, say, printed that in the constructor, and if that 'd happen, sherwood_v8_table::emplace() would end up invoking rehash(), and that would, in turn, invoke deallocate_data() and (begin == BlockType::empty_block()) evaluate to false and so it 'd try to deallocate memory that was never allocated.

t made no sense, and, while I did put some time into trying to understand it, nothing immediately obvious was found, and it was only reproducible under specific conditions. I suspect it manifests when dealing with dlls/dsos. One quick fix involved changing the definition to:

static inline sherwood_v8_block *empty_block() {
        static std::unique_ptr<int8_t[]> data([] {
                auto ptr = new int8_t[BlockSize];

                std::fill(ptr, ptr + BlockSize, sherwood_v8_constants<>::magic_for_empty);
                return ptr;
        }());

        return reinterpret_cast<sherwood_v8_block *>(data.get());
}
cebtenzzre commented 5 years ago

I'm fairly certain the C++ standard guarantees that the existing code is correct. Dynamic linkage alone shouldn't affect this unless your compiler or linker has bugs. I can only imagine three ways this could break:

If changing the definition as you said actually fixes this, then I think that only the first case could be true.

EDIT: See #26. Depending on how the standard is interpreted, shared objects may not be considered translation units as per the standard's usual definition. Linux has the safer and more conforming behavior, Windows may not. This StackOverflow question explains this in more detail.