[lld] Destroy CommonLinkerContext inside lld::*::link after D108850

[MaskRay]

Summary

Close https://github.com/llvm/llvm-project/issues/59874

D108850 changes ports to allocate CommonLinkerContext on the heap and destroys
lctx in lld/tools/lld/lld.cpp:lldMain. For API users of lld::*::link, they
need to manually call CommonLinkerContext::destroy() to prevent states from a
previous invocation. This is not ergonomic.

This patch moves CommonLinkerContext destruction into lld::*::link by using
local variables. COFFLinkerContext consumes ~5KiB stack space but it should be
fine.

Only one CommonLinkerContext::destroy() remains: in safeLldMain for
destruction after longjmp in the presence of a fatal error.

[aganea]

The OP in PR59874 could use safeLldMain instead of lld::elf::link and that will solve their issue (but lld.cpp.obj has to be linked in). With D119049 we should probably decide if we want to expose the naked lld::*::link functions outside of LLD, or if users should always use safeLldMain, or something else.

The reason for not allocating ctx on the stack is to survive calls to lld::error() in LLD-as-lib scenarios. lld::error() calls lld::exitLld() which calls llvm::Process::Exit(), which, if a llvm::CrashRecoveryContext exists on the stack, restores the flow back in lld::safeLldMain. As soon as longjmp jumps back into safeLldMain, the stack below it becomes invalid in theory. At that point it is not safe anymore to call CommonLinkerContext::destroy(), right? (even if that could work in some implementations).

[aganea]

In D142949#4093346, @aganea wrote:

The reason for not allocating ctx on the stack is to survive calls to lld::error() in LLD-as-lib scenarios.

Just another quick note -- in theory we can survive crashes as well here. But there's at least a case in lldELF which throws an error() inside a constructor, and that corrupts the heap on ctx deletion, on Windows at least, see rG45b8a741fbbf271e0fb71294cb7cdce3ad4b9bf3

[MaskRay]

Requiring safeLldMain looks good to me as well. Perhaps we should just remove declarations of lld::*::link.

[MaskRay]

In D142949#4093397, @aganea wrote:

In D142949#4093346, @aganea wrote:

The reason for not allocating ctx on the stack is to survive calls to lld::error() in LLD-as-lib scenarios.

Just another quick note -- in theory we can survive crashes as well here. But there's at least a case in lldELF which throws an error() inside a constructor, and that corrupts the heap on ctx deletion, on Windows at least, see rG45b8a741fbbf271e0fb71294cb7cdce3ad4b9bf3

Do you happen to have more information about which specific line may trigger a problem on Windows? Does this patch regress it?

[aganea]

In D142949#4094414, @MaskRay wrote:

Do you happen to have more information about which specific line may trigger a problem on Windows? Does this patch regress it?

The previous issue was caused by this call to fatal but you fixed it in rGd9dbf9e30a581fcadd667b6d8e5827a4003b85a2, see problem description in rG45b8a741fbbf.
My whole point is that we don't need this current patch, users can just call lld::safeLldMain().

[bjoo]

I will try that

[MaskRay]

In D142949#4094729, @aganea wrote:

In D142949#4094414, @MaskRay wrote:

Do you happen to have more information about which specific line may trigger a problem on Windows? Does this patch regress it?

The previous issue was caused by this call to fatal but you fixed it in rGd9dbf9e30a581fcadd667b6d8e5827a4003b85a2, see problem description in rG45b8a741fbbf.
My whole point is that we don't need this current patch, users can just call lld::safeLldMain().

How is auto *ctx = new CommonLinkerContext (with a later cleanup) different from an on-stack COFFLinkerContext ctx; whose destructor is not called?

[aganea]

In D142949#4098579, @MaskRay wrote:

How is auto *ctx = new CommonLinkerContext (with a later cleanup) different from an on-stack COFFLinkerContext ctx; whose destructor is not called?

How is the destructor not called in this patch? ctx on the stack in this patch will call ~CommonLinkerContext() and the would destroy all the instances, the string saver, the bump allocator, etc. We want to avoid that in the "normal" scenario, for a quick exit.

You're absolutely right in the sense that as code stands right now, ctx could very well live on the stack. But since your fix rGd9dbf9e30a58, we could revert rG45b8a741fbbf and catch fatal() and exceptions in LLD-as-lib usages. Do we want that? Or do we keep assuming that on the first fatal error, we return SafeReturn { ... canRunAgain = false } to force the enclosing app. to exit? I'd prefer something more resilient if possible (that is, revert rG45b8a741fbbf and not commit this patch).

[MaskRay]

In D142949#4105043, @aganea wrote:

In D142949#4098579, @MaskRay wrote:

How is auto *ctx = new CommonLinkerContext (with a later cleanup) different from an on-stack COFFLinkerContext ctx; whose destructor is not called?

How is the destructor not called in this patch? ctx on the stack in this patch will call ~CommonLinkerContext() and the would destroy all the instances, the string saver, the bump allocator, etc. We want to avoid that in the "normal" scenario, for a quick exit.

You're absolutely right in the sense that as code stands right now, ctx could very well live on the stack. But since your fix rGd9dbf9e30a58, we could revert rG45b8a741fbbf and catch fatal() and exceptions in LLD-as-lib usages. Do we want that? Or do we keep assuming that on the first fatal error, we return SafeReturn { ... canRunAgain = false } to force the enclosing app. to exit? I'd prefer something more resilient if possible (that is, revert rG45b8a741fbbf and not commit this patch).

Reverting 45b8a741fbbf271e0fb71294cb7cdce3ad4b9bf3 seems reasonable. I need to play more with your another patch...

[MaskRay]

Abandon after D119049

lld/COFF/Driver.cpp

 using namespace llvm::object;
 using namespace llvm::COFF;
 using namespace llvm::sys;
 
 namespace lld::coff {
 
 bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
           llvm::raw_ostream &stderrOS, bool exitEarly, bool disableOutput) {
-  // This driver-specific context will be freed later by lldMain().
-  auto *ctx = new COFFLinkerContext;
-
-  ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
-  ctx->e.logName = args::getFilenameWithoutExe(args[0]);
-  ctx->e.errorLimitExceededMsg = "too many errors emitted, stopping now"
-                                 " (use /errorlimit:0 to see all errors)";
+  COFFLinkerContext ctx;
+  ctx.e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
+  ctx.e.logName = args::getFilenameWithoutExe(args[0]);
+  ctx.e.errorLimitExceededMsg = "too many errors emitted, stopping now"
 
-  ctx->driver.linkerMain(args);
+  ctx.driver.linkerMain(args);
 
   return errorCount() == 0;
 }
 
 // Parse options of the form "old;new".
 static std::pair<StringRef, StringRef> getOldNewOptions(opt::InputArgList &args,
                                                         unsigned id) {
   auto *arg = args.getLastArg(id);

lld/ELF/Driver.cpp

void Ctx::reset() {
   backwardReferences.clear();
   hasSympart.store(false, std::memory_order_relaxed);
   needsTlsLd.store(false, std::memory_order_relaxed);
 }
 
 bool elf::link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
                llvm::raw_ostream &stderrOS, bool exitEarly,
                bool disableOutput) {
-  // This driver-specific context will be freed later by lldMain().
-  auto *ctx = new CommonLinkerContext;
-
-  ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
-  ctx->e.cleanupCallback = []() {
+  CommonLinkerContext ctx;
+  ctx.e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
+  ctx.e.cleanupCallback = []() {
     elf::ctx.reset();
     symtab = SymbolTable();
 
     outputSections.clear();
     symAux.clear();
 
     tar = nullptr;
     in.reset();
 
     partitions.clear();
     partitions.emplace_back();
 
     SharedFile::vernauxNum = 0;
   };
-  ctx->e.logName = args::getFilenameWithoutExe(args[0]);
-  ctx->e.errorLimitExceededMsg = "too many errors emitted, stopping now (use "
-                                 "--error-limit=0 to see all errors)";
+  ctx.e.logName = args::getFilenameWithoutExe(args[0]);
+  ctx.e.errorLimitExceededMsg = "too many errors emitted, stopping now (use "
 
   config = ConfigWrapper();
   script = std::make_unique<LinkerScript>();
 
   symAux.emplace_back();
 
   partitions.clear();
   partitions.emplace_back();

lld/MachO/Driver.cpp

parallelForEach(symtab->getSymbols(), [](Symbol *sym) {
           defined->privateExtern = true;
     });
   }
 }
 
 bool macho::link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
                  llvm::raw_ostream &stderrOS, bool exitEarly,
                  bool disableOutput) {
-  // This driver-specific context will be freed later by lldMain().
-  auto *ctx = new CommonLinkerContext;
-
-  ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
-  ctx->e.cleanupCallback = []() {
+  CommonLinkerContext ctx;
+  ctx.e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
+  ctx.e.cleanupCallback = []() {
     resolvedFrameworks.clear();
     resolvedLibraries.clear();
     cachedReads.clear();
     concatOutputSections.clear();
     inputFiles.clear();
     inputSections.clear();
     loadedArchives.clear();
     loadedObjectFrameworks.clear();
     missingAutolinkWarnings.clear();
     syntheticSections.clear();
     thunkMap.clear();
 
     firstTLVDataSection = nullptr;
     tar = nullptr;
     memset(&in, 0, sizeof(in));
 
     resetLoadedDylibs();
     resetOutputSegments();
     resetWriter();
     InputFile::resetIdCount();
   };
 
-  ctx->e.logName = args::getFilenameWithoutExe(argsArr[0]);
+  ctx.e.logName = args::getFilenameWithoutExe(argsArr[0]);
 
   MachOOptTable parser;
   InputArgList args = parser.parse(argsArr.slice(1));
 
-  ctx->e.errorLimitExceededMsg = "too many errors emitted, stopping now "
-                                 "(use --error-limit=0 to see all errors)";
-  ctx->e.errorLimit = args::getInteger(args, OPT_error_limit_eq, 20);
-  ctx->e.verbose = args.hasArg(OPT_verbose);
+  ctx.e.errorLimitExceededMsg = "too many errors emitted, stopping now "
+  ctx.e.errorLimit = args::getInteger(args, OPT_error_limit_eq, 20);
+  ctx.e.verbose = args.hasArg(OPT_verbose);
 
   if (args.hasArg(OPT_help_hidden)) {
     parser.printHelp(argsArr[0], /*showHidden=*/true);
     return true;
   }
   if (args.hasArg(OPT_help)) {
     parser.printHelp(argsArr[0], /*showHidden=*/false);
     return true;

lld/MinGW/Driver.cpp

if (!bStatic) {
       if (std::optional<std::string> s = findFile(dir, name + ".dll"))
         return *s;
     }
   }
   error("unable to find library -l" + name);
   return "";
 }
 
-// Convert Unix-ish command line arguments to Windows-ish ones and
-// then call coff::link.
-bool mingw::link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
-                 llvm::raw_ostream &stderrOS, bool exitEarly,
-                 bool disableOutput) {
-  auto *ctx = new CommonLinkerContext;
-  ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
+static std::optional<bool> translate(ArrayRef<const char *> argsArr,
+                                     llvm::raw_ostream &stdoutOS,
+                                     llvm::raw_ostream &stderrOS,
+                                     bool exitEarly, bool disableOutput,
+                                     std::vector<const char *> &vec) {
+  CommonLinkerContext ctx;
+  ctx.e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
 
   MinGWOptTable parser;
   opt::InputArgList args = parser.parse(argsArr.slice(1));
 
   if (errorCount())
     return false;
 
   if (args.hasArg(OPT_help)) {
static std::optional<bool> translate(ArrayRef<const char *> argsArr,
 
   if (args.hasArg(OPT_verbose) || args.hasArg(OPT__HASH_HASH_HASH))
     lld::errs() << llvm::join(linkArgs, " ") << "\n";
 
   if (args.hasArg(OPT__HASH_HASH_HASH))
     return true;
 
   // Repack vector of strings to vector of const char pointers for coff::link.
-  std::vector<const char *> vec;
   for (const std::string &s : linkArgs)
     vec.push_back(s.c_str());
   // Pass the actual binary name, to make error messages be printed with
   // the right prefix.
   vec[0] = argsArr[0];
+  return std::nullopt;
+}
 
-  // The context will be re-created in the COFF driver.
-  lld::CommonLinkerContext::destroy();
-
+// Convert Unix-ish command line arguments to Windows-ish ones and
+// then call coff::link.
+bool mingw::link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
+                 llvm::raw_ostream &stderrOS, bool exitEarly,
+                 bool disableOutput) {
+  std::vector<const char *> vec;
+  if (std::optional<bool> ret =
+          translate(argsArr, stdoutOS, stderrOS, exitEarly, disableOutput, vec))
+    return *ret;
   return coff::link(vec, stdoutOS, stderrOS, exitEarly, disableOutput);
 }

lld/tools/lld/lld.cpp

static int lldMain(int argc, const char **argv, llvm::raw_ostream &stdoutOS,
   }();
   // Run the driver. If an error occurs, false will be returned.
   bool r = link(args, stdoutOS, stderrOS, exitEarly, inTestOutputDisabled);
 
   // Call exit() if we can to avoid calling destructors.
   if (exitEarly)
     exitLld(!r ? 1 : 0);
 
-  // Delete the global context and clear the global context pointer, so that it
-  // cannot be accessed anymore.
-  CommonLinkerContext::destroy();
-
   return !r ? 1 : 0;
 }
 
 // Similar to lldMain except that exceptions are caught.
 SafeReturn lld::safeLldMain(int argc, const char **argv,
                             llvm::raw_ostream &stdoutOS,
                             llvm::raw_ostream &stderrOS) {
   int r = 0;

lld/wasm/Driver.cpp

private:
   bool inWholeArchive = false;
 
   std::vector<InputFile *> files;
 };
 } // anonymous namespace
 
 bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
           llvm::raw_ostream &stderrOS, bool exitEarly, bool disableOutput) {
-  // This driver-specific context will be freed later by lldMain().
-  auto *ctx = new CommonLinkerContext;
-
-  ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
-  ctx->e.logName = args::getFilenameWithoutExe(args[0]);
-  ctx->e.errorLimitExceededMsg = "too many errors emitted, stopping now (use "
-                                 "-error-limit=0 to see all errors)";
+  CommonLinkerContext ctx;
+  ctx.e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
+  ctx.e.logName = args::getFilenameWithoutExe(args[0]);
+  ctx.e.errorLimitExceededMsg = "too many errors emitted, stopping now (use "
 
   config = make<Configuration>();
   symtab = make<SymbolTable>();
 
   initLLVM();
   LinkerDriver().linkerMain(args);
 
   return errorCount() == 0;