[PATCH] D155598: [libc++abi] Use std::abort() instead of std::terminate() on failure to allocate

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.

LGTM.

I have a reliable reproduce on Linux using `ulimit -v` to limit the virtual memory size.

  printf 'int main() { for(;;) *(new char[995]) = 0; }' > a.cc
  myclang++ -stdlib=libc++ --rtlib=compiler-rt --unwindlib=libunwind a.cc -Wl,-rpath,$(dirname $(myclang --print-file-name=libc++.so.1)) -g -o aaa



  % (ulimit -v 10000; ./aaa)
  [1]    992598 segmentation fault  ( ulimit -v 10000; ./aaa; )

We can pick sizes different from 995. Smaller sizes are more likely to fail.
I think D119177 <https://reviews.llvm.org/D119177> made us more likely to get into a libc++ demangling_terminate_handler / libc++abi `__cxa_demangle` loop, eventually leading to a stach overflow.
But a small `new char[995]` will give us a stack overflow even if we revert D119177 <https://reviews.llvm.org/D119177>.

This patch will change the stack overflow cases (erroneous cases within our runtime libraries) to SIGABRT, which I think is an improvement.

> Just aborting on being out of memory really doesn't seem super nice to me, especially when there is a proper way to tell the user. So IMO fixing the code to return nullptr on failure is

the right thing to do.

I do not worry about these cases. I think this patch would bring us to a much better state.

`push_back` is called 30+ times within src/demangle. They all need to be changed to propagate an error. That would be too much engineering for very little benefit.



================
Comment at: libcxxabi/src/demangle/README.txt:37
 If you're working on the generic library, then do the work first in
-libcxxabi, then run the cp-to-llvm.sh script in src/demangle. This
+libcxxabi, then run libcxxabi/src/demangle/cp-to-llvm.sh. This
 script takes as an optional argument the path to llvm, and copies the
----------------
nickdesaulniers wrote:
> this LGTM if you want to commit this change to libcxxabi/src/demangle/README.txt separately of this commit.
LGTM +1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155598/new/

https://reviews.llvm.org/D155598