[LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures

[aganea]

Summary

This is a follow-up for D70378 (Cover usage of LLD as a library).

I'm increasingly convinced that it is impossible to support re-entrance once a failure occurs in LLD, even if that currently works on the surface.

While debugging an intermittent failure on a bot, I recalled this scenario which causes the issue:

1. When executing lld/test/ELF/invalid/symtab-sh-info.s L45, we reach lld::elf::ObjFile::ObjFile() which goes straight into its base ELFFileBase(), then ELFFileBase::init().
2. At that point fatal() is thrown in lld/ELF/InputFiles.cpp L381, leaving a half-initialized ObjFile instance.
3. We end up in lld::exitLld() and since we are running with LLD_IN_TEST, we hapily restore the control flow to CrashRecoveryContext::RunSafely() then back in lld::safeLldMain().
4. Before this patch, we called errorHandler().reset() just after, and this attempted to reset the associated SpecificAlloc<ObjFile<ELF64LE>>. That tried to free the half-initialized ObjFile instance, and more precisely its ObjFile::dwarf member.

Sometimes that worked, sometimes it failed and was catched by the CrashRecoveryContext. This scenario was the reason we called errorHandler().reset() through a CrashRecoveryContext.

But in some rare cases, the above repro somehow corrupted the heap, creating a stack overflow. When the CrashRecoveryContext's filter (that is, __except (ExceptionFilter(GetExceptionInformation()))) tried to handle the exception, it crashed again since the stack was exhausted -- and that took the whole application down. That is the issue seen on the bot. Locally it happens about 1 times out of 15.

Now this situation can happen anywhere in LLD. Since catching stack overflows is not a reliable scenario when using CrashRecoveryContext, we're now preventing further re-entrance when such failures occur, by signaling lld::SafeReturn::canRunAgain=false. When running with LLD_IN_TEST=2, only one iteration will be executed in this case.

[aganea]

Diff with context.

[rnk]

I think I'm OK with the functional change, but I think I would prefer not to keep the code that buffers stdout/stderr for the lit test. I think the complexity/size cost outweighs the test benefit.

[MaskRay]

Since catching stack overflows is not a reliable scenario when using CrashRecoveryContext

I have seen this in a language server. It seems that CrashRecoveryContext does not (correctly) use sigaltstack(). With sigaltstack, we can recover after a stack overflow.
For the stack overflow you observed, how did that happen?

In D88348#2299551, @rnk wrote:

I think I'm OK with the functional change, but I think I would prefer not to keep the code that buffers stdout/stderr for the lit test. I think the complexity/size cost outweighs the test benefit.

I share the same feeling. Hope bufferedStdout can be avoided

llvm/lib/Support/CrashRecoveryContext.cpp

447

This deserves a comment

llvm/lib/Support/Process.cpp

24

_Exit does not require unistd.h

[aganea]

In D88348#2299620, @MaskRay wrote:

I have seen this in a language server. It seems that CrashRecoveryContext does not (correctly) use sigaltstack(). With sigaltstack, we can recover after a stack overflow.

Good to know at least it could work on Linux. I wanted to activate coverage for this at some point in D74229 but then it wasn't working on Debug builds on Windows. I'll need to get back to it.

For the stack overflow you observed, how did that happen?

The stack overflow happens in the first place somewhere in the Windows CRT while releasing heap blocks. This is rare, as it depends on the random pattern that was in memory at the location of ObjFile::dwarf when fatal() was called by ELFFileBase::init().

In D88348#2299551, @rnk wrote:

I think I'm OK with the functional change, but I think I would prefer not to keep the code that buffers stdout/stderr for the lit test. I think the complexity/size cost outweighs the test benefit.

I share the same feeling. Hope bufferedStdout can be avoided

I'll try to find another way. The issue is that we're blocking stdout/stderr on all iterations, but the last one. Which doesn't work if fatal() occurs in the first iteration (as expected, in a test). Perhaps I could simply set LLD_IN_TEST=1 for tests that expect fatal() or a crash.

[aganea]

Address comments. Removed stdout/stderr buffering, and replaced with env LLD_IN_TEST=1 set appropriately in tests, where needed.

[aganea]

Ping! Any further comments?

[rnk]

The code changes seem fine, but I haven't figured out why we need to sprinkle LLD_IN_TEST=1 for every LLD invocation we expect to error. What goes wrong exactly? I thought the point of your change is that, we *won't* re-run LLD when LLD_IN_TEST=2 after it reports an error.

[aganea]

In D88348#2328425, @rnk wrote:

The code changes seem fine, but I haven't figured out why we need to sprinkle LLD_IN_TEST=1 for every LLD invocation we expect to error. What goes wrong exactly? I thought the point of your change is that, we *won't* re-run LLD when LLD_IN_TEST=2 after it reports an error.

When LLD_IN_TEST>0 we only emit stdout/stderr on the last iteration. But if we crash, FileCheck requires for stout & stderr to be emitted before the crash. That was the point for buffering the streams in the previous version of this patch. Either that, or LLD_IN_TEST=1 below. If you think of other ideas, I would be willing to try!

[aganea]

Ping.

I'd like to fix the lld.ELF/invalid::symtab-sh-info.s that fails for a while, and this patch has the fix.

If you think this whole LLD-as-a-library should go into a different direction please let me know.

[MaskRay]

Have the stdout/stderr changes be removed? The updated diff looks good to me. I agree that in the absence of exceptions (proper stack unwinding), we cannot safely recover from fatal(). For library use cases, if they can ensure no fatal() is called, they are probably still safe.

Marking LLD_IN_TEST=1 seems fine. It serves as a hint that abort() is called. LG with some nits, though I still hope @rnk or @amccarth can take a look.

llvm/lib/Support/CrashRecoveryContext.cpp

445

Nit: newer functions should stick with the naming rule. For Exit, you can keep it capitalized to reduce disruption since you just add a default argument which does not require all call sites to change.

llvm/lib/Support/Process.cpp

99

This needs stdlib.h

[bd1976llvm]

Will this land soon? We are eagerly anticipating its arrival :)

[aganea]

In D88348#2388543, @bd1976llvm wrote:

Will this land soon? We are eagerly anticipating its arrival :)

Yes, today! :) Let me finish a few things, I'll land it around noon EST.

lld/Common/ErrorHandler.cpp

 }
 
 raw_ostream &lld::outs() {
-  if (errorHandler().disableOutput)
-    return llvm::nulls();
+  if (errorHandler().disableOutput) {
+    static raw_string_ostream buf(errorHandler().bufferedStdout);
+    return buf;
+  }
   return stdoutOS ? *stdoutOS : llvm::outs();
 }
 
 raw_ostream &lld::errs() {
-  if (errorHandler().disableOutput)
-    return llvm::nulls();
+  if (errorHandler().disableOutput) {
+    static raw_string_ostream buf(errorHandler().bufferedStderr);
+    return buf;
+  }
   return stderrOS ? *stderrOS : llvm::errs();
 }
 

   if (errorHandler().outputBuffer)
     errorHandler().outputBuffer->discard();
 
+  // Emit stdout and stderr if they were hidden. This is only for testing.
+  if (errorHandler().disableOutput) {
+    errorHandler().disableOutput = false;
+    std::lock_guard<std::mutex> lock(mu);
+    lld::outs() << errorHandler().bufferedStdout;
+    lld::errs() << errorHandler().bufferedStderr;
+  }
+
+  // Re-throw a possible signal or exception once/if it was catched by
+  // safeLldMain().
+  CrashRecoveryContext::ThrowIfCrash(val);
+
   // Dealloc/destroy ManagedStatic variables before calling _exit().
   // In an LTO build, allows us to get the output of -time-passes.
   // Ensures that the thread pool for the parallel algorithms is stopped to

     lld::outs().flush();
     lld::errs().flush();
   }
-  llvm::sys::Process::Exit(val);
+  // When running inside safeLldMain(), this call restores the control flow back
+  // to the CrashRecoveryContext. Otherwise we simply use _exit(), meanning no
+  // cleanup, since we want to avoid further crashes on shutdown.
+  llvm::sys::Process::Exit(val, /*NoCleanup=*/true);
 }
 
 void lld::diagnosticHandler(const DiagnosticInfo &di) {

lld/include/lld/Common/Driver.h

 // Generic entry point when using LLD as a library, safe for re-entry, supports
 // crash recovery. Returns a general completion code and a boolean telling
 // whether it can be called again. In some cases, a crash could corrupt memory
-// and re-entry would not be possible anymore.
+// and re-entry would not be possible anymore. Use exitLld() in that case to
+// properly exit your application and avoid intermittent crashes on exit caused
+// by cleanup.
 SafeReturn safeLldMain(int argc, const char **argv, llvm::raw_ostream &stdoutOS,
                        llvm::raw_ostream &stderrOS);
 

lld/include/lld/Common/ErrorHandler.h

   bool vsDiagnostics = false;
   bool disableOutput = false;
   std::function<void()> cleanupCallback;
+  // When the output is disabled, we cache stdout/stderr in these buffers
+  // instead, to emit later if necessary. One such example is if LLD_IN_TEST=2
+  // and the first run fails with fatal() we still want to emit the output. This
+  // is purely for the lit testing.
+  std::string bufferedStdout;
+  std::string bufferedStderr;
 
   void error(const Twine &msg);
   LLVM_ATTRIBUTE_NORETURN void fatal(const Twine &msg);

lld/tools/lld/lld.cpp

     if (!crc.RunSafely([&]() {
           r = lldMain(argc, argv, stdoutOS, stderrOS, /*exitEarly=*/false);
         }))
-      r = crc.RetCode;
+      return {crc.RetCode, /*canRunAgain=*/false};
   }
 
   // Cleanup memory and reset everything back in pristine condition. This path

     // Execute one iteration.
     auto r = safeLldMain(argc, argv, llvm::outs(), llvm::errs());
     if (!r.canRunAgain)
-      _exit(r.ret); // Exit now, can't re-execute again.
+      exitLld(r.ret); // Exit now, can't re-execute again.
 
     if (!mainRet) {
       mainRet = r.ret;

       return r.ret;
     }
   }
-#if LLVM_ON_UNIX
-  // Re-throw the signal so it can be caught by WIFSIGNALED in
-  // llvm/lib/Support/Unix/Program.inc. This is required to correctly handle
-  // usages of `not --crash`.
-  if (*mainRet > 128) {
-    llvm::sys::unregisterHandlers();
-    raise(*mainRet - 128);
-  }
-#endif
   return *mainRet;
 }

llvm/include/llvm/Support/CrashRecoveryContext.h

   LLVM_ATTRIBUTE_NORETURN
   void HandleExit(int RetCode);
 
+  /// Throw again a signal or an exception, after it was catched once.
+  static bool ThrowIfCrash(int RetCode);
+
   /// In case of a crash, this is the crash identifier.
   int RetCode = 0;
 

llvm/include/llvm/Support/Process.h

   /// Equivalent to ::exit(), except when running inside a CrashRecoveryContext.
   /// In that case, the control flow will resume after RunSafely(), like for a
   /// crash, rather than exiting the current process.
+  /// Use \arg NoCleanup for calling _exit() instead of exit().
   LLVM_ATTRIBUTE_NORETURN
-  static void Exit(int RetCode);
+  static void Exit(int RetCode, bool NoCleanup = false);
 };
 
 }

llvm/lib/Support/CrashRecoveryContext.cpp

   llvm_unreachable("Most likely setjmp wasn't called!");
 }
 
+bool CrashRecoveryContext::ThrowIfCrash(int RetCode) {
+#if defined(_WIN32)
+  unsigned Code = ((unsigned)RetCode & 0xF0000000) >> 28;
+  if (Code != 0xC && Code != 8)
+    return false;
+  ::RaiseException(RetCode, 0, 0, NULL);
+#else
+  if (RetCode <= 128)
+    return false;
+  llvm::sys::unregisterHandlers();
+  raise(RetCode - 128);
+#endif
+  return true;
+}
+
 // FIXME: Portability.
 static void setThreadBackgroundPriority() {
 #ifdef __APPLE__

llvm/lib/Support/Process.cpp

 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 
+#if LLVM_ON_UNIX
+#include <unistd.h> // _exit
+#endif
+
 using namespace llvm;
 using namespace sys;
 

 bool Process::AreCoreFilesPrevented() { return coreFilesPrevented; }
 
 LLVM_ATTRIBUTE_NORETURN
-void Process::Exit(int RetCode) {
+void Process::Exit(int RetCode, bool NoCleanup) {
   if (CrashRecoveryContext *CRC = CrashRecoveryContext::GetCurrent())
     CRC->HandleExit(RetCode);
-  ::exit(RetCode);
+
+  if (NoCleanup)
+    _exit(RetCode);
+  else
+    ::exit(RetCode);
 }
 
 // Include the platform-specific parts of this class.