Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace

[nlopes]

Summary

There are a few places where we use report_fatal_error when the input is broken. Currently, this function always crashes LLVM with an abort signal, which then triggers the backtrace printing code.
I think this is excessive, as wrong input shouldn't give a link to LLVM's github issue URL and tell users to file a bug report. We shouldn't print a stack trace either.

This patch changes report_fatal_error so it uses exit() rather than abort() when its argument GenCrashDiag=true.

As a side-effect, and the motivation for my change, this fixes 5 Alive2 reports, as our test driver doesn't propagate signal data.

[nikic]

Makes sense to me.

You probably also want to update https://github.com/llvm/llvm-project/blob/af430944b3ba8ca55c4fd6b73f53c198c469ffee/llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp#L911.

This patch changes report_fatal_error so it uses exit() rather than abort() when its argument GenCrashDiag=true.

This should probably read GenCrashDiag=false.

[RKSimon]

This patch changes report_fatal_error so it uses exit() rather than abort() when its argument GenCrashDiag=true.

GenCrashDiag=false?

[MaskRay]

llvm/lib/Support/ErrorHandling.cpp

125

https://www.opengroup.org/austin/docs/austin_1110.pdf

"Calling exit() with a value greater than 255 or less than 0 is something that only programs
which are specifically designed to have their exit status obtained by waitid() should do
(since it does not truncate the exit status to 8 bits). ``Pure ISO C’’ programs that call
exit(EXIT_FAILURE) do not meet this design criterion."

[nlopes]

llvm/lib/Support/ErrorHandling.cpp

125

Man page disagrees:
"The exit() function causes normal process termination and the value of status & 0377 is returned to the parent see wait(2))."

[MaskRay]

llvm/lib/Support/ErrorHandling.cpp

125

A value between 1 and 125 can be used.

1 and 2 are often used for other purposes.

Perhaps use a value larger than EX__MAX in sysexits.h

[MaskRay]

llvm/lib/Support/ErrorHandling.cpp

125

The Linux manpage is incorrect. With waitid you may access more than the least significant 8 bites, but that's not usage in practice.

bash has this https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html and zsh is similar.

[MaskRay]

llvm/lib/Support/ErrorHandling.cpp

125

1 and 2 are often used for other purposes. So better to use another.

[nlopes]

llvm/lib/Support/ErrorHandling.cpp

125

I will not bikeshed over this. Please suggest a value you like, otherwise I think 1 is just fine. Anything non-zero works.

[MaskRay]

llvm/lib/Support/ErrorHandling.cpp

125

1 and 2 are often used for other purposes.

Perhaps use a value larger than EX__MAX in sysexits.h

Please note that in a previous comment I mentioned this. Then I said it should be 126. A value in this range should be fine.

[nlopes]

llvm/lib/Support/ErrorHandling.cpp

125

1 and 2 are often used for other purposes.

Perhaps use a value larger than EX__MAX in sysexits.h

Please note that in a previous comment I mentioned this. Then I said it should be 126. A value in this range should be fine.

OK, thank you, will do.

[thakis]

This seems to break all kinds of tests, see http://45.33.8.238/linux/77205/step_12.txt

Did you run tests locally before landing?

Please take a look and revert for now if it takes a while to fix.

[nlopes]

In D126550#3546219, @thakis wrote:

This seems to break all kinds of tests, see http://45.33.8.238/linux/77205/step_12.txt

Did you run tests locally before landing?

Please take a look and revert for now if it takes a while to fix.

I'm on it, sorry for the breakage.
It seems not doesn't like exit value of 126,

[MaskRay]

llvm/lib/Support/ErrorHandling.cpp

125

Sorry, I made a typo as well. I should have said "between 1 and 125", as my previous comment said.

A value between 1 and 125 can be used.

[MaskRay]

llvm/lib/Support/ErrorHandling.cpp

125

Correction: the shell requires 1 to 125.
Many utilities use 1 and 2 for other purposes, so better to avoid them.
sysexits.h uses values up to 78.

A value satisfying all the requirement is good.

[nlopes]

llvm/lib/Support/ErrorHandling.cpp

125

Sorry, I made a typo as well. I should have said "between 1 and 125", as my previous comment said.

No worries. I had to quickly revert to 1 to fix build bots.
Feel free to change it; I really don't have any preference at all. (plus I don't have much hw resources to run LLVM tests frequently). Thank you!

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();
 
+  if (GenCrashDiag)
     abort();
+  else
+    exit(-1);
 }
 
 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/lib/Transforms/IPO/BlockExtractor.cpp

Buf->getBuffer().split(Lines, '\n', /*MaxSplit=*/-1,
                          /*KeepEmpty=*/false);
   for (const auto &Line : Lines) {
     SmallVector<StringRef, 4> LineSplit;
     Line.split(LineSplit, ' ', /*MaxSplit=*/-1,
                /*KeepEmpty=*/false);
     if (LineSplit.empty())
       continue;
     if (LineSplit.size()!=2)
-      report_fatal_error("Invalid line format, expecting lines like: 'funcname bb1[;bb2..]'");
+      report_fatal_error("Invalid line format, expecting lines like: 'funcname bb1[;bb2..]'",
+                         /*GenCrashDiag=*/false);
     SmallVector<StringRef, 4> BBNames;
     LineSplit[1].split(BBNames, ';', /*MaxSplit=*/-1,
                        /*KeepEmpty=*/false);
     if (BBNames.empty())
       report_fatal_error("Missing bbs name");
     BlocksByName.push_back(
         {std::string(LineSplit[0]), {BBNames.begin(), BBNames.end()}});
   }
bool BlockExtractor::runOnModule(Module &M) {
   }
 
   // Get all the blocks specified in the input file.
   unsigned NextGroupIdx = GroupsOfBlocks.size();
   GroupsOfBlocks.resize(NextGroupIdx + BlocksByName.size());
   for (const auto &BInfo : BlocksByName) {
     Function *F = M.getFunction(BInfo.first);
     if (!F)
-      report_fatal_error("Invalid function name specified in the input file");
+      report_fatal_error("Invalid function name specified in the input file",
+                         /*GenCrashDiag=*/false);
     for (const auto &BBInfo : BInfo.second) {
       auto Res = llvm::find_if(*F, [&](const BasicBlock &BB) {
         return BB.getName().equals(BBInfo);
       });
       if (Res == F->end())
-        report_fatal_error("Invalid block name specified in the input file");
+        report_fatal_error("Invalid block name specified in the input file",
+                           /*GenCrashDiag=*/false);
       GroupsOfBlocks[NextGroupIdx].push_back(&*Res);
     }
     ++NextGroupIdx;
   }
 
   // Extract each group of basic blocks.
   for (auto &BBs : GroupsOfBlocks) {
     SmallVector<BasicBlock *, 32> BlocksToExtractVec;
     for (BasicBlock *BB : BBs) {
       // Check if the module contains BB.
       if (BB->getParent()->getParent() != &M)
-        report_fatal_error("Invalid basic block");
+        report_fatal_error("Invalid basic block", /*GenCrashDiag=*/false);
       LLVM_DEBUG(dbgs() << "BlockExtractor: Extracting "
                         << BB->getParent()->getName() << ":" << BB->getName()
                         << "\n");
       BlocksToExtractVec.push_back(BB);
       if (const InvokeInst *II = dyn_cast<InvokeInst>(BB->getTerminator()))
         BlocksToExtractVec.push_back(II->getUnwindDest());
       ++NumExtracted;
       Changed = true;

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

GCOVOptions GCOVOptions::getDefault() {
   GCOVOptions Options;
   Options.EmitNotes = true;
   Options.EmitData = true;
   Options.NoRedZone = false;
   Options.Atomic = AtomicCounter;
 
   if (DefaultGCOVVersion.size() != 4) {
     llvm::report_fatal_error(Twine("Invalid -default-gcov-version: ") +
-                             DefaultGCOVVersion);
+                             DefaultGCOVVersion, /*GenCrashDiag=*/false);
   }
   memcpy(Options.Version, DefaultGCOVVersion.c_str(), 4);
   return Options;
 }
 
 namespace {
 class GCOVFunction;
 

llvm/test/Transforms/BlockExtractor/invalid-block.ll

 ; RUN: echo 'bar invalidbb' > %t
-; RUN: not --crash opt -S -extract-blocks -extract-blocks-file=%t %s 2>&1 | FileCheck %s
+; RUN: not opt -S -extract-blocks -extract-blocks-file=%t %s 2>&1 | FileCheck %s
 
 ; CHECK: Invalid block
 define void @bar() {
 bb:
   ret void
 }
 

llvm/test/Transforms/BlockExtractor/invalid-function.ll

 ; RUN: echo 'foo bb' > %t
-; RUN: not --crash opt -S -extract-blocks -extract-blocks-file=%t %s 2>&1 | FileCheck %s
+; RUN: not opt -S -extract-blocks -extract-blocks-file=%t %s 2>&1 | FileCheck %s
 
 ; CHECK: Invalid function
 define void @bar() {
 bb:
   ret void
 }
 

llvm/test/Transforms/BlockExtractor/invalid-line.ll

 ; RUN: echo 'foo' > %t
-; RUN: not --crash opt -S -extract-blocks -extract-blocks-file=%t %s 2>&1 | FileCheck %s
+; RUN: not opt -S -extract-blocks -extract-blocks-file=%t %s 2>&1 | FileCheck %s
 
 ; CHECK: Invalid line
 define void @bar() {
 bb:
   ret void
 }
 

llvm/test/Transforms/GCOVProfiling/version.ll

 ; RUN: rm -rf %t && mkdir -p %t
 ; RUN: echo 'target datalayout = "e"' > %t/little.txt
 ; RUN: echo 'target datalayout = "E"' > %t/big.txt
 ; RUN: echo '!9 = !{!"%/t/version.ll", !0}' > %t/version.txt
 ; RUN: cat %t/little.txt %s %t/version.txt > %t/2
 
 ; RUN: opt -passes=insert-gcov-profiling -disable-output < %t/2
 ; RUN: head -c8 %t/version.gcno | grep '^oncg.804'
 ; RUN: rm %t/version.gcno
-; RUN: not --crash opt -passes=insert-gcov-profiling -default-gcov-version=asdfasdf -disable-output < %t/2
+; RUN: not opt -passes=insert-gcov-profiling -default-gcov-version=asdfasdf -disable-output < %t/2
 ; RUN: opt -passes=insert-gcov-profiling -default-gcov-version='402*' -disable-output < %t/2
 ; RUN: head -c8 %t/version.gcno | grep '^oncg.204'
 ; RUN: rm %t/version.gcno
 
 ; RUN: cat %t/big.txt %s %t/version.txt > %t/big.ll
 ; RUN: opt -passes=insert-gcov-profiling -disable-output < %t/big.ll
 ; RUN: head -c8 %t/version.gcno | grep '^gcno408.'