Make SmallVector assert if it cannot grow.

[browneee]

Summary

Context:

/// Double the size of the allocated memory, guaranteeing space for at
/// least one more element or MinSize if specified.
void grow(size_t MinSize = 0) { this->grow_pod(MinSize, sizeof(T)); }

void push_back(const T &Elt) {
  if (LLVM_UNLIKELY(this->size() >= this->capacity()))
    this->grow();
  memcpy(reinterpret_cast<void *>(this->end()), &Elt, sizeof(T));
  this->set_size(this->size() + 1);
}

When grow is called in push_back() without a MinSize specified, this is
relying on the guarantee of space for at least one more element.

There is an edge case bug where the SmallVector is already at its maximum size
and push_back() calls grow() with default MinSize of zero. Grow is unable to
provide space for one more element, but push_back() assumes the additional
element it will be available. This can result in silent memory corruption, as
this->end() will be an invalid pointer and the program may continue executing.

Another alternative to fix would be to remove the default argument from
grow(), which would mean several changing grow() to grow(this->size()+1)
in several places.

No test case added because it would require allocating ~4GB.

[tellenbach]

llvm/lib/Support/SmallVector.cpp

42

Could you change this function's comment to include that the SmallVector needs to be able to grow by at least one element (please also change in llvm/include/llvm/ADT/SmallVector.h)? This is currently only documented for grow but your patch adds a restriction that should be documented locally.

51

Can you use Capacity directly and omit the function call?

[browneee]

Added function comment on grow_pod.

[browneee]

llvm/lib/Support/SmallVector.cpp

51

I expect this call will be inlined, along with the existing calls to capacity() and size() in this function.
Using capacity() seems consistent with the rest of the SmallVector code.

[tellenbach]

llvm/include/llvm/ADT/SmallVector.h

49 ↗

(On Diff #255557)

Here is some word missing: This function will report a fatal error if it cannot increase capactity.?

llvm/lib/Support/SmallVector.cpp

42

Same here: A word is missing.

[browneee]

Add "report a " to comments.

[tellenbach]

LGTM, thanks.

[browneee]

I am not a committer, please submit for me.

[browneee]

Incorporated into https://reviews.llvm.org/D77621 (because this is not submitted yet, and that change now moves this code).

[browneee]

Update. Add check to SmallVectorTemplateBase<T, TriviallyCopyable>::grow too.

[browneee]

Rebase to head.

[dexonsmith]

In D77601#1974745, @browneee wrote:

Incorporated into https://reviews.llvm.org/D77621 (because this is not submitted yet, and that change now moves this code).

Based on this comment I thought you'd abandoned this patch...

In D77601#1995667, @browneee wrote:

Rebase to head.

But since you're updating here, I should note that the patch as it looks here is still working with UINT32_MAX instead of max_size(). It should be updated in light of https://reviews.llvm.org/D77621.

[browneee]

Landed as a30e7ea88e75568feed020aedae73c52de888835

[browneee]

In D77601#1995831, @dexonsmith wrote:

In D77601#1974745, @browneee wrote:

Incorporated into https://reviews.llvm.org/D77621 (because this is not submitted yet, and that change now moves this code).

Based on this comment I thought you'd abandoned this patch...

I should have been more explicit here. After D77621 was reverted, I picked it up again to make this change separately.

In D77601#1995667, @browneee wrote:

Rebase to head.

But since you're updating here, I should note that the patch as it looks here is still working with UINT32_MAX instead of max_size(). It should be updated in light of https://reviews.llvm.org/D77621.

Sorry, I didn't see this comment before pushing.

I think UINT32_MAX is consistent with existing usage in these functions. In D77621 the max_size() part of the change was necessary because it could vary. Here we can leave that change to a separate patch.

llvm/lib/Support/SmallVector.cpp

 static_assert(sizeof(SmallVector<Struct32B, 0>) >= alignof(Struct32B),
               "missing padding for 32-byte aligned T");
 static_assert(sizeof(SmallVector<void *, 1>) ==
                   sizeof(unsigned) * 2 + sizeof(void *) * 2,
               "wasted space in SmallVector size 1");
 
 /// grow_pod - This is an implementation of the grow() method which only works
 /// on POD-like datatypes and is out of line to reduce code duplication.
 void SmallVectorBase::grow_pod(void *FirstEl, size_t MinCapacity,
                                size_t TSize) {
   // Ensure we can fit the new capacity in 32 bits.
   if (MinCapacity > UINT32_MAX)
     report_bad_alloc_error("SmallVector capacity overflow during allocation");
 
+  // Ensure we can meet the guarantee of space for at least one more element.
+  // The above check alone will not catch the case where grow is called with a
+  // default MinCapacity of 0, but the current capacity cannot be increased.
+  if (capacity() == size_t(UINT32_MAX))
+    report_bad_alloc_error("SmallVector capacity unable to grow");
+
   size_t NewCapacity = 2 * capacity() + 1; // Always grow.
   NewCapacity =
       std::min(std::max(NewCapacity, MinCapacity), size_t(UINT32_MAX));
 
   void *NewElts;
   if (BeginX == FirstEl) {
     NewElts = safe_malloc(NewCapacity * TSize);