This is an archive of the discontinued LLVM Phabricator instance.

Improve error handling for SmallVector programming errors.
ClosedPublic

Authored by GMNGeoffrey on Aug 31 2020, 12:50 PM.

Details

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

Diff Detail

Event Timeline

GMNGeoffrey created this revision.Aug 31 2020, 12:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2020, 12:50 PM
GMNGeoffrey requested review of this revision.Aug 31 2020, 12:50 PM
MaskRay added inline comments.
llvm/lib/Support/ErrorHandling.cpp
171–172

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

llvm/lib/Support/SmallVector.cpp
51–60

This can continue to use report_bad_alloc_error for a bit more context

GMNGeoffrey added inline comments.Aug 31 2020, 1:04 PM
llvm/lib/Support/ErrorHandling.cpp
171–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
51–60

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 added inline comments.Aug 31 2020, 1:06 PM
llvm/lib/Support/ErrorHandling.cpp
171–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.

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
171–172

Casting will do.

llvm/lib/Support/SmallVector.cpp
52

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

In general I use instead Twine.

mehdi_amini accepted this revision.Aug 31 2020, 1:44 PM
This revision is now accepted and ready to land.Aug 31 2020, 1:44 PM
GMNGeoffrey marked 2 inline comments as done.

Apply same behavior to SmallVector::grow

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
52

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?

GMNGeoffrey edited the summary of this revision. (Show Details)Aug 31 2020, 2:57 PM

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 added inline comments.Aug 31 2020, 3:24 PM
llvm/lib/Support/SmallVector.cpp
52

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 added a comment.EditedAug 31 2020, 3:32 PM

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 accepted this revision.Aug 31 2020, 4:05 PM

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
175

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

Throw a std::length_error when exceptions are enabled.

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?

Use auto for local string vars.

GMNGeoffrey added inline comments.Aug 31 2020, 5:25 PM
llvm/lib/Support/ErrorHandling.cpp
175

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 added inline comments.Aug 31 2020, 6:04 PM
llvm/include/llvm/ADT/SmallVector.h
276

If you want to use #ifdef LLVM_ENABLE_EXCEPTIONS

#ifdef LLVM_ENABLE_EXCEPTIONS
#else
#endif
llvm/lib/Support/ErrorHandling.cpp
175

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 added inline comments.Aug 31 2020, 6:13 PM
llvm/include/llvm/ADT/SmallVector.h
276

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
175

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

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.

And one more typo...

Always build before uploading even for trivial fixes.

Harbormaster completed remote builds in B70325: Diff 289323.

Suppress pre-existing clang-tidy warnings

MaskRay added inline comments.Sep 2 2020, 2:45 PM
llvm/include/llvm/ADT/SmallVector.h
276

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

#else will match the standard interface better.

Use #else after throwing exception

GMNGeoffrey marked an inline comment as done.Sep 2 2020, 2:53 PM

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

Ok done :-)

GMNGeoffrey edited the summary of this revision. (Show Details)Sep 2 2020, 2:54 PM

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 marked an inline comment as done.Sep 2 2020, 2:56 PM

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

Drop NOLINTNEXTLINE

Thanks for reviewing!

This revision was landed with ongoing or failed builds.Sep 2 2020, 3:01 PM
This revision was automatically updated to reflect the committed changes.