Make --reproduce produce more reproducible logs.

[ruiu]

Summary

The aim of this patch is to make it easy to re-run the command without
updating paths in the command line. Here is a use case.

Assume that Alice is having an issue with lld and is reporting the issue
to developer Bob. Alice's current directly is /home/alice/work and her
command line is "ld.lld -o foo foo.o ../bar.o". She adds "--reproduce repro"
to the command line and re-run. Then the following text will be produced as
invocation.txt (notice that the paths are rewritten so that they are
relative to /home/alice/work/repro.)

-o home/alice/work/foo home/alice/work/foo.o home/alice/bar.o

The command also produces the following files by copying inputs.

/home/alice/repro/home/alice/work/foo.o
/home/alice/repro/home/alice/bar.o

Alice zips the directory and send it to Bob. Bob get an archive from Alice
and extract it to his home directory as /home/bob/repro. Now his directory
have the following files.

/home/bob/repro/invocation.txt
/home/bob/repro/home/alice/work/foo.o
/home/bob/repro/home/alice/bar.o

Bob then re-run the command with these files by the following commands.

cd /home/bob/repro
ld.lld @invocation.txt

[silvas]

This sounds like it would lead to a highly degraded experience for windows users.

Let me explain the technique that I use to reconstruct games which works pretty well and has been used across windows/linux/mac for some time now, to see if it would work for your use case too:

Basically, your resulting directory structure is as follows (filenames changed for LLD's --reproduce use case):

repro.dir/
    invocation.txt
    FS_ROOT/
        C/
            Users/
                alice/

On Unix it would be something like:

repro.dir/
    invocation.txt
    FS_ROOT/
        home/
            alice/

The key thing is that the FS_ROOT captures every file that LLD needs to reproduce the build.

Then there is one more key step: all paths on the command line are converted to be relative paths to repro.dir. That is, LLD, when run with its CWD set to repro.dir, sees only paths of the form FS_ROOT/.... (I call such paths "relatively absolute").

This approach has worked for me in practice robustly across Windows, Mac, and Linux.

In the future, we can make invocation.txt a bit more robust, such as making it a rsp file so that we know we will interpret correctly when invoked as ld.lld @invocation.txt. We control the response file quoting for this and so we can do this robustly. It seems overcomplicated to me (and non-portable) to have a dependency on sh/bash simply for running our own invocation.

[llvm-commits]

• msg-7357-55.txt162 BDownload

[silvas]

I think we can accomplish the goals of this patch without adding a dependence on a shell.

ELF/DriverUtils.cpp

126 ↗

(On Diff #55649)

It would be better to make invocation.txt be a response file (where we control the quoting) so that we don't depend on having a unix shell. That also avoids needing to hardcode /ld.lld which makes things further unportable.

[ruiu]

I don't agree that this would degrade the experience on Windows because currently on Windows (or even on Unix) you have to edit the command line to re-run the command. And it wouldn't add dependency to the Unix shell -- I'm trying to make it reconstractable so I employed a mechanical rule that is the same as the Unix's single quotation rule. Currently, you cannot distinguish spaces between filename with original spaces between arguments. So, at least, this wouldn't make things worse.

But I think it is a nice idea to make it produce a response file.

I could rewrite paths so that they are relative to FS_ROOT, but how would you handle thin archive files that contain pathnames rather than actual member files?

[ruiu]

• Updated as per Sean's comment. invocation.txt is now a response file.

[ruiu]

• Fix typo.

[silvas]

In D19737#417445, @ruiu wrote:

I could rewrite paths so that they are relative to FS_ROOT, but how would you handle thin archive files that contain pathnames rather than actual member files?

We could look inside them in this code for --reproduce or save some information in our InputFile structures. Don't we have a single point of truth for every file we open?

ELF/DriverUtils.cpp

122 ↗

(On Diff #55680)

Clang generates response files for the linker and has been in production for some time. Can we share that code? I think the code we want to share is part of Command::writeResponseFile.

144 ↗

(On Diff #55680)

You may want to call this invocation.rsp since it is a response file (it obviously doesn't matter, but since we are changing the meaning from the previous invocation.txt it might help to avoid confusion).

152 ↗

(On Diff #55680)

Nice reusing libOption for this! This is really nice!

[silvas]

test/ELF/reproduce.s

20 ↗

(On Diff #55680)

When I tried the first version of your patch this was failing on windows for the same reason as the original reproduce patch did (lit expands a path foo/C:/...). Can we avoid the problem?

[ruiu]

ELF/DriverUtils.cpp

122 ↗

(On Diff #55680)

I'll try.

144 ↗

(On Diff #55680)

How about response.txt? I think on Windows "rsp" extension is not recognized so that it is not easy to look inside.

152 ↗

(On Diff #55680)

:)

test/ELF/reproduce.s

20 ↗

(On Diff #55680)

Reid suggested defining a new variable "%:t" to get a path without a drive letter. But I'm not sure if it's worth to do. I'm inclined to simply add "REQUIRES: shell" to this file.

[silvas]

test/ELF/reproduce.s

20 ↗

(On Diff #55680)

As we have seen there are some Windows-specific considerations for --reproduce, so disabling testing for this feature on Windows is unfortunately not an option.

[ruiu]

• Make tests work on Windows.

ELF/DriverUtils.cpp

122 ↗

(On Diff #55680)

Command::writeResponseFile is not reusable for this purpose, and that function is too small to generalize. I think this function is just fine.

[ruiu]

• Update comments.

[• rafael]

I think this is an improvement. Both in feature (getting a response file with quotes) and code organization.

There is more work to be done, but I think it is fine to do it as a follow up patches.

So, fine with me, but please wait to see if Sean has more to add.

[silvas]

Yeah, let's get this in.

[davide]

LGTM

[ruiu]

Can you guys also take a look at http://reviews.llvm.org/D19757? This patch depends on that.

lld/trunk/ELF/Driver.h

 #define OPTION(_1, _2, ID, _4, _5, _6, _7, _8, _9, _10, _11) OPT_##ID,
 #include "Options.inc"
 #undef OPTION
 };
 
 void printHelp(const char *Argv0);
 void printVersion();
 
-std::string concat_paths(StringRef S, StringRef T);
-void copyFile(StringRef Src, StringRef Dest);
-
+void saveLinkerInputs(const llvm::opt::InputArgList &Args);
 std::string findFromSearchPaths(StringRef Path);
 std::string searchLibrary(StringRef Path);
 std::string buildSysrootedPath(llvm::StringRef Dir, llvm::StringRef File);
 
 } // namespace elf
 } // namespace lld
 
 #endif

lld/trunk/ELF/Driver.cpp

 }
 
 // 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())
-    copyFile(Path, concat_paths(Config->Reproduce, Path));
 
   Optional<MemoryBufferRef> Buffer = readFile(Path);
   if (!Buffer.hasValue())
     return;
   MemoryBufferRef MBRef = *Buffer;
 
   switch (identify_magic(MBRef.getBuffer())) {
   case file_magic::unknown:

 
 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 logCommandline(ArrayRef<const char *> Args) {
-  if (std::error_code EC = sys::fs::create_directories(
-        Config->Reproduce, /*IgnoreExisting=*/false)) {
-    error(EC, Config->Reproduce + ": can't create directory");
-    return;
-  }
-
-  SmallString<128> Path;
-  path::append(Path, Config->Reproduce, "invocation.txt");
-  std::error_code EC;
-  raw_fd_ostream OS(Path, EC, sys::fs::OpenFlags::F_None);
-  check(EC);
-
-  OS << Args[0];
-  for (size_t 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;
   }
 
   readConfigs(Args);
   initLLVM(Args);
 
   if (!Config->Reproduce.empty())
-    logCommandline(ArgsArr);
+    saveLinkerInputs(Args);
 
   createFiles(Args);
   checkOptions(Args);
   if (HasError)
     return;
 
   switch (Config->EKind) {
   case ELF32LEKind:

lld/trunk/ELF/DriverUtils.cpp

 void elf::printVersion() {
   outs() << "LLD " << getLLDVersion();
   std::string S = getLLDRepositoryVersion();
   if (!S.empty())
     outs() << " " << S;
   outs() << "\n";
 }
 
-// Concatenates S and T so that the resulting path becomes S/T.
-// There are a few exceptions:
-//
-//  1. The result will never escape from S. Therefore, all ".."
-//     are removed from T before concatenatig them.
-//  2. Windows drive letters are removed from T before concatenation.
-std::string elf::concat_paths(StringRef S, StringRef T) {
-  // Remove leading '/' or a drive letter, and then remove "..".
-  SmallString<128> T2(path::relative_path(T));
-  path::remove_dots(T2, /*remove_dot_dot=*/true);
-
+// Makes a given pathname an absolute path first, and then remove
+// beginning /. For example, "../foo.o" is converted to "home/john/foo.o",
+// assuming that the current directory is "/home/john/bar".
+static std::string relative_to_root(StringRef Path) {
+  SmallString<128> Abs = Path;
+  if (std::error_code EC = fs::make_absolute(Abs))
+    fatal("make_absolute failed: " + EC.message());
+  path::remove_dots(Abs, /*remove_dot_dot=*/true);
+
+  // This is Windows specific. root_name() returns a drive letter
+  // (e.g. "c:") or a UNC name (//net). We want to keep it as part
+  // of the result.
   SmallString<128> Res;
-  path::append(Res, S, T2);
+  StringRef Root = path::root_name(Path);
+  if (Path.endswith(":"))
+    Res = Root.drop_back();
+  else if (Path.startswith("//"))
+    Res = Root.substr(2);
+
+  path::append(Res, path::relative_path(Abs));
   return Res.str();
 }
 
-void elf::copyFile(StringRef Src, StringRef Dest) {
+// Copies file Src to {Config->Reproduce}/Src.
+// Returns the new path relative to Config->Reproduce.
+static std::string copyFile(StringRef Src) {
+  std::string Relpath = relative_to_root(Src);
+  SmallString<128> Dest;
+  path::append(Dest, Config->Reproduce, Relpath);
+
   SmallString<128> Dir(Dest);
   path::remove_filename(Dir);
-  if (std::error_code EC = sys::fs::create_directories(Dir)) {
+  if (std::error_code EC = sys::fs::create_directories(Dir))
     error(EC, Dir + ": can't create directory");
-    return;
-  }
   if (std::error_code EC = sys::fs::copy_file(Src, Dest))
     error(EC, "failed to copy file: " + Dest);
+  return Relpath;
+}
+
+// Quote a given string if it contains a space character.
+static std::string quote(StringRef S) {
+  if (S.find(' ') == StringRef::npos)
+    return S;
+  return ("\"" + S + "\"").str();
+}
+
+// Copies all input files to Config->Reproduce directory and
+// create a response file as "response.txt", so that you can re-run
+// the same command with the same inputs just by executing
+// "ld.lld @response.txt". Used by --reproduce. This feature is
+// supposed to be used by users to report an issue to LLD developers.
+void elf::saveLinkerInputs(const llvm::opt::InputArgList &Args) {
+  // Create the output directory.
+  if (std::error_code EC = sys::fs::create_directories(
+        Config->Reproduce, /*IgnoreExisting=*/false)) {
+    error(EC, Config->Reproduce + ": can't create directory");
+    return;
+  }
+
+  // Open "response.txt".
+  SmallString<128> Path;
+  path::append(Path, Config->Reproduce, "response.txt");
+  std::error_code EC;
+  raw_fd_ostream OS(Path, EC, sys::fs::OpenFlags::F_None);
+  check(EC);
+
+  // Dump the command line to response.txt while copying files
+  // and rewriting paths.
+  for (auto *Arg : Args) {
+    switch (Arg->getOption().getID()) {
+    case OPT_reproduce:
+      break;
+    case OPT_script:
+      OS << "--script ";
+      // fallthrough
+    case OPT_INPUT: {
+      StringRef Path = Arg->getValue();
+      if (fs::exists(Path))
+        OS << quote(copyFile(Path)) << "\n";
+      else
+        OS << quote(Path) << "\n";
+      break;
+    }
+    default:
+      OS << Arg->getAsString(Args) << "\n";
+    }
+  }
 }
 
 std::string elf::findFromSearchPaths(StringRef Path) {
   for (StringRef Dir : Config->SearchPaths) {
     std::string FullPath = buildSysrootedPath(Dir, Path);
     if (sys::fs::exists(FullPath))
       return FullPath;
   }

lld/trunk/test/ELF/reproduce.s

 # REQUIRES: x86
 
 # RUN: rm -rf %t.dir
 # RUN: mkdir -p %t.dir/build1
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.dir/build1/foo.o
 # RUN: cd %t.dir
 # RUN: ld.lld build1/foo.o -o bar -shared --as-needed --reproduce repro
-# RUN: diff build1/foo.o repro/build1/foo.o
+# RUN: diff build1/foo.o repro/%:t.dir/build1/foo.o
 
-# RUN: FileCheck %s --check-prefix=INVOCATION < repro/invocation.txt
-# INVOCATION: lld{{[^\s]*}} build1/foo.o -o bar -shared --as-needed --reproduce repro
+# RUN: FileCheck %s --check-prefix=RSP < repro/response.txt
+# RSP:      {{.*}}foo.o
+# RSP-NEXT: -o bar
+# RSP-NEXT: -shared
+# RSP-NEXT: --as-needed
 
 # RUN: mkdir -p %t.dir/build2/a/b/c
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.dir/build2/foo.o
 # RUN: cd %t.dir/build2/a/b/c
 # RUN: ld.lld ./../../../foo.o -o bar -shared --as-needed --reproduce repro
-# RUN: diff %t.dir/build2/foo.o repro/foo.o
+# RUN: diff %t.dir/build2/foo.o repro/%:t.dir/build2/foo.o
+
+# RUN: not ld.lld build1/foo.o --reproduce repro2 'foo bar' -soname=foo
+# RUN: FileCheck %s --check-prefix=RSP2 < repro2/response.txt
+# RSP2:      "foo bar"
+# RSP2-NEXT: -soname=foo
 
 # RUN: not ld.lld build1/foo.o -o bar -shared --as-needed --reproduce . 2>&1 \
 # RUN:   | FileCheck --check-prefix=ERROR %s
 # ERROR: can't create directory
 
 .globl _start
 _start:
   mov $60, %rax
   mov $42, %rdi
   syscall