[ELF] - Use backward slashes inside response files

[grimar]

Summary

Patch replaces forward slashes with backward inside response.txt

This is https://llvm.org/bugs/show_bug.cgi?id=30951.

[• rafael]

lib/Core/Reproduce.cpp

98

Exactly the same line is in CpioFile::append. Is it dead with this change?

[grimar]

lib/Core/Reproduce.cpp

98

I'll check, I did know about that one.

[grimar]

lib/Core/Reproduce.cpp

98

So no, it is not dead.

Current line is responsible for converting slashes in lines of response.txt file.
CpioFile::append()'s line converts the archive filename.

I thought about placing them together in CpioFile::append:

// Use unix path separators so the cpio can be extracted on both unix and
// windows.
std::replace(Fullpath.begin(), Fullpath.end(), '\\', '/');
std::replace(Data.begin(), Data.end(), '\\', '/');

But that probably not good thing to do because in that way slashes will be converted for all responce file and not only for path lines, what can change the original invocation result line.
Also it seems convinent that relativeToRoot() just always returns fixed path.

[ruiu]

lib/Core/Reproduce.cpp

81

Please add a comment when you do something not obvious.

// Returned string is a slash-separated path even on Windows to avoid
// a mess with backslash-as-escape and backslash-as-path-separator.

test/ELF/reproduce-windows2.s

11

Don't use --to-stdout as it won't work on non-GNU systems.

[grimar]

• Addressed review comments

lib/Core/Reproduce.cpp

81

Done.

test/ELF/reproduce-windows2.s

11

I wonder how to make a test then.

I think we can not unpack cpio on windows as "can get over the path limit on windows" like mentioned in reproduce-error.s.
Since this test is about windows specific functionality, what about running it on windows only ?

[• rafael]

LGTM by me with the suggested change, but please wait for Rui to comment.

lib/Core/Reproduce.cpp

98

Ah, I see it now that CpioFile::append uses path::append with might add a \. Even if that was changed to use '/' directly, Basename might still have a \ in it.

So OK, we need the two calls, but can you factor it into a trivial helper function? Something like convertToUnixPathSeparator?

test/lit.cfg

210

llvm use the name system-windows.

This is basically assuming that the cpio on windows is gnu, which I think is fine. Rui, is that OK?

[ruiu]

LGTM

test/ELF/reproduce-windows2.s

11

Being on Windows doesn't mean that the installed cpio command is GNU's, but maybe it's a fairly reasonable assumption.

test/lit.cfg

210

Yup.

lib/Core/Reproduce.cpp

void CpioFile::append(StringRef Path, StringRef Data) {
   uint64_t Pos = OS->tell();
   writeMember(*OS, "TRAILER!!!", "");
   OS->seek(Pos);
 }
 
 // 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".
+// Returned string is a backslash-separated path even on Windows to avoid
+// a mess with backslash-as-escape and backslash-as-path-separator.
 std::string lld::relativeToRoot(StringRef Path) {
   SmallString<128> Abs = Path;
   if (sys::fs::make_absolute(Abs))
     return Path;
   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;
   StringRef Root = path::root_name(Abs);
   if (Root.endswith(":"))
     Res = Root.drop_back();
   else if (Root.startswith("//"))
     Res = Root.substr(2);
 
   path::append(Res, path::relative_path(Abs));
+  std::replace(Res.begin(), Res.end(), '\\', '/');
 
   return Res.str();
 }
 
 // Quote a given string if it contains a space character.
 std::string lld::quote(StringRef S) {
   if (S.find(' ') == StringRef::npos)
     return S;

test/ELF/reproduce-windows2.s

+# REQUIRES: windows, x86, cpio
+
+# Test that a response.txt file always uses / instead of \.
+# RUN: rm -rf %t.dir
+# RUN: mkdir -p %t.dir/build
+# RUN: llvm-mc %s -o %t.dir/build/foo.o -filetype=obj -triple=x86_64-pc-linux
+# RUN: cd %t.dir
+# RUN: ld.lld build/foo.o --reproduce repro
+# RUN: echo "*response.txt" > list.txt
+# RUN: cpio -i --to-stdout --pattern-file=list.txt < repro.cpio | FileCheck %s
+# CHECK: {{.*}}/build/foo.o

test/lit.cfg

 # Running on Darwin OS
 if platform.system() in ['Darwin']:
     config.available_features.add('system-linker-mach-o')
 
 # Running on ELF based *nix
 if platform.system() in ['FreeBSD', 'Linux']:
     config.available_features.add('system-linker-elf')
 
+# Running on Windows
+if platform.system() in ['Windows']:
+    config.available_features.add('windows')
+
 # Set if host-cxxabi's demangler can handle target's symbols.
 if platform.system() not in ['Windows']:
     config.available_features.add('demangler')
 
 # llvm-config knows whether it is compiled with asserts (and)
 # whether we are operating in release/debug mode.
 import subprocess
 try: