abseil / abseil-cpp

Abseil Common Libraries (C++)
https://abseil.io
Apache License 2.0
15.05k stars 2.62k forks source link

Error in reset_ctrl ? #334

Open oferim opened 5 years ago

oferim commented 5 years ago

https://github.com/abseil/abseil-cpp/blob/43ef2148c0936ebf7cb4be6b19927a9d9d145b8f/absl/container/internal/raw_hash_set.h#L1717

The length of ctrl is capacity + width + 1, and your comment says set it all to empty, except the last element, to sentinel. You are setting it all to empty, except the last element, but instead of setting the last element to sentinel, you are ignoring width. So: ctrl[capacity] = kSentinel; ==> ctrl[capacity_ + Group::kWidth] = kSentinel;

fowles commented 5 years ago

The sentinel doesn't go on the last element of the metadata array, it goes on the last capacity_ element. The reason is that we replicate the early bytes past the end two allow for circular scans to work out.

https://github.com/abseil/abseil-cpp/blob/43ef2148c0936ebf7cb4be6b19927a9d9d145b8f/absl/container/internal/raw_hash_set.h#L1738

is the code that will replicate the early ctrl with the ones after capacity to allow this.

oferim commented 5 years ago

In that case, isn't the last (ctrl_t) byte of the controls left uninitialized? Here https://github.com/abseil/abseil-cpp/blob/43ef2148c0936ebf7cb4be6b19927a9d9d145b8f/absl/container/internal/raw_hash_set.h#L557 you allocate capacity + width + 1. But here: https://github.com/abseil/abseil-cpp/blob/43ef2148c0936ebf7cb4be6b19927a9d9d145b8f/absl/container/internal/raw_hash_set.h#L1718 you initialize (to empty) only capacity + width.

fowles commented 5 years ago

Interesting, I am going to have to think on this one for a bit. My tentative read is "maybe" but I am shocked we would not have found this in our tests with sanitizers...

oferim commented 5 years ago

From looking here: https://github.com/abseil/abseil-cpp/blob/43ef2148c0936ebf7cb4be6b19927a9d9d145b8f/absl/container/internal/raw_hash_set.h#L467 we can see that you do use capacity + kWidth + 1 bytes for the controls. Still unclear why you do not initialize the last byte.