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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: dexonsmith.

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.



================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:445
 
+bool CrashRecoveryContext::ThrowIfCrash(int RetCode) {
+#if defined(_WIN32)
----------------
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.


================
Comment at: llvm/lib/Support/Process.cpp:99
+  if (NoCleanup)
+    _Exit(RetCode);
+  else
----------------
This needs stdlib.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88348