Fix PR44093: use of stale error stream when linking

[thopre]

Summary

llvm::lld::link() calls llvm::lld::enableColors() before initializing
the streams llvm::lld::stdoutOS and llvm::lld::stderrOS. This is a
problem because enableColors() uses stderrOS indirectly via a call to
lld::errs(). It can lead to random behavior if the lifetime of the
object passed as stderrOS in link() expires before a subsequent
invocation of link().

This commit remove enableColors() altogether and replace it by calls to
enable_colors(), making it explicit on which stream to enable colors.
link() is then changed to invoke enable_colors() on stderrOS.

[ruiu]

lld/COFF/Driver.cpp

76

Let's remove enableColors and replace it with stderrOS..enable_colors(stderrOS.has_colors()). I wrote that single-line function but in hindsight it doesn't provide much value.

[thopre]

Remove enableColors and replace calls to it by the underlying enable_colors()

[ruiu]

James Knight has submitted a very similar change to fix the problem.

[thopre]

In D70507#1756181, @ruiu wrote:

James Knight has submitted a very similar change to fix the problem.

Indeed, awesome.

lld/COFF/Driver.cpp

 
 bool link(ArrayRef<const char *> args, bool canExitEarly, raw_ostream &stdoutOS,
           raw_ostream &stderrOS) {
   errorHandler().logName = args::getFilenameWithoutExe(args[0]);
   errorHandler().errorLimitExceededMsg =
       "too many errors emitted, stopping now"
       " (use /errorlimit:0 to see all errors)";
   errorHandler().exitEarly = canExitEarly;
-  enableColors(stderrOS.has_colors());
+  stderrOS.enable_colors(stderrOS.has_colors());
 
   lld::stdoutOS = &stdoutOS;
   lld::stderrOS = &stderrOS;
 
   config = make<Configuration>();
   symtab = make<SymbolTable>();
   driver = make<LinkerDriver>();
 
   driver->link(args);
 
   // Call exit() if we can to avoid calling destructors.
   if (canExitEarly)

lld/COFF/DriverUtils.cpp

 // Set color diagnostics according to --color-diagnostics={auto,always,never}
 // or --no-color-diagnostics flags.
 static void handleColorDiagnostics(opt::InputArgList &args) {
   auto *arg = args.getLastArg(OPT_color_diagnostics, OPT_color_diagnostics_eq,
                               OPT_no_color_diagnostics);
   if (!arg)
     return;
   if (arg->getOption().getID() == OPT_color_diagnostics) {
-    enableColors(true);
+    lld::errs().enable_colors(true);
   } else if (arg->getOption().getID() == OPT_no_color_diagnostics) {
-    enableColors(false);
+    lld::errs().enable_colors(false);
   } else {
     StringRef s = arg->getValue();
     if (s == "always")
-      enableColors(true);
+      lld::errs().enable_colors(true);
     else if (s == "never")
-      enableColors(false);
+      lld::errs().enable_colors(false);
     else if (s != "auto")
       error("unknown option: --color-diagnostics=" + s);
   }
 }
 
 static cl::TokenizerCallback getQuotingStyle(opt::InputArgList &args) {
   if (auto *arg = args.getLastArg(OPT_rsp_quoting)) {
     StringRef s = arg->getValue();

lld/Common/ErrorHandler.cpp

 raw_ostream &lld::outs() { return stdoutOS ? *stdoutOS : llvm::outs(); }
 raw_ostream &lld::errs() { return stderrOS ? *stderrOS : llvm::errs(); }
 
 ErrorHandler &lld::errorHandler() {
   static ErrorHandler handler;
   return handler;
 }
 
-void lld::enableColors(bool enable) { lld::errs().enable_colors(enable); }
-
 void lld::exitLld(int val) {
   // Delete any temporary file, while keeping the memory mapping open.
   if (errorHandler().outputBuffer)
     errorHandler().outputBuffer->discard();
 
   // Dealloc/destroy ManagedStatic variables before calling
   // _exit(). In a non-LTO build, this is a nop. In an LTO
   // build allows us to get the output of -time-passes.

lld/ELF/Driver.cpp

 
 bool link(ArrayRef<const char *> args, bool canExitEarly, raw_ostream &stdoutOS,
           raw_ostream &stderrOS) {
   errorHandler().logName = args::getFilenameWithoutExe(args[0]);
   errorHandler().errorLimitExceededMsg =
       "too many errors emitted, stopping now (use "
       "-error-limit=0 to see all errors)";
   errorHandler().exitEarly = canExitEarly;
-  enableColors(stderrOS.has_colors());
+  stderrOS.enable_colors(stderrOS.has_colors());
 
   lld::stdoutOS = &stdoutOS;
   lld::stderrOS = &stderrOS;
 
   inputSections.clear();
   outputSections.clear();
   binaryFiles.clear();
   bitcodeFiles.clear();

lld/ELF/DriverUtils.cpp

 // Set color diagnostics according to -color-diagnostics={auto,always,never}
 // or -no-color-diagnostics flags.
 static void handleColorDiagnostics(opt::InputArgList &args) {
   auto *arg = args.getLastArg(OPT_color_diagnostics, OPT_color_diagnostics_eq,
                               OPT_no_color_diagnostics);
   if (!arg)
     return;
   if (arg->getOption().getID() == OPT_color_diagnostics) {
-    enableColors(true);
+    lld::errs().enable_colors(true);
   } else if (arg->getOption().getID() == OPT_no_color_diagnostics) {
-    enableColors(false);
+    lld::errs().enable_colors(false);
   } else {
     StringRef s = arg->getValue();
     if (s == "always")
-      enableColors(true);
+      lld::errs().enable_colors(true);
     else if (s == "never")
-      enableColors(false);
+      lld::errs().enable_colors(false);
     else if (s != "auto")
       error("unknown option: --color-diagnostics=" + s);
   }
 }
 
 static cl::TokenizerCallback getQuotingStyle(opt::InputArgList &args) {
   if (auto *arg = args.getLastArg(OPT_rsp_quoting)) {
     StringRef s = arg->getValue();

lld/MinGW/Driver.cpp

searchLibrary(StringRef name, ArrayRef<StringRef> searchPaths, bool bStatic) {
   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, bool canExitEarly,
                  raw_ostream &stdoutOS, raw_ostream &stderrOS) {
-  enableColors(stderrOS.has_colors());
+  stderrOS.enable_colors(stderrOS.has_colors());
   lld::stdoutOS = &stdoutOS;
   lld::stderrOS = &stderrOS;
 
   MinGWOptTable parser;
   opt::InputArgList args = parser.parse(argsArr.slice(1));
 
   if (errorCount())
     return false;

lld/include/lld/Common/ErrorHandler.h

private:
   using Colors = raw_ostream::Colors;
 
   std::string getLocation(const Twine &msg);
 };
 
 /// Returns the default error handler.
 ErrorHandler &errorHandler();
 
-void enableColors(bool enable);
-
 inline void error(const Twine &msg) { errorHandler().error(msg); }
 inline LLVM_ATTRIBUTE_NORETURN void fatal(const Twine &msg) {
   errorHandler().fatal(msg);
 }
 inline void log(const Twine &msg) { errorHandler().log(msg); }
 inline void message(const Twine &msg) { errorHandler().message(msg); }
 inline void warn(const Twine &msg) { errorHandler().warn(msg); }
 inline uint64_t errorCount() { return errorHandler().errorCount; }

lld/lib/Driver/DarwinLdDriver.cpp

 /// This is where the link is actually performed.
 bool link(llvm::ArrayRef<const char *> args, bool CanExitEarly,
           raw_ostream &StdoutOS, raw_ostream &StderrOS) {
   errorHandler().logName = args::getFilenameWithoutExe(args[0]);
   errorHandler().errorLimitExceededMsg =
       "too many errors emitted, stopping now (use "
       "'-error-limit 0' to see all errors)";
   errorHandler().exitEarly = CanExitEarly;
-  enableColors(StderrOS.has_colors());
+  StderrOS.enable_colors(StderrOS.has_colors());
 
   lld::stdoutOS = &StdoutOS;
   lld::stderrOS = &StderrOS;
 
   MachOLinkingContext ctx;
   if (!parse(args, ctx))
     return false;
   if (ctx.doNothing())

lld/wasm/Driver.cpp

 } // anonymous namespace
 
 bool link(ArrayRef<const char *> args, bool canExitEarly, raw_ostream &stdoutOS,
           raw_ostream &stderrOS) {
   errorHandler().logName = args::getFilenameWithoutExe(args[0]);
   errorHandler().errorLimitExceededMsg =
       "too many errors emitted, stopping now (use "
       "-error-limit=0 to see all errors)";
-  enableColors(stderrOS.has_colors());
+  stderrOS.enable_colors(stderrOS.has_colors());
 
   lld::stdoutOS = &stdoutOS;
   lld::stderrOS = &stderrOS;
 
   config = make<Configuration>();
   symtab = make<SymbolTable>();
 
   initLLVM();

 // Set color diagnostics according to -color-diagnostics={auto,always,never}
 // or -no-color-diagnostics flags.
 static void handleColorDiagnostics(opt::InputArgList &args) {
   auto *arg = args.getLastArg(OPT_color_diagnostics, OPT_color_diagnostics_eq,
                               OPT_no_color_diagnostics);
   if (!arg)
     return;
   if (arg->getOption().getID() == OPT_color_diagnostics) {
-    enableColors(true);
+    lld::errs().enable_colors(true);
   } else if (arg->getOption().getID() == OPT_no_color_diagnostics) {
-    enableColors(false);
+    lld::errs().enable_colors(false);
   } else {
     StringRef s = arg->getValue();
     if (s == "always")
-      enableColors(true);
+      lld::errs().enable_colors(true);
     else if (s == "never")
-      enableColors(false);
+      lld::errs().enable_colors(false);
     else if (s != "auto")
       error("unknown option: --color-diagnostics=" + s);
   }
 }
 
 // Find a file by concatenating given paths.
 static Optional<std::string> findFile(StringRef path1, const Twine &path2) {
   SmallString<128> s;