4 ms·

Starting a paragraph about how to "safely" remove safeguards, and then tongue-in-cheek joking about Rust, kind of looks worse when your very first example isn't

by chc4 2y ago

Starting a paragraph about how to "safely" remove safeguards, and then tongue-in-cheek joking about Rust, kind of looks worse when your very first example isn't safe and liable to segfault even before the "proverbial gun" variant :) It needs to be > auto &lhs = *node_storage.emplace_back(make_unique(...)); instead of dereferencing lhs on the third line: emplace_back returns a reference to the inserted item, so it's a &unique_ptr...and that reference is invalidated by the rhs emplace_back reallocating the vector. Rust also has a number of easy to use ways to do the same thing - slotmap or typed_arena are the standard, or just doing the vector indices scheme manually like at the bottom: this post is talking in circles around the fact the vector is really being used as an arena allocator without saying that, and so has obvious problems with it being extremely fragile to anyone who reads the type and expects it to be vector-like. It would probably do better explicitly saying that and linking to the wikipedia page or whatever, so the reader can look into the proper way of doing the code pattern.

menaerus 2y ago
> instead of dereferencing lhs on the third line: emplace_back returns a reference to the inserted item, so it's a &unique_ptr...and that reference is invalidated by the rhs emplace_back reallocating the vector. Correct but only under the assumption that growing the backing storage of std::vector is implemented through a completely new instance of malloc, which indeed seems to be the case with libstdc++ implementation AFAICS. Otherwise, if realloc had been used, such condition wouldn't be guaranteed since realloc can grow from existing memory and thus not invalidating all other references to it. That said, I wonder if this behavior is mandated by the standard. https://en.cppreference.com/w/cpp/container/vector/emplace_back https://en.cppreference.com/w/cpp/container/vector/emplace_b... implies that it is but I didn't check If after the operation the new size() is greater than old capacity() a reallocation takes place, in which case all iterators (including the end() iterator) and all references to the elements are invalidated. Otherwise only the end() iterator is invalidated.
- withoutboats3 2y ago
  This comment is pedantic but not relevant, the grandparent comment was right. If the vector is reallocated to a new location in memory (either because it uses malloc or because realloc can't grow the existing allocation and must allocate somewhere new), the references will be invalidated and the behavior of this program is undefined. Using realloc would not make this code correct. The references also won't be invalidated if there's no need to reallocate, because the vector has enough space. Enumerating the cases in which this wouldn't cause UB does not change the fact that there are cases in which it would cause UB. I'm sure it feels good to make comments like this, but it's a disservice to newer developers who may be misled by your comment about whether or not this code contains UB.
  - menaerus 2y ago
    My comment acknowledges the issue mentioned by the parent comment and extends it with extra information out of my own curiosity, and which others may find useful or valuable too. Or not. I don't care. There surely could exist std::vector implementation with std::realloc implementation under the hood where the issue described wouldn't always be reproducible? I think that MSVC actually uses this technique AFAIR.
- Rusky 2y ago
  Unfortunately, C++ containers generally cannot use realloc, because they are required to invoke their elements' move/copy constructors when their addresses change.
  - menaerus 2y ago
    Yes, you're correct that generally you would not be able to use realloc for reasons you mentioned (and also because of destructors) but for simple POD (trivially-constructible) types I think it should be possible, no? In that case, realloc will either (1) grow the existing memory, perhaps default initialize it, and call it a day (2) Or if growing the memory is not possible, (a) allocate a new region of memory, (b) memcpy the contents from the old block and (c) free the old block. Perhaps I am missing something but I think those are all the guarantees that realloc is giving to you.

by chc4 2y ago

menaerus 2y ago

> instead of dereferencing lhs on the third line: emplace_back returns a reference to the inserted item, so it's a &unique_ptr...and that reference is invalidated by the rhs emplace_back reallocating the vector. Correct but only under the assumption that growing the backing storage of std::vector is implemented through a completely new instance of malloc, which indeed seems to be the case with libstdc++ implementation AFAICS. Otherwise, if realloc had been used, such condition wouldn't be guaranteed since realloc can grow from existing memory and thus not invalidating all other references to it. That said, I wonder if this behavior is mandated by the standard. https://en.cppreference.com/w/cpp/container/vector/emplace_back https://en.cppreference.com/w/cpp/container/vector/emplace_b... implies that it is but I didn't check If after the operation the new size() is greater than old capacity() a reallocation takes place, in which case all iterators (including the end() iterator) and all references to the elements are invalidated. Otherwise only the end() iterator is invalidated.

withoutboats3 2y ago
This comment is pedantic but not relevant, the grandparent comment was right. If the vector is reallocated to a new location in memory (either because it uses malloc or because realloc can't grow the existing allocation and must allocate somewhere new), the references will be invalidated and the behavior of this program is undefined. Using realloc would not make this code correct. The references also won't be invalidated if there's no need to reallocate, because the vector has enough space. Enumerating the cases in which this wouldn't cause UB does not change the fact that there are cases in which it would cause UB. I'm sure it feels good to make comments like this, but it's a disservice to newer developers who may be misled by your comment about whether or not this code contains UB.
- menaerus 2y ago
  My comment acknowledges the issue mentioned by the parent comment and extends it with extra information out of my own curiosity, and which others may find useful or valuable too. Or not. I don't care. There surely could exist std::vector implementation with std::realloc implementation under the hood where the issue described wouldn't always be reproducible? I think that MSVC actually uses this technique AFAIR.
Rusky 2y ago
Unfortunately, C++ containers generally cannot use realloc, because they are required to invoke their elements' move/copy constructors when their addresses change.
- menaerus 2y ago
  Yes, you're correct that generally you would not be able to use realloc for reasons you mentioned (and also because of destructors) but for simple POD (trivially-constructible) types I think it should be possible, no? In that case, realloc will either (1) grow the existing memory, perhaps default initialize it, and call it a day (2) Or if growing the memory is not possible, (a) allocate a new region of memory, (b) memcpy the contents from the old block and (c) free the old block. Perhaps I am missing something but I think those are all the guarantees that realloc is giving to you.