[Support] make report_fatal_error `abort` instead of `exit`

[ychen]

Summary

This patch could be treated as a rebase of D33960. It also fixes PR35547.
A fix for llvm/test/Other/close-stderr.ll is proposed in D68164. Seems
the consensus is that the test is passing by chance and I'm not
sure how important it is for us. So it is removed like in D33960 for now.
The rest of the test fixes are just adding --crash flag to not tool.

• The reason it fixes PR35547 is

exit does cleanup including calling class destructor whereas abort
does not do any cleanup. In multithreading environment such as ThinLTO or JIT,
threads may share states which mostly are ManagedStatic<>. If faulting thread
tearing down a class when another thread is using it, there are chances of
memory corruption. This is bad 1. It will stop error reporting like pretty
stack printer; 2. The memory corruption is distracting and nondeterministic in
terms of error message, and corruption type (depending one the timing, it
could be double free, heap free after use, etc.).

[ychen]

Here is another case showing the issue http://lists.llvm.org/pipermail/llvm-dev/2019-September/135283.html

[rnk]

This has been proposed before by @MatzeB:
https://reviews.llvm.org/D33960

At the time, there was an issue that some people had been "fixing" fuzzer bugs by replacing asserts with if (cond) report_fatal_error(...);. Moving to abort will reopen all of those fuzzer bugs. However, I think we have consensus that that is the desired behavior: every call to report_fatal_error is essentially a bug,. Code should be using the error handlers on MCContext and LLVMContext instead and recovering.

[ychen]

In D67847#1677251, @rnk wrote:

This has been proposed before by @MatzeB:
https://reviews.llvm.org/D33960

At the time, there was an issue that some people had been "fixing" fuzzer bugs by replacing asserts with if (cond) report_fatal_error(...);. Moving to abort will reopen all of those fuzzer bugs. However, I think we have consensus that that is the desired behavior: every call to report_fatal_error is essentially a bug,. Code should be using the error handlers on MCContext and LLVMContext instead and recovering.

Thanks for the pointer, Reid. It feels to me that both versions of report_fatal_error have their practical value in the codebase. If the only known value of the exit version is to make the fuzzer happy, should we just make fuzzer not report abort version of report_fatal_error? This way we could encourage the correct error handling and not miss anything from the fuzzer side?

PS: I think ThinLTO is another case of should not use report_fatal_error in the first place.

[rnk]

The main alternative worth mentioning is _exit, which LLD already uses for the same reason you mention here.

Honestly, I think there is consensus that calling abort here is the right thing and we should just do it. It's not like LLVM is already fuzzer clean, and if this does reopen fuzzer bugs, it's good to fix them properly anyway. :)

I mostly wanted to link back to the past attempt to do this to get the reviewers involved in that to +1 this new attempt. I guess I'll @ some more: @davide, @filcab, @mehdi_amini

[mehdi_amini]

LGTM: report_fatal_error() should not be considered like a "normal way" of aborting the compilation flow, crashing is never a good thing: in general I'm in favor of better error propagation (we claim that LLVM is a library framework right?)

[steven_wu]

This sounds fine to me. We should probably replace most of report_fatal_error with llvm::Error and propagate them correctly. In the meantime, having better crash is an improvement and does not conflict with what we want to do eventually.

[MaskRay]

This also fixes a C-compliance problem. report_fatal_error() may be called by a destructor, registered by atexit(). C11 says "If a program calls the exit function more than once, or calls the quick_exit function in addition to the exit function, the behavior is undefined."

As noted at https://reviews.llvm.org/D68164#1689155, close-stderr.ll should be updated to use not --crash opt --reject-this-option 2>&-. Don't rely on the exit code.

[ychen]

Fix tests

[jyknight]

The abort() function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior?

Either _exit() (long available extension, which lld already uses) or quick_exit() (the new C standard way) seem possibly preferable?

[ychen]

In D67847#1691898, @jyknight wrote:

The abort() function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior?

Either _exit() (long available extension, which lld already uses) or quick_exit() (the new C standard way) seem possibly preferable?

I think this patch is more for the purpose of making error handling better than for fixing PR35547. Fixing PR35547 is probably just a side effect. Apologies that I should have updated the summary to reflect that.

[rnk]

In D67847#1691898, @jyknight wrote:

The abort() function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior?

Either _exit() (long available extension, which lld already uses) or quick_exit() (the new C standard way) seem possibly preferable?

It's easy to crash LLVM even without this change, so anyone running LLVM better have core dumps configured the way they like. Failed asserts raise SIGABRT already, for example, and we have tons of those. The only difference is that now end users, who may have never configured this stuff, may see more crashes. If it's a problem, I'd consider it QoI: we should fix the report_fatal_error to use proper diagnostics anyway so end users don't see them as often, just as we would treat any other user-visible crash.

[ychen]

• rebase

[merge_guards_bot]

Unit tests: fail. 61892 tests passed, 2 failed and 782 were skipped.

failed: Clang.CXX/temp/temp_arg/temp_arg_template/p3-2a.cpp
failed: LLVM.CodeGen/SystemZ/mverify-optypes.mir

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[rnk]

I'm still in favor of this change.

[ychen]

Apologies that it takes so long. I'll fix the test and land it today.

[jyknight]

In D67847#1693694, @rnk wrote:

In D67847#1691898, @jyknight wrote:

The abort() function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior?

Either _exit() (long available extension, which lld already uses) or quick_exit() (the new C standard way) seem possibly preferable?

It's easy to crash LLVM even without this change, so anyone running LLVM better have core dumps configured the way they like. Failed asserts raise SIGABRT already, for example, and we have tons of those. The only difference is that now end users, who may have never configured this stuff, may see more crashes. If it's a problem, I'd consider it QoI: we should fix the report_fatal_error to use proper diagnostics anyway so end users don't see them as often, just as we would treat any other user-visible crash.

That may be, but right now we do report a bunch of errors via report_fatal_error, even if we should not. Coredumps seem unlikely to be of use in diagnosing the issue involved (unlike, perhaps, with a segfault), so I don't see why we'd _want_ to call abort vs quick_exit/_exit.

I express this as my opinion, but do not veto acceptance given by others.

[ychen]

• fix tests

[merge_guards_bot]

Unit tests: pass. 61894 tests passed, 0 failed and 782 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[ychen]

There are some interesting failures that need to be investigated.
https://reviews.llvm.org/rG647c3f4e47de8a850ffcaa897db68702d8d2459a

[rnk]

If we can reland this patch, it should fix this new clang bug:
https://bugs.llvm.org/show_bug.cgi?id=44705

This was the list of failures you noted on the commit:

In rG647c3f4e47de8a850ffcaa897db68702d8d2459a#885042, @ychen wrote:

This is going deeper...

• Host compiler crash (looks like ppc64be-specific)

http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/23016
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43016
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/34286

• clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp becomes pass/fail randomly

• test timeout

http://lab.llvm.org:8011/builders/openmp-gcc-x86_64-linux-debian/builds/2170/steps/test-openmp/logs/stdio

• ThinLTO test failures on Redhat (passes on my ubuntu 18.04)

http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/592

Will investigate.

The ppc64be stuff seems like only someone with hardware will be able to debug it.
For clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp, you are saying you can reproduce that locally?
The timeout seems like it could be flakiness or something unrelated.
The thinlto failures... I'm not sure either, but again it's PPC64, so it could be flakiness.

It seems like it would be best to alert the owners of those bots and reland and get new results, assuming things pass on less exotic bot configurations.

[ychen]

In D67847#1847757, @rnk wrote:

If we can reland this patch, it should fix this new clang bug:
https://bugs.llvm.org/show_bug.cgi?id=44705

This was the list of failures you noted on the commit:

In rG647c3f4e47de8a850ffcaa897db68702d8d2459a#885042, @ychen wrote:

This is going deeper...

• Host compiler crash (looks like ppc64be-specific)

http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/23016
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43016
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/34286

• clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp becomes pass/fail randomly

• test timeout

http://lab.llvm.org:8011/builders/openmp-gcc-x86_64-linux-debian/builds/2170/steps/test-openmp/logs/stdio

• ThinLTO test failures on Redhat (passes on my ubuntu 18.04)

http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/592

Will investigate.

The ppc64be stuff seems like only someone with hardware will be able to debug it.
For clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp, you are saying you can reproduce that locally?
The timeout seems like it could be flakiness or something unrelated.
The thinlto failures... I'm not sure either, but again it's PPC64, so it could be flakiness.

It seems like it would be best to alert the owners of those bots and reland and get new results, assuming things pass on less exotic bot configurations.

p3-2a.cpp random failure is fixed.
Thinlto / PPC issues are OOM related. PPC admin suggested recommit.

I've confirmed locally that failures on sanitier_windows are real (Patches are below), but I have no clue about how they are related.
D73329
D73327

llvm/lib/Support/ErrorHandling.cpp

if (handler) {
     (void)written; // If something went wrong, we deliberately just give up.
   }
 
   // If we reached here, we are failing ungracefully. Run the interrupt handlers
   // to make sure any special cleanups get done, in particular that we remove
   // files registered with RemoveFileOnSignal.
   sys::RunInterruptHandlers();
 
-  exit(1);
+  abort();
 }
 
 void llvm::install_bad_alloc_error_handler(fatal_error_handler_t handler,
                                            void *user_data) {
 #if LLVM_ENABLE_THREADS == 1
   std::lock_guard<std::mutex> Lock(BadAllocErrorHandlerMutex);
 #endif
   assert(!ErrorHandler && "Bad alloc error handler already registered!\n");

llvm/test/Bitcode/function-default-address-spaces.ll

 ; RUN: llvm-as %s  -o - | llvm-dis - | FileCheck %s -check-prefixes CHECK,PROG-AS0
 ; RUN: llvm-as -data-layout "P200" %s  -o - | llvm-dis | FileCheck %s -check-prefixes CHECK,PROG-AS200
-; RUN: not llvm-as -data-layout "P123456789" %s -o /dev/null 2>&1 | FileCheck %s -check-prefix BAD-DATALAYOUT
+; RUN: not --crash llvm-as -data-layout "P123456789" %s -o /dev/null 2>&1 | FileCheck %s -check-prefix BAD-DATALAYOUT
 ; BAD-DATALAYOUT: LLVM ERROR: Invalid address space, must be a 24-bit integer
 
 ; PROG-AS0-NOT: target datalayout
 ; PROG-AS200: target datalayout = "P200"
 
 ; Check that a function declaration without an address space (i.e. AS0) does not
 ; have the addrspace() attribute printed if it is address space zero and it is
 ; equal to the program address space.

llvm/test/MachineVerifier/test_g_inttoptr.mir

-#RUN: not llc -o - -march=arm64 -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s
+#RUN: not --crash llc -o - -march=arm64 -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s
 # REQUIRES: global-isel, aarch64-registered-target
 
 ---
 name:            test_inttoptr
 legalized:       true
 regBankSelected: false
 selected:        false
 tracksRegLiveness: true

llvm/test/tools/llvm-lto2/X86/pipeline.ll

define void @patatino() {
   ret void
 }
 
 ; CUSTOM: define void @patatino() {
 ; CUSTOM-NEXT:   ret void
 ; CUSTOM-NEXT: }
 
 ; Check that invalid pipelines are caught as errors.
-; RUN: not llvm-lto2 run %t1.bc -o %t.o \
+; RUN: not --crash llvm-lto2 run %t1.bc -o %t.o \
 ; RUN:  -r %t1.bc,patatino,px -opt-pipeline foogoo 2>&1 | \
 ; RUN:  FileCheck %s --check-prefix=ERR
 
 ; ERR: LLVM ERROR: unable to parse pass pipeline description 'foogoo': unknown pass name 'foogoo'
 
-; RUN: not llvm-lto2 run %t1.bc -o %t.o \
+; RUN: not --crash llvm-lto2 run %t1.bc -o %t.o \
 ; RUN:  -r %t1.bc,patatino,px -aa-pipeline patatino \
 ; RUN:  -opt-pipeline loweratomic 2>&1 | \
 ; RUN:  FileCheck %s --check-prefix=AAERR
 
 ; AAERR: LLVM ERROR: unable to parse AA pipeline description 'patatino': unknown alias analysis name 'patatino'

llvm/test/tools/llvm-objdump/relocations-elf.test

- Name:    glob1
     Size:    4
     Binding: STB_GLOBAL
   - Name:    glob2
     Binding: STB_GLOBAL
 
 ## Check we report an error if the relocated section identified by the
 ## sh_info field of a relocation section is invalid.
 # RUN: yaml2obj --docnum=2 %s > %t2
-# RUN: not llvm-objdump --reloc %t2 2>&1 | FileCheck %s --check-prefix=ERR
+# RUN: not --crash llvm-objdump --reloc %t2 2>&1 | FileCheck %s --check-prefix=ERR
 # ERR: LLVM ERROR: Invalid data was encountered while parsing the file
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data: ELFDATA2LSB
   Type: ET_REL
   Machine: EM_X86_64
 Sections:
 - Name: .rela.foo
   Type: SHT_RELA
   Link: .symtab
   Info: 0x255
   Relocations:
     - Offset: 0x1
       Type: R_X86_64_NONE

llvm/test/tools/llvm-readobj/arm64-many-epilogs.s

 // REQUIRES: aarch64-registered-target
 // RUN: llvm-mc -filetype=obj -triple aarch64-windows %s -o - \
-// RUN:   | not llvm-readobj --unwind - | FileCheck %s
+// RUN:   | not --crash llvm-readobj --unwind - 2>&1 | FileCheck %s
 
 // Older versions of LLVM had a bug where we would accidentally
 // truncate the number of epilogue scopes to a uint8_t; make
 // sure this doesn't happen.
 //
 // We expect the llvm-readobj invocation to fail because the
 // xdata section is truncated (to reduce the size of the testcase).
 

llvm/test/tools/llvm-readobj/arm64-win-error2.s

 ## Check that the sanity check for an inconsistent header works.
 ## The first word contains the bad value for CodeWords, 0xf, which indicates
 ## that we need 0x11110 << 2 =  120 bytes of space for the unwind codes.
 ## It follows that the .xdata section is badly formed as only 8 bytes are
 ## allocated for the unwind codes.
 
 // REQUIRES: aarch64-registered-target
 // RUN: llvm-mc -filetype=obj -triple aarch64-windows %s -o - \
-// RUN:   | not llvm-readobj --unwind - 2>&1 | FileCheck %s
+// RUN:   | not --crash llvm-readobj --unwind - 2>&1 | FileCheck %s
 
 // CHECK: LLVM ERROR: Malformed unwind data
 
   .text
   .globl  "?func@@YAHXZ"
   .p2align  3
 "?func@@YAHXZ":
   sub     sp,sp,#0x10