[COFF] add /errorrepro to save reproducer on error

[inglorion]

Summary

This adds an /errorrepro:<dir> flag to lld-link. The flag
instructs the linker to write a repro.tar file in the specified
directory if linking fails. The functionality is modeled after
/linkrepro, which unconditionally writes the reproducer.

[ruiu]

What was the motivation for this new option? Given that it is easy to write run the same command again with /linkrepro, I don't see an immediate need to add this as a new option.

[inglorion]

I am investigating https://bugs.chromium.org/p/chromium/issues/detail?id=786127, which is a failure that does not reproduce reliably. I could use /linkrepro to save all link inputs always, but that would use up a lot of disk space. Running the command again with /linkrepro will almost certainly not reproduce the failure. Having a way to only save the reproducer when an error occurs would help here, and also seems a generally useful feature to have.

[ruiu]

lld/COFF/Driver.cpp

58

Is this really called when lld exits? I think by default lld calls _exit() to avoid the cost of destruction, so dtors are not called on exit.

[pcc]

If the test failure is related to LTO, would this really help? As I understand it, the failures are on the object files created by LTO, which I don't think would appear in the linkrepro file.

I wrote a small patch:

diff --git a/lld/COFF/Chunks.cpp b/lld/COFF/Chunks.cpp
index 6d8a0c7c1f4..2931bc23d21 100644
--- a/lld/COFF/Chunks.cpp
+++ b/lld/COFF/Chunks.cpp
@@ -97,9 +97,11 @@ void SectionChunk::applyRelX64(uint8_t *Off, uint16_t Type, OutputSection *OS,
   case IMAGE_REL_AMD64_REL32_5:  add32(Off, S - P - 9); break;
   case IMAGE_REL_AMD64_SECTION:  applySecIdx(Off, OS); break;
   case IMAGE_REL_AMD64_SECREL:   applySecRel(this, Off, OS, S); break;
-  default:
+  default: {
+    llvm::outs() << toHex(File->MB.getBuffer()) << '\n';
     fatal("unsupported relocation type 0x" + Twine::utohexstr(Type));
   }
+  }
 }

 void SectionChunk::applyRelX86(uint8_t *Off, uint16_t Type, OutputSection *OS,

and used it to build chromium in a loop on my machine overnight. With that I was able to reproduce the problem and capture one of the object files. I'll take a look at it.

[inglorion]

Thanks, @pcc . Out of curiosity, how did you build chrome (I guess I'm mostly interested in args.gn and the command you used to build). I've been trying this on my machine and it can literally go for weeks without hitting the problem.

[inglorion]

lld/COFF/Driver.cpp

58

Good point. In an earlier version I had implemented hooks to be run by exitLld() so that I could write the error repro there. This version was an attempt to simplify that, based on the idea that the destructor for ReproWriter is run before control leaves LinkerDriver::link(). That works fine for some cases, but not cases where we exceed the error limit and ExitEarly is true, or we call fatal(). Let me see if I can come up with a better way.

[pcc]

Here is the args.gn I was using. It's basically the same as the bot:

is_clang = true
is_component_build = false
is_debug = false
is_official_build = true
symbol_level = 2
use_lld = true
use_thin_lto = true

I also had a local patch to chrome to get it to use my hacked lld:

diff --git a/build/toolchain/win/BUILD.gn b/build/toolchain/win/BUILD.gn
index 4d9d1f45f870..8e4a30b223ba 100644
--- a/build/toolchain/win/BUILD.gn
+++ b/build/toolchain/win/BUILD.gn
@@ -109,7 +109,7 @@ template("msvc_toolchain") {
       # lld-link includes a replacement for lib.exe that can produce thin
       # archives and understands bitcode (for lto builds).
       lib = "$prefix/$lld_link /lib /llvmlibthin"
-      link = "$prefix/$lld_link"
+      link = "C:/src/l/ra/bin/lld-link.exe"
       if (host_os != "win") {
         # See comment adding --rsp-quoting to $cl above for more information.
         link = "$link --rsp-quoting=posix"

I built it with this batch file:

for /l %%x in (1, 1, 100) do (
  mkdir llvm_exp%%x
  cd llvm_exp%%x
  copy ..\args.gn .
  gn gen .
  ninja all > ninja.out
  cd ..
)

[inglorion]

lld/COFF/Driver.cpp

58

I think we are guaranteed that we will either leave the scope of LinkerDriver::link(), or we will call exitLld() (there is another case where we die from a signal, but that's not a case I'm looking to address here; we can address that in a later patch if we want to).

This patch as it is already addresses the case of going out of scope, so we need to address the exitLld() case. exitLld() already calls llvm_shutdown(), so we could use ManagedStatic...but I prefer to actually add a way to register hooks for exitLld().

I'll go ahead and add that, unless someone objects.

[inglorion]

Ensure reproducer is also written when calling exitLld().

[inglorion]

Re-upload.

[pcc]

I still don't see a compelling reason to do this to be honest. The use case we had in mind was to help debug an intermittent failure, but as we discovered it wouldn't have helped debug that failure. Also, if a failure is intermittent then it can be reproduced just as much with /errorrepro as with /linkrepro.

[inglorion]

I still don't see a compelling reason to do this to be honest.

Fair enough, and if the consensus is that we'd rather not have this, I'll withdraw it. You've reduced the problem that originally prompted me to write this (and you're right it doesn't capture the object files LTO generates). However, aside from that specific problem, I still think that something like this would be useful to diagnose linker errors we may encounter in the future, particularly ones that don't reproduce reliably. As I pointed out earlier, /linkrepro isn't really what I would want for that because you either need to re-run the link until you hit the same failure again, which could be very time consuming, or pre-emptively run with /linkrepro, which slows down links and writes lots of bytes you usually don't care about.

[pcc]

As I pointed out earlier, /linkrepro isn't really what I would want for that because you either need to re-run the link until you hit the same failure again, which could be very time consuming, or pre-emptively run with /linkrepro, which slows down links and writes lots of bytes you usually don't care about.

You could always rerun the link with /linkrepro if the link fails. Even if the second link succeeds, you still have a reproducer that you can try repeatedly until you hit the failure.

[inglorion]

After checking with ruiu, it seems we are all on the side of not adding this. Abandoning.

lld/COFF/Driver.h

public:
   // Used by ArchiveFile to enqueue members.
   void enqueueArchiveMember(const Archive::Child &C, StringRef SymName,
                             StringRef ParentName);
 
   MemoryBufferRef takeBuffer(std::unique_ptr<MemoryBuffer> MB);
 
 private:
   std::unique_ptr<llvm::TarWriter> Tar; // for /linkrepro
+  std::function<void(StringRef, StringRef)> TakeBufferHook;
 
   // Opens a file. Path has to be resolved already.
   MemoryBufferRef openFile(StringRef Path);
 
   // Searches a file from search paths.
   Optional<StringRef> findFile(StringRef Filename);
   Optional<StringRef> findLib(StringRef Filename);
   StringRef doFindFile(StringRef Filename);

lld/COFF/Driver.cpp

 #include "lld/Common/Args.h"
 #include "lld/Common/Driver.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Exit.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Timer.h"
 #include "lld/Common/Version.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Object/ArchiveWriter.h"
 #include "llvm/Object/COFFImportFile.h"
 #include "llvm/Object/COFFModuleDefinition.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/TarWriter.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/ToolDrivers/llvm-lib/LibDriver.h"
 #include <algorithm>
+#include <atomic>
 #include <future>
 #include <memory>
 
 using namespace llvm;
 using namespace llvm::object;
 using namespace llvm::COFF;
 using llvm::sys::Process;
 
+namespace {
+class ReproWriter {
+public:
+  ReproWriter(StringRef OutputFileName, StringRef Prefix)
+      : OutputFileName(OutputFileName), Prefix(Prefix), NeedsWrite(true) {}
+  ~ReproWriter() {
+    if (!std::atomic_exchange(&NeedsWrite, false) || lld::errorCount() == 0)
+      return;
+    Expected<std::unique_ptr<TarWriter>> Tar =
+        TarWriter::create(OutputFileName, Prefix);
+    if (!Tar) {
+      lld::warn("Could not write reproducer to " + OutputFileName + ": " +
+                toString(Tar.takeError()));
+      return;
+    }
+    for (const auto &I : Files)
+      (*Tar)->append(I.getKey(), I.getValue());
+  }
+  void addBuffer(StringRef Path, StringRef Data) {
+    Files.try_emplace(Path, Data);
+  }
+
+private:
+  std::string OutputFileName;
+  StringRef Prefix;
+  StringMap<StringRef> Files;
+  std::atomic_bool NeedsWrite;
+};
+} // anonymous namespace
+
 namespace lld {
 namespace coff {
 
 static Timer InputFileTimer("Input File Reading", Timer::root());
 
 Configuration *Config;
 LinkerDriver *Driver;
 
return std::async(Strategy, [=]() {
     return MBErrPair{std::move(*MBOrErr), std::error_code()};
   });
 }
 
 MemoryBufferRef LinkerDriver::takeBuffer(std::unique_ptr<MemoryBuffer> MB) {
   MemoryBufferRef MBRef = *MB;
   make<std::unique_ptr<MemoryBuffer>>(std::move(MB)); // take ownership
 
-  if (Driver->Tar)
-    Driver->Tar->append(relativeToRoot(MBRef.getBufferIdentifier()),
+  if (Driver->TakeBufferHook)
+    Driver->TakeBufferHook(relativeToRoot(MBRef.getBufferIdentifier()),
                            MBRef.getBuffer());
+
   return MBRef;
 }
 
 void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> MB,
                              bool WholeArchive) {
   StringRef Filename = MB->getBufferIdentifier();
 
   MemoryBufferRef MBRef = takeBuffer(std::move(MB));

 static std::string createResponseFile(const opt::InputArgList &Args,
                                       ArrayRef<StringRef> FilePaths,
                                       ArrayRef<StringRef> SearchPaths) {
   SmallString<0> Data;
   raw_svector_ostream OS(Data);
 
   for (auto *Arg : Args) {
     switch (Arg->getOption().getID()) {
+    case OPT_errorrepro:
     case OPT_linkrepro:
     case OPT_INPUT:
     case OPT_defaultlib:
     case OPT_libpath:
     case OPT_manifest:
     case OPT_manifest_colon:
     case OPT_manifestdependency:
     case OPT_manifestfile:
for (ObjFile *Obj : ObjFile::Instances) {
     raw_fd_ostream OS(Fd, /*shouldClose*/ true);
     OS << Obj->MB.getBuffer();
     Temps.push_back(S.str());
     Rsp += quote(S) + "\n";
   }
 
   for (auto *Arg : Args) {
     switch (Arg->getOption().getID()) {
+    case OPT_errorrepro:
     case OPT_linkrepro:
     case OPT_lldmap:
     case OPT_lldmap_file:
     case OPT_lldsavetemps:
     case OPT_msvclto:
       // LLD-specific options are stripped.
       break;
     case OPT_opt:
Expected<std::unique_ptr<TarWriter>> ErrOrWriter =
         TarWriter::create(Path, "repro");
 
     if (ErrOrWriter) {
       Tar = std::move(*ErrOrWriter);
     } else {
       error("/linkrepro: failed to open " + Path + ": " +
             toString(ErrOrWriter.takeError()));
     }
+
+    TakeBufferHook = [this](StringRef Path, StringRef Data) {
+      Tar->append(Path, Data);
+    };
+  }
+
+  std::unique_ptr<ReproWriter> ReproWriter;
+  addLldExitHandler([&ReproWriter]() { ReproWriter.reset(); });
+  if (auto *Arg = Args.getLastArg(OPT_errorrepro)) {
+    if (Args.getLastArg(OPT_linkrepro))
+      fatal("/errorrepro must not be specified with /linkrepro");
+    SmallString<64> Path = StringRef(Arg->getValue());
+    sys::path::append(Path, "repro.tar");
+    ReproWriter = make_unique<class ReproWriter>(Path, "repro");
+    TakeBufferHook = [&ReproWriter](StringRef Path, StringRef Data) {
+      ReproWriter->addBuffer(Path, Data);
+    };
   }
 
   if (!Args.hasArg(OPT_INPUT)) {
     if (Args.hasArg(OPT_deffile))
       Config->NoEntry = true;
     else
       fatal("no input files");
   }
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   }
 
   // Input files can be Windows resource files (.res files). We use
   // WindowsResource to convert resource files to a regular COFF file,
   // then link the resulting file normally.
   if (!Resources.empty())
     Symtab->addFile(make<ObjFile>(convertResToCOFF(Resources)));
 
-  if (Tar)
-    Tar->append("response.txt",
-                createResponseFile(Args, FilePaths,
-                                   ArrayRef<StringRef>(SearchPaths).slice(1)));
+  if (TakeBufferHook)
+    TakeBufferHook(
+        "response.txt",
 
   // Handle /largeaddressaware
   Config->LargeAddressAware = Args.hasFlag(
       OPT_largeaddressaware, OPT_largeaddressaware_no, Config->is64());
 
   // Handle /highentropyva
   Config->HighEntropyVA =
       Config->is64() &&

lld/COFF/Options.td

defm tsaware  : B<"tsaware",
                   "Create non-Terminal Server aware executable">;
 
 def help : F<"help">;
 def help_q : Flag<["/?", "-?"], "">, Alias<help>;
 
 // LLD extensions
 def debug_ghash : F<"debug:ghash">;
 def debug_dwarf : F<"debug:dwarf">;
+def errorrepro : P<"errorrepro", "If linking fails, write repro.tar here">;
 def export_all_symbols : F<"export-all-symbols">;
 def kill_at : F<"kill-at">;
 def lldmingw : F<"lldmingw">;
 def msvclto : F<"msvclto">;
 def output_def : Joined<["/", "-"], "output-def:">;
 def rsp_quoting : Joined<["--"], "rsp-quoting=">,
   HelpText<"Quoting style for response files, 'windows' (default) or 'posix'">;
 def dash_dash_version : Flag<["--"], "version">,

lld/test/COFF/errorrepro.test

+# REQUIRES: x86, shell
+
+# RUN: rm -rf %t.dir
+# RUN: yaml2obj < %p/Inputs/hello32.yaml > %t.obj
+
+# RUN: mkdir -p %t.dir/noerrorrepro
+# RUN: cd %t.dir/noerrorrepro
+# RUN: not lld-link %t.obj -entry:main -out:%t.exe
+# RUN: not test -f repro.tar || (echo 'repro.tar should not exist' >&2; false)
+
+# RUN: mkdir -p %t.dir/noerror
+# RUN: cd %t.dir/noerror
+# RUN: lld-link %t.obj %p/Inputs/std32.lib -errorrepro:. -entry:main -out:%t.exe
+# RUN: not test -f repro.tar || (echo 'repro.tar should not exist' >&2; false)
+
+# RUN: mkdir -p %t.dir/errorrepro
+# RUN: cd %t.dir/errorrepro
+# RUN: not lld-link %t.obj -errorrepro:. -entry:main -out:%t.exe
+# RUN: tar xf repro.tar
+# RUN: diff %t.obj repro/%:t.obj
+# RUN: FileCheck %s --check-prefix=RSP < repro/response.txt
+
+# RUN: mkdir -p %t.dir/fatal
+# RUN: cd %t.dir/fatal
+# RUN: not env LLD_IN_TEST=0 \
+# RUN:     lld-link %t.obj -errorrepro:. -errorlimit:1 -entry:main -out:%t.exe
+# RUN: tar xf repro.tar
+# RUN: diff %t.obj repro/%:t.obj
+# RUN: FileCheck %s --check-prefix=FATALRSP < repro/response.txt
+
+# RSP-NOT: -errorrepro
+# RSP: -entry:main
+# RSP: -out:
+# RSP: errorrepro.test.tmp.obj
+
+# FATALRSP-NOT: -errorrepro
+# FATALRSP: -errorlimit:1
+# FATALRSP: -entry:main
+# FATALRSP: -out:
+# FATALRSP: errorrepro.test.tmp.obj