Improve error handling for SmallVector programming errors.

[GMNGeoffrey]

Summary

This patch changes errors in SmallVector::grow that are independent of
memory capacity to be reported using report_fatal_error or
std::length_error instead of report_bad_alloc_error, which falsely signals
an OOM.

It also cleans up a few related things:

• makes report_bad_alloc_error to print the failure reason passed to it.
• fixes the documentation to indicate that report_bad_alloc_error calls abort() not "an assertion"
• uses a consistent name for the size/capacity argument to grow and grow_pod

[MaskRay]

llvm/lib/Support/ErrorHandling.cpp

172

If the return value is to be ignored, the variable can be removed, i.e. (void)::write(...)

llvm/lib/Support/SmallVector.cpp

52

This can continue to use report_bad_alloc_error for a bit more context

[GMNGeoffrey]

llvm/lib/Support/ErrorHandling.cpp

172

Yeah I wasn't clear why this variable was here, but was just following the existing code. Happy to drop it.

llvm/lib/Support/SmallVector.cpp

52

I disagree. report_bad_alloc_error reports an OOM, which this is not. There is no amount of system memory that would make this not an error. The particular time we hit it, we'd accidentally passed a negative int to SmallVector::reserve. Having this reported as an OOM was confusing and I ended up wasting a bunch of debugging time trying to figure out what was using all our memory.

[GMNGeoffrey]

llvm/lib/Support/ErrorHandling.cpp

172

I assume it's to guard against some sort of MUST_USE_RETURN_VALUE check. I'm not sure whether casting directly to void without a variable will avoid that.

[mehdi_amini]

I disagree. report_bad_alloc_error reports an OOM, which this is not. There is no amount of system memory that would make this not an error.

What would new do?

LGTM overall.

llvm/lib/Support/ErrorHandling.cpp

172

Casting will do.

llvm/lib/Support/SmallVector.cpp

53

This use to be banned from LLVM because some stdlib we supported were buggy with this (Android maybe?)

In general I use instead Twine.

[GMNGeoffrey]

Apply same behavior to SmallVector::grow

[GMNGeoffrey]

In D86892#2248125, @mehdi_amini wrote:

I disagree. report_bad_alloc_error reports an OOM, which this is not. There is no amount of system memory that would make this not an error.

What would new do?

What's the question exactly?

new llvm::SmallVector<int, 4>(-1); will also call report_fatal_error when it tries to grow. Note that I also tested this out with a SmallVector of bools, which uses unit64_t as the size type because of https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/ADT/SmallVector.h#L87-L89. Then reserve(-1) doesn't trigger this failure (because that is theoretically a size you can request), but I do get a bad_alloc_error (because I don't have that much RAM).

LLVM ERROR: out of memory
Allocation failed

llvm/lib/Support/SmallVector.cpp

53

I just noticed that the normal SmallVector::grow has the same problem as this, so going to update that as well. That lives in SmallVector.h though, and trying to include Twine there gets us into all sorts of weird trouble. Is to_string still banned? Do you have another suggestion?

[mehdi_amini]

In D86892#2248235, @GMNGeoffrey wrote:

In D86892#2248125, @mehdi_amini wrote:

I disagree. report_bad_alloc_error reports an OOM, which this is not. There is no amount of system memory that would make this not an error.

What would new do?

What's the question exactly?

Would new char[std::numeric_limit<uint64_t>::max()]; issue an OOM exception? (The There is no amount of system memory that would make this not an error applies equally well here).

I guess a more accurate comparison would be what would std::vector<int> vec; vec.resize(-1); do? (throw std::length_error is the answer)

[mehdi_amini]

llvm/lib/Support/SmallVector.cpp

53

I grepped in LLVM and it seems like std::to_string is widely used now, so I guess Android (or whoever had this bug) upgraded their stdlibc++

[GMNGeoffrey]

In D86892#2248297, @mehdi_amini wrote:

In D86892#2248235, @GMNGeoffrey wrote:

In D86892#2248125, @mehdi_amini wrote:

I disagree. report_bad_alloc_error reports an OOM, which this is not. There is no amount of system memory that would make this not an error.

What would new do?

What's the question exactly?

Would new char[std::numeric_limit<uint64_t>::max()]; issue an OOM exception? (The There is no amount of system memory that would make this not an error applies equally well here).

array is too large (18446744073709551615 elements)
  new char[std::numeric_limits<uint64_t>::max()];

I guess a more accurate comparison would be what would std::vector<int> vec; vec.resize(-1); do? (throw std::length_error is the answer)

std::__u::__throw_length_error(char const*)

My point here is that we can provide a better and more specific error message, so reporting an OOM is pretty unhelpful. An assert would be another option, although if we're already reporting some sort of fatal error here even not in debug mode it seems redundant.

[MaskRay]

std::throw_length_error(char const*) is indeed more appropriate if we intend to match the standard interface. Since we also need to work in the absence of exceptions, report_fatal_error seems good as an alternative.

llvm/lib/Support/ErrorHandling.cpp

177

(void)::write(2, "\n", 1);

[GMNGeoffrey]

Throw a std::length_error when exceptions are enabled.

[GMNGeoffrey]

In D86892#2248345, @MaskRay wrote:

std::throw_length_error(char const*) is indeed more appropriate if we intend to match the standard interface. Since we also need to work in the absence of exceptions, report_fatal_error seems good as an alternative.

Agreed. I didn't find anywhere in LLVM that throws a length error. I'm actually not very familiar with C++ exceptions, since I never use them. It looks like LLVM_ENABLE_EXCEPTIONS isn't used much, so not sure whether it should be here? I've modified the patch to use it. Maybe this would warrant a report_length_error similar to report_bad_alloc_error in ErrorHandling?

[GMNGeoffrey]

Use auto for local string vars.

[GMNGeoffrey]

llvm/lib/Support/ErrorHandling.cpp

177

The way I had it seems more explicit about what's going on here (same reason OOMMessage is a separate variable, I assume). It's obviously easier to count bytes in newline, but seems better to let the compiler do it. I figured the compiler should optimize it to the same thing anyway, but fiddling with godbolt, it seems that's not actually the case. The char[] is still allocated. This actually applies to the OOMMessage variable too. Switching it to const char* (or auto, which deduces this) makes the compiled instructions the same, and the source code more explicit, I think. https://godbolt.org/z/q5KzhK

[MaskRay]

llvm/include/llvm/ADT/SmallVector.h

276 ↗

(On Diff #289050)

If you want to use #ifdef LLVM_ENABLE_EXCEPTIONS

#ifdef LLVM_ENABLE_EXCEPTIONS
#else
#endif

llvm/lib/Support/ErrorHandling.cpp

177

const char OOMMessage[] = ...

• clang performs constant evaluation of strlen while emitting LLVM IR.

const char *OOMMessage = ...

• clang does not evaluate the length.
• SimplifyLibCalls.cpp called by instcombine can perform constant evaluation of strlen.

char OOMMessage[] = ...

• clang does evaluate the length.
• SimplifyLibCalls.cpp called by instcombine cannot optimize the call.
• There is a runtime string copy and a runtime call of strlen.

[GMNGeoffrey]

llvm/include/llvm/ADT/SmallVector.h

276 ↗

(On Diff #289050)

You mean

#ifdef LLVM_ENABLE_EXCEPTIONS
  throw std::length_error(Reason);
#else
  report_fatal_error(Reason);
#endif

?

Why? Shouldn't throwing break us out of the control flow here?

llvm/lib/Support/ErrorHandling.cpp

177

Hmmm adding const doesn't seem to change anything: https://godbolt.org/z/19aYME

[GMNGeoffrey]

Fix lint. Switch to explicit const char *

llvm/lib/Support/SmallVector.cpp

47

I believe this is clang-tidy complaining about the pre-existing function name, which I don't think I should change.

[GMNGeoffrey]

Fix typo

[GMNGeoffrey]

And one more typo...

Always build before uploading even for trivial fixes.

[GMNGeoffrey]

Suppress pre-existing clang-tidy warnings

[MaskRay]

llvm/include/llvm/ADT/SmallVector.h

276 ↗

(On Diff #289050)

In both libstdc++ and libc++, they don't call abort or __builtin_abort after throw...

#else will match the standard interface better.

[GMNGeoffrey]

Use #else after throwing exception

[GMNGeoffrey]

If y'all are good with this now (and think that others will not have objects), could one of you land it for me? I don't have commit access.

llvm/include/llvm/ADT/SmallVector.h

276 ↗

(On Diff #289050)

Ok done :-)

[MaskRay]

In D86892#2253324, @GMNGeoffrey wrote:

If y'all are good with this now (and think that others will not have objects), could one of you land it for me? I don't have commit access.

Will do:) You can drop NOLINTNEXTLINE(readability-identifier-naming). The issues are known and are everywhere... Just ignore them and don't bother annotating the code...

[GMNGeoffrey]

In D86892#2253328, @MaskRay wrote:

In D86892#2253324, @GMNGeoffrey wrote:

If y'all are good with this now (and think that others will not have objects), could one of you land it for me? I don't have commit access.

Will do:) You can drop NOLINTNEXTLINE(readability-identifier-naming). The issues are known and are everywhere... Just ignore them and don't bother annotating the code...

I did that because without it the whole premerge check is reported as failing :-( I can drop them, but I wasn't even sure it wasn't masking some other error.

Seems that "build fails" and "clang-tidy fails and has commented" should not be the same failing check :-P

[GMNGeoffrey]

Drop NOLINTNEXTLINE

[GMNGeoffrey]

Thanks for reviewing!

llvm/include/llvm/Support/ErrorHandling.h

 /// error handler. In contrast to the generic 'report_fatal_error'
 /// functions, this function might not terminate, e.g. the user
 /// defined error handler throws an exception, but it won't return.
 ///
 /// Note: When throwing an exception in the bad alloc handler, make sure that
 /// the following unwind succeeds, e.g. do not trigger additional allocations
 /// in the unwind chain.
 ///
-/// If no error handler is installed (default), then a bad_alloc exception
-/// is thrown, if LLVM is compiled with exception support, otherwise an
-/// assertion is called.
+/// If no error handler is installed (default), throws a bad_alloc exception
+/// if LLVM is compiled with exception support. Otherwise prints the error
+/// to standard error and calls abort().
 LLVM_ATTRIBUTE_NORETURN void report_bad_alloc_error(const char *Reason,
                                                     bool GenCrashDiag = true);
 
 /// This function calls abort(), and prints the optional message to stderr.
 /// Use the llvm_unreachable macro (that adds location info), instead of
 /// calling this function directly.
 LLVM_ATTRIBUTE_NORETURN void
 llvm_unreachable_internal(const char *msg = nullptr, const char *file = nullptr,

llvm/lib/Support/ErrorHandling.cpp

 
 #ifdef LLVM_ENABLE_EXCEPTIONS
   // If exceptions are enabled, make OOM in malloc look like OOM in new.
   throw std::bad_alloc();
 #else
   // Don't call the normal error handler. It may allocate memory. Directly write
   // an OOM to stderr and abort.
   char OOMMessage[] = "LLVM ERROR: out of memory\n";
   ssize_t written = ::write(2, OOMMessage, strlen(OOMMessage));
+  written = ::write(2, Reason, strlen(Reason));
   (void)written;
   abort();
 #endif
 }
 
 #ifdef LLVM_ENABLE_EXCEPTIONS
 // Do not set custom new handler if exceptions are enabled. In this case OOM
 // errors are handled by throwing 'std::bad_alloc'.
 void llvm::install_out_of_memory_new_handler() {
 }
 #else
 // Causes crash on allocation failure. It is called prior to the handler set by

llvm/lib/Support/SmallVector.cpp

static_assert(sizeof(SmallVector<void *, 1>) ==
               "wasted space in SmallVector size 1");
 
 static_assert(sizeof(SmallVector<char, 0>) ==
                   sizeof(void *) * 2 + sizeof(void *),
               "1 byte elements have word-sized type for size and capacity");
 
 // Note: Moving this function into the header may cause performance regression.
 template <class Size_T>
 void SmallVectorBase<Size_T>::grow_pod(void *FirstEl, size_t MinCapacity,
                                        size_t TSize) {
   // Ensure we can fit the new capacity.
   // This is only going to be applicable when the capacity is 32 bit.
   if (MinCapacity > SizeTypeMax())
-    report_bad_alloc_error("SmallVector capacity overflow during allocation");
+    report_fatal_error("SmallVector unable to grow. Requested capacity (" +
+                       std::to_string(MinCapacity) +
+                       ") is larger than maximum capacity for size type (" +
+                       std::to_string(SizeTypeMax()) + ")");
 
   // 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.
   // This is only going to be applicable when the capacity is 32 bit.
   if (capacity() == SizeTypeMax())
-    report_bad_alloc_error("SmallVector capacity unable to grow");
+    report_fatal_error(
+        "SmallVector capacity unable to grow. Already at maximum size " +
+        std::to_string(SizeTypeMax()));
 
   // In theory 2*capacity can overflow if the capacity is 64 bit, but the
   // original capacity would never be large enough for this to be a problem.
   size_t NewCapacity = 2 * capacity() + 1; // Always grow.
   NewCapacity = std::min(std::max(NewCapacity, MinCapacity), SizeTypeMax());
 
   void *NewElts;
   if (BeginX == FirstEl) {