[ELF] Introduce --reproduce flag

[davide]

Summary

Preface: lld is very likely to become the default linker in FreeBSD (in the next year or such).
In order to help the transition, we're going to ask FreeBSD developers to try out the new linker and report bugs.
--reproduce dumps the object files in a directory chosen (preserving the file system layout) and the linker invocation so that people can create an archive and upload to bugzilla.

[ruiu]

ELF/Config.h

51 ↗

(On Diff #54896)

Remove = "". Strings are initialized to empty string by default. Also please rename Reproduce as the members of Config have the same name as command line options.

ELF/Driver.cpp

101–107 ↗

(On Diff #54896)

Why do you need to make a path full path?

115 ↗

(On Diff #54896)

I think you want to use path::append instead of appending "/".

118 ↗

(On Diff #54896)

Let's create a std::string for the directory name so that we don't need to repeat this string concatenation twice.

135 ↗

(On Diff #54896)

Let's make it explicit that the feature is optional.

if (Config->Reproduce)
  dumpFile(Path);

263 ↗

(On Diff #54896)

error(EC, "--reproduce: can't create directory");

270 ↗

(On Diff #54896)

auto -> const char *

271 ↗

(On Diff #54896)

This would leave a space character at end of the line and does not terminate the line with '\n'. As this is not a speed-critical path, I'd do

OS << llvm::join(Args.begin(), Args.end(), " ") << "\n";

288 ↗

(On Diff #54896)

Guard with if (Config->Reproduce) as well.

436–437 ↗

(On Diff #54896)

This error would be caught in dumpLinkerInvocation, no?

[• rafael]

For what it is worth, something like this would have been very handy
at creating the test directories I use for benchmarking :-)

[davide]

ELF/Driver.cpp

101–107 ↗

(On Diff #54896)

I don't think I need. Fixed.

271 ↗

(On Diff #54896)

I don't think your solution works because the way join works right now.
In fact, it doesnt' even build.

In file included from ../tools/lld/ELF/Driver.cpp:21:
../include/llvm/ADT/StringExtras.h:177:20: error: member reference base type 'const char' is not a structure or union

Len += (*Begin).size();
       ~~~~~~~~^~~~~

../include/llvm/ADT/StringExtras.h:192:10: note: in instantiation of function template specialization 'llvm::join_impl<const char *>' requested here

return join_impl(Begin, End, Separator, tag());
       ^

../tools/lld/ELF/Driver.cpp:271:15: note: in instantiation of function template specialization 'llvm::join<const char *>' requested here

OS << llvm::join(*Args.begin(), *Args.end(), " ");
            ^

1 error generated.

We might want to fix in StringExtras.h, but I'd rather do that later.
I added a '\n' at the end.

436–437 ↗

(On Diff #54896)

It doesn't seem so, i.e. if I remove the check:

[davide@localhost build-clang]$ rm -rf /home/davide/work/llvm/tools/lld/test/ELF/repro2
[davide@localhost build-clang]$ mkdir /home/davide/work/llvm/tools/lld/test/ELF/repro2
[davide@localhost build-clang]$ /home/davide/work/llvm/build-clang/./bin/not /home/davide/work/llvm/build-clang/./bin/ld.lld /home/davide/work/llvm/build-clang/tools/lld/test/ELF/Output/reproduce.s.tmp -o /home/davide/work/llvm/build-clang/tools/lld/test/ELF/Output/reproduce.s.tmp2 --reproduce /home/davide/work/llvm/tools/lld/test/ELF/repro2
[davide@localhost build-clang]$

[ruiu]

ELF/Driver.cpp

251 ↗

(On Diff #54936)

Remove trailing . and pass EC to error. Then error will format the message for you by appending a colon and EC.message().

error(EC, "--reproduce: can't create directory");

262 ↗

(On Diff #54936)

OK, but please do not emit trailing whitespace to the generated file.

428–429 ↗

(On Diff #54936)

Looks like create_directories accepts IgnoreExisting parameter as the second parameter.

std::error_code create_directories(const Twine &path,
                                   bool IgnoreExisting = true,
                                   perms Perms = owner_all | group_all);

You want to pass false then you can remove this code.

[davide]

Addressed second round of comments.

[ruiu]

ELF/Driver.cpp

102–103 ↗

(On Diff #54945)

Remove.

105 ↗

(On Diff #54945)

Please do not add llvm:: for consistency.

106–107 ↗

(On Diff #54945)

if (std::error_code EC = sys::fs::create_directories(DirName))

108 ↗

(On Diff #54945)

Please pass EC as the first argument at this place too.

114–115 ↗

(On Diff #54945)

if (std::error_code EC = sys::fs::copy_file(SrcPath, DestPathName))

256 ↗

(On Diff #54945)

Please fix this place as well. You want to use sys::path::append instead of textually appending path fragments.

261 ↗

(On Diff #54945)

I'd do this (because ARGV[0] must exist)

OS << Args[0];
for (size_t I = 1, E = Args.size(); I < E; ++I)
  OS << " " << Args[I];
OS << "\n";

[ruiu]

LGTM with a nit.

ELF/Driver.cpp

423–424 ↗

(On Diff #54948)

Replace this with

Config->Reproduce = getString(Args, OPT_reproduce, "");

and move to the group at line 350-356 (and sort the group).

[silvas]

ELF/Driver.cpp

259 ↗

(On Diff #54948)

An argument might contain a space, so let's separate these by newlines instead.
(an argument can technically contain a newline but I think that's sufficiently rare that we don't need to worry about that in a first patch.)

[ruiu]

ELF/Driver.cpp

259 ↗

(On Diff #54948)

Pathname with spaces are also rare (except on Windows), no? An upside of writing all options in one line is you can (re-)execute the command just by passing the file to the shell.

[silvas]

ELF/Driver.cpp

259 ↗

(On Diff #54948)

Pathname with spaces are also rare (except on Windows), no?

Exactly. Windows is one of our supported platforms.

An upside of writing all options in one line is you can (re-)execute the command just by passing the file to the shell.

For now, let's go ahead with how Davide had it (one line). We can make this more robust in future patches.

lld/trunk/ELF/Config.h

struct Configuration {
   llvm::StringRef Entry;
   llvm::StringRef Emulation;
   llvm::StringRef Fini;
   llvm::StringRef Init;
   llvm::StringRef OutputFile;
   llvm::StringRef SoName;
   llvm::StringRef Sysroot;
   std::string RPath;
+  std::string Reproduce;
   std::vector<llvm::StringRef> DynamicList;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<llvm::StringRef> Undefined;
   std::vector<llvm::StringRef> VersionScriptGlobals;
   bool AllowMultipleDefinition;
   bool AsNeeded = false;
   bool Bsymbolic;
   bool BsymbolicFunctions;

lld/trunk/ELF/Driver.cpp

 #include "InputFiles.h"
 #include "LinkerScript.h"
 #include "SymbolListFile.h"
 #include "SymbolTable.h"
 #include "Target.h"
 #include "Writer.h"
 #include "lld/Driver/Driver.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
 #include <utility>
 
 using namespace llvm;
 using namespace llvm::ELF;
 using namespace llvm::object;
 
LinkerDriver::getArchiveMembers(MemoryBufferRef MB) {
 
   // Take ownership of memory buffers created for members of thin archives.
   for (std::unique_ptr<MemoryBuffer> &MB : File->takeThinBuffers())
     OwningMBs.push_back(std::move(MB));
 
   return V;
 }
 
+static void dumpFile(StringRef SrcPath) {
+  SmallString<128> DirName;
+  sys::path::append(DirName, Config->Reproduce, sys::path::parent_path(SrcPath));
+  if (std::error_code EC = sys::fs::create_directories(DirName)) {
+    error(EC, "--reproduce: can't create directory");
+    return;
+  }
+
+  SmallString<128> DestPathName;
+  sys::path::append(DestPathName, Config->Reproduce, SrcPath);
+  if (std::error_code EC = sys::fs::copy_file(SrcPath, DestPathName))
+    error(EC, "--reproduce: can't copy file");
+}
+
 // Opens and parses a file. Path has to be resolved already.
 // Newly created memory buffers are owned by this driver.
 void LinkerDriver::addFile(StringRef Path) {
   using namespace llvm::sys::fs;
   if (Config->Verbose)
     llvm::outs() << Path << "\n";
+  if (!Config->Reproduce.empty())
+    dumpFile(Path);
+
   Optional<MemoryBufferRef> Buffer = readFile(Path);
   if (!Buffer.hasValue())
     return;
   MemoryBufferRef MBRef = *Buffer;
 
   switch (identify_magic(MBRef.getBuffer())) {
   case file_magic::unknown:
     readLinkerScript(MBRef);

 
 static bool hasZOption(opt::InputArgList &Args, StringRef Key) {
   for (auto *Arg : Args.filtered(OPT_z))
     if (Key == Arg->getValue())
       return true;
   return false;
 }
 
+static void dumpLinkerInvocation(ArrayRef<const char *> Args) {
+  std::error_code EC = sys::fs::create_directories(Config->Reproduce,
+    false /* IgnoreExisting */);
+  if (EC) {
+    error(EC, "--reproduce: can't create directory");
+    return;
+  }
+
+  SmallString<128> InvocationPath;
+  sys::path::append(InvocationPath, Config->Reproduce, "invocation.txt");
+  raw_fd_ostream OS(InvocationPath, EC, sys::fs::OpenFlags::F_None);
+  check(EC);
+
+  OS << Args[0];
+  for (unsigned I = 1, E = Args.size(); I < E; ++I)
+    OS << " " << Args[I];
+  OS << "\n";
+}
+
 void LinkerDriver::main(ArrayRef<const char *> ArgsArr) {
   ELFOptTable Parser;
   opt::InputArgList Args = Parser.parse(ArgsArr.slice(1));
   if (Args.hasArg(OPT_help)) {
     printHelp(ArgsArr[0]);
     return;
   }
   if (Args.hasArg(OPT_version)) {
     printVersion();
     return;
   }
 
   initLLVM(Args);
   readConfigs(Args);
+
+  if (!Config->Reproduce.empty())
+    dumpLinkerInvocation(ArgsArr);
+
   createFiles(Args);
   checkOptions(Args);
   if (HasError)
     return;
 
   switch (Config->EKind) {
   case ELF32LEKind:
     link<ELF32LE>(Args);
void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->Verbose = Args.hasArg(OPT_verbose);
   Config->WarnCommon = Args.hasArg(OPT_warn_common);
 
   Config->DynamicLinker = getString(Args, OPT_dynamic_linker);
   Config->Entry = getString(Args, OPT_entry);
   Config->Fini = getString(Args, OPT_fini, "_fini");
   Config->Init = getString(Args, OPT_init, "_init");
   Config->OutputFile = getString(Args, OPT_o);
+  if (auto *Arg = Args.getLastArg(OPT_reproduce))
+    Config->Reproduce = Arg->getValue();
   Config->SoName = getString(Args, OPT_soname);
   Config->Sysroot = getString(Args, OPT_sysroot);
 
   Config->Optimize = getInteger(Args, OPT_O, 0);
   Config->LtoO = getInteger(Args, OPT_lto_O, 2);
   if (Config->LtoO > 3)
     error("invalid optimization level for LTO: " + getString(Args, OPT_lto_O));
   Config->LtoJobs = getInteger(Args, OPT_lto_jobs, 1);

lld/trunk/ELF/Options.td

def o : JoinedOrSeparate<["-"], "o">, MetaVarName<"<path>">,
   HelpText<"Path to file to write output">;
 
 def pie : Flag<["-"], "pie">,
   HelpText<"Create a position independent executable">;
 
 def print_gc_sections: Flag<["--"], "print-gc-sections">,
   HelpText<"List removed unused sections">;
 
+def reproduce : Separate<["--"], "reproduce">,
+  HelpText<"Dump linker invocation and input files for debugging">;
+
 def rpath : Separate<["-"], "rpath">,
   HelpText<"Add a DT_RUNPATH to the output">;
 
 def relocatable : Flag<["--"], "relocatable">,
   HelpText<"Create relocatable object file">;
 
 def script : Separate<["--"], "script">,
   HelpText<"Read linker script">;

lld/trunk/test/ELF/reproduce.s

+# REQUIRES: x86
+
+# RUN: rm -rf %S/repro
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: ld.lld %t -o %t2 -shared --as-needed --reproduce %S/repro
+# RUN: llvm-objdump -d %S/repro/%t | FileCheck %s --check-prefix=DUMP
+# RUN: cat %S/repro/invocation.txt | FileCheck %s --check-prefix=INVOCATION
+
+.globl _start;
+_start:
+  mov $60, %rax
+  mov $42, %rdi
+  syscall
+
+# DUMP: Disassembly of section .text:
+# DUMP: _start:
+# DUMP:        0:       48 c7 c0 3c 00 00 00    movq    $60, %rax
+# DUMP:        7:       48 c7 c7 2a 00 00 00    movq    $42, %rdi
+# DUMP:        e:       0f 05   syscall
+
+# INVOCATION: lld {{.*}}reproduce.s{{.*}} -o {{.*}} -shared --as-needed --reproduce
+
+# RUN: rm -rf %S/repro2
+# RUN: mkdir %S/repro2
+# RUN: not ld.lld %t -o %t2 --reproduce %S/repro2 2>&1 | FileCheck --check-prefix=EDIR %s
+# EDIR: --reproduce: can't create directory