[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

96 ↗

(On Diff #77324)

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

[grimar]

lib/Core/Reproduce.cpp

96 ↗

(On Diff #77324)

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

[grimar]

lib/Core/Reproduce.cpp

96 ↗

(On Diff #77324)

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

79 ↗

(On Diff #77324)

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

10 ↗

(On Diff #77324)

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

[grimar]

• Addressed review comments

lib/Core/Reproduce.cpp

79 ↗

(On Diff #77324)

Done.

test/ELF/reproduce-windows2.s

10 ↗

(On Diff #77324)

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

96 ↗

(On Diff #77324)

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 ↗

(On Diff #77461)

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

10 ↗

(On Diff #77324)

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 ↗

(On Diff #77461)

Yup.

lld/trunk/lib/Core/Reproduce.cpp

static void writeMember(raw_fd_ostream &OS, StringRef Path, StringRef Data) {
   OS << "000000";                        // c_rdev
   OS << "00000000000";                   // c_mtime
   OS << format("%06o", Path.size() + 1); // c_namesize
   OS << format("%011o", Data.size());    // c_filesize
   OS << Path << '\0';                    // c_name
   OS << Data;                            // c_filedata
 }
 
+// Converts path to use unix path separators so the cpio can be extracted on
+// both unix and windows.
+static void convertToUnixPathSeparator(SmallString<128> &Path) {
+  std::replace(Path.begin(), Path.end(), '\\', '/');
+}
+
 void CpioFile::append(StringRef Path, StringRef Data) {
   if (!Seen.insert(Path).second)
     return;
 
   // Construct an in-archive filename so that /home/foo/bar is stored
   // as baz/home/foo/bar where baz is the basename of the output file.
   // (i.e. in that case we are creating baz.cpio.)
   SmallString<128> Fullpath;
   path::append(Fullpath, Basename, Path);
-
-  // Use unix path separators so the cpio can be extracted on both unix and
-  // windows.
-  std::replace(Fullpath.begin(), Fullpath.end(), '\\', '/');
+  convertToUnixPathSeparator(Fullpath);
 
   writeMember(*OS, Fullpath, Data);
 
   // Print the trailer and seek back.
   // This way we have a valid archive if we crash.
   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));
+  convertToUnixPathSeparator(Res);
 
   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;

lld/trunk/test/ELF/reproduce-windows2.s

+# REQUIRES: system-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

lld/trunk/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('system-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: