Handle Windows drive letters and ".." for --reproduce.

[ruiu]

Summary

When --reproduce <path> is given, then we need to concatenate input
file paths to the given path to save input files to the directory.
Previously, path concatenation didn't handle Windows drive letters
so it could generate invalid paths such as "C:\D:\foo". It also didn't
handle ".." path components, so it could produce some bad paths
such as "foo/../../etc/passwd".

In this patch, Windows drive letters and ".." are removed before
concatenating paths.

[davide]

ELF/Driver.cpp

106 ↗

(On Diff #55070)

concatenating.

258 ↗

(On Diff #55070)

I prefer dumpLinkerInvocation still.

272 ↗

(On Diff #55070)

We use unsigned everywhere else in the code base, try to be consistent or change all the other occurrences.

[davide]

My understanding is that you're trying to remove all the ".." from T before doing the concatenation.
What happens if you have ../foo and ../../foo ? Would they collide? If not, can you please add a test to ensure this won't happen?

Thanks for your work, btw.

[ruiu]

ELF/Driver.cpp

258 ↗

(On Diff #55070)

To me this name is more plain and therefore good. It just logs command line -- I can tell it would obviously have no side effect (except writing a log somewhere) just by looking at the name. But dumpLinkerInvocation sounds something more complex.

272 ↗

(On Diff #55070)

I don't think we do. I actually use size_t where it means the size of an array. This is also technically accurate (even though it is unrealistic that we have more than 2^32 arguments).

[ruiu]

If we have "../foo" and "../../foo", then one will overwrite the other.

[davide]

ELF/Driver.cpp

258 ↗

(On Diff #55070)

As you wish. Discussing this is probably not worth more time.

272 ↗

(On Diff #55070)

I think these should be changed then:

Writer.cpp: for (unsigned I = 0; I < OutputSections.size(); ++I) {
OutputSections.cpp: for (unsigned I = 0, N = Offsets.size(); I != N; ++I) {

I don't care which one we use just try to use the same everywhere. Feel free to change in another patch.

[davide]

In D19551#412677, @ruiu wrote:

If we have "../foo" and "../../foo", then one will overwrite the other.

hmm, that's undesiderable. I think converting to absolute paths before appending *might* fix the issue, although I'd like to hear your opinion on it.

[ruiu]

Right. That's surely undesirable, but I guess it wouldn't happen in reality. Full path would be fine, but that would be more tricky to test (because it is not easy to concatenate two absolute paths in tests), so I think this is fine. Since this is a developer option, we can change its behavior later.

[davide]

OK, not entirely convinced, but enough to make progress.
Should we change the logCommandLine() invocation to include the transformed paths?

[ruiu]

If we concatenate Config->Reproduce and "invocation.txt" before passing it
to logCommandline, we need to decompose the path to get the directory part
which is exactly Config->Reproducible, so it is better to not concatenate them.

[davide]

In D19551#412732, @ruiu wrote:

If we concatenate Config->Reproduce and "invocation.txt" before passing it
to logCommandline, we need to decompose the path to get the directory part
which is exactly Config->Reproducible, so it is better to not concatenate them.

lgtm.

lld/trunk/ELF/Driver.cpp

 #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;
+using namespace llvm::sys;
 
 using namespace lld;
 using namespace lld::elf;
 
 Configuration *elf::Config;
 LinkerDriver *elf::Driver;
 
 bool elf::link(ArrayRef<const char *> Args, raw_ostream &Error) {
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");
+// 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.
+static std::string concat(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);
+
+  SmallString<128> Res;
+  path::append(Res, S, T2);
+  return Res.str();
+}
+
+static void copyFile(StringRef Src, StringRef Dest) {
+  SmallString<128> Dir(Dest);
+  path::remove_filename(Dir);
+  if (std::error_code EC = sys::fs::create_directories(Dir)) {
+    error(EC, Dir + ": 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");
+  if (std::error_code EC = sys::fs::copy_file(Src, Dest))
+    error(EC, "failed to copy file: " + Dest);
 }
 
 // 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);
+    copyFile(Path, concat(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 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");
+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> InvocationPath;
-  sys::path::append(InvocationPath, Config->Reproduce, "invocation.txt");
-  raw_fd_ostream OS(InvocationPath, EC, sys::fs::OpenFlags::F_None);
+  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 (unsigned I = 1, E = Args.size(); I < E; ++I)
+  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;
   }
 
   initLLVM(Args);
   readConfigs(Args);
 
   if (!Config->Reproduce.empty())
-    dumpLinkerInvocation(ArgsArr);
+    logCommandline(ArgsArr);
 
   createFiles(Args);
   checkOptions(Args);
   if (HasError)
     return;
 
   switch (Config->EKind) {
   case ELF32LEKind:

lld/trunk/test/ELF/reproduce.s

 # REQUIRES: x86
 
-# RUN: rm -rf %t.dir1
-# RUN: mkdir -p %t.dir1/build
-# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.dir1/build/foo.o
-# RUN: cd %t.dir1
-# RUN: ld.lld build/foo.o -o bar -shared --as-needed --reproduce repro
-# RUN: diff build/foo.o repro/build/foo.o
+# 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: FileCheck %s --check-prefix=INVOCATION < repro/invocation.txt
+# INVOCATION: lld{{[^\s]*}} build1/foo.o -o bar -shared --as-needed --reproduce repro
+
+# 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: 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
-
-# RUN: FileCheck %s --check-prefix=INVOCATION < repro/invocation.txt
-# INVOCATION: lld{{[^\s]*}} build/foo.o -o bar -shared --as-needed --reproduce repro
-
-# RUN: not ld.lld build/foo.o -o bar -shared --as-needed --reproduce repro 2>&1 \
-# RUN:   | FileCheck --check-prefix=ERROR %s
-# ERROR: --reproduce: can't create directory