[ELF] Don't replace path separators on *NIX

[davide]

Summary

Avoid wasted work. Inspired by Peter Collingbourne's similar change for libArchive. (
https://reviews.llvm.org/D26696)

[pcc]

Test case?

I'd note that this isn't so much about wasted work as it is about correctness -- it is legal (although probably not a good idea) for a filename to contain a backslash on *nix.

[pcc]

And for --reproduce in particular I can easily imagine wanting to preserve unintentional uses of backslash in filenames, e.g. when porting a Windows based build system to *nix.

[davide]

uh, never seen something like that in the wild.

[davide]

Added test.

[• rafael]

lib/Core/Reproduce.cpp

55

Move the ifdef inside the function so that we only need one.

[ruiu]

I don't really care about silly filename containing \ as part of it, but because that's semantically correct, so I'm fine with this.

Not directly related to this patch, but I found that gnuwin32's cpio behaves very weirdly with filenames containing / as path separators. It mostly works, but it prints out error messages. I guess some part of gnuwin32's cpio doesn't handle / as path separators.

[davide]

Simplify (as per Rafael's request).

[ruiu]

test/ELF/reproduce-backslash.s

5–6 ↗

(On Diff #78224)

I don't think you need to to create a directory.

12 ↗

(On Diff #78224)

Checking the existence of response.txt is extraneous.

[ruiu]

test/ELF/reproduce-backslash.s

5 ↗

(On Diff #78224)

And I want to avoid "rm -rf" in general because one mistake can really be disastrous (not only one person but everybody who checks out a buggy test file and run it).

[davide]

test/ELF/reproduce-backslash.s

5 ↗

(On Diff #78224)

What's the alternative? The directory might be non-empty, so rmdir doesn't work. We can remove the rm comment altogether, but the risk is leaving random test output around.

[davide]

test/ELF/reproduce-backslash.s

5 ↗

(On Diff #78224)

I mean, in this case is possible (as I did in the new patch), but in general I doubt we can avoid calling rm -rf.

[ruiu]

LGTM

test/ELF/reproduce-backslash.s

5 ↗

(On Diff #78224)

There might be a case in which we really need to run "rm -rf", but before writing that command, I'll do whatever I can do to avoid it. That's what I mean.

[filcab]

I'm a bit late because I forgot to submit the comments.

You're missing a Windows test which shows you're replacing the path components.
Probably with REQUIRES: system-windows, x86, cpio

P.S: Sorry for the old, "not visible" comments. I can't get phabricator to delete them.

lib/Core/Reproduce.cpp

55

You can even do if (LLVM_ON_WIN32) and get rid of the ifdefs.
Ends up making sure code always compiles :-)

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

Nit: How about if (LLVM_ON_WIN32)?
It would force you to always be able to compile the code :-)

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

1 ↗

(On Diff #78236)

Can shell be replaced by UNSUPPORTED: system-windows, or are you relying on having a "proper shell" and I'm missing something?

test/ELF/reproduce-backslash.s

7 ↗

(On Diff #78144)

This probably won't work on Windows, though.
UNSUPPORTED: system-windows, or something?

[ruiu]

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

Is that a common practice? I believe for #define'd macros, we usually use #ifdef.

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

1 ↗

(On Diff #78236)

shell is used to imply "not on Windows", though it is indeed confusing. Windows has shells (command prompt and explorer), of course, so we probably should define "unix", "posix" or something like that to identify Unix.

[davide]

In D26734#598802, @filcab wrote:

I'm a bit late because I forgot to submit the comments.

You're missing a Windows test which shows you're replacing the path components.
Probably with REQUIRES: system-windows, x86, cpio

There are reproduce-windows.s and reproduce-windows-2.s , no?

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

Probably, but we should change also in the Archiver at that point.

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

1 ↗

(On Diff #78236)

I've seen this used in general to constrain tests to be ran only on Windows (at least in lld).

[davide]

i.e.

[davide@cupiditate ELF]$ grep -R 'UNSUPPORTED' *
[davide@cupiditate ELF]$ grep -R 'REQUIRES: shell' *
invalid-dynamic-list.test:# REQUIRES: shell
invalid-linkerscript.test:# REQUIRES: shell
linkerscript/linkerscript.s:# REQUIRES: shell
linkerscript/linkerscript2.s:# REQUIRES: shell
reproduce-error.s:# REQUIRES: shell
reproduce.s:# REQUIRES: shell

[davide]

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

Just checked, and noticed we're inconsistent here.

[davide@cupiditate lib]$ grep -R 'if LLVM_ON_WIN32' * | wc -l
8
[davide@cupiditate lib]$ grep -R 'ifdef LLVM_ON_WIN32' * | wc -l
29

So, Filipe, you prefer the other spelling? If so, I can change, but I'd rather change it everywhere.

[ruiu]

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

I was thinking that Filipe suggested use C's if instead of C preprocessor's #ifdef (which I don't agree).

[filcab]

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

I'd prefer the if (LLVM_ON_WIN32) variant, as it doesn't allow for code that doesn't compile. But if the regular contributors have objections, I don't mind it too much.

56 ↗

(On Diff #78236)

ruiu: It's very common in the sanitizers, and it ends up being cleaner and safer.

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

1 ↗

(On Diff #78236)

Indeed: shell is not used very well on llvm. I think it would be nice to get this test right (especially since it might be run under Windows with a "shell"), but I don't think we should refactor all tests right now. (And if this test ends up using shell to mean "Windows", it's not the worst thing.)

Refactoring the features should probably be discussed in llvm-dev first, since we'll need to nail down the semantics correctly for the several cases we need (and make it consistent, which is the biggest problem).

[filcab]

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

Since Rui doesn't seem to agree, I'll withdraw this comment.

[ruiu]

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

At LLVM top directory:

$ git grep '#if.*LLVM_ON_WIN32'|wc -l
67

$ git grep 'if (LLVM_ON_WIN32)' | wc -l
0

Is this common?

[ruiu]

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

Are you suggesting

#if LLVM_ON_WIN32

instead of

#ifdef LLVM_ON_WIN32

?

Or,

static void convertToUnixPathSeparator(SmallString<128> &Path) {
  if (LLVM_ON_WIN32)
    std::replace(Path.begin(), Path.end(), '\\', '/');
}

?

[filcab]

lld/trunk/lib/Core/Reproduce.cpp

56 ↗

(On Diff #78236)

It's probably a sanitizer thing, then.
More common in the compiler-rt repo (but we still have a ton of ifdefs for cases when one of the parts won't compile on "the other" system, for example).

lib/Core/Reproduce.cpp

static void writeMember(raw_fd_ostream &OS, StringRef Path, StringRef Data) {
   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.
+#ifdef LLVM_ON_WIN32
 static void convertToUnixPathSeparator(SmallString<128> &Path) {
   std::replace(Path.begin(), Path.end(), '\\', '/');
 }
+#endif
 
 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);
+
+#ifdef LLVM_ON_WIN32
   convertToUnixPathSeparator(Fullpath);
+#endif
 
   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);