This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by davide on Nov 16 2016, 12:03 AM.

Diff Detail

Event Timeline

davide updated this revision to Diff 78142.Nov 16 2016, 12:03 AM
davide retitled this revision from to [ELF] Don't replace path separators on *NIX.
davide updated this object.
davide added reviewers: rafael, pcc.
davide added a subscriber: llvm-commits.
pcc edited edge metadata.Nov 16 2016, 12:42 AM

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 added a comment.Nov 16 2016, 12:49 AM

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 updated this revision to Diff 78144.Nov 16 2016, 12:58 AM
davide edited edge metadata.

uh, never seen something like that in the wild.

Added test.

grimar added a subscriber: grimar.Nov 16 2016, 1:24 AM
rafael added inline comments.Nov 16 2016, 8:05 AM
lib/Core/Reproduce.cpp
55

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

ruiu added a subscriber: ruiu.Nov 16 2016, 8:53 AM

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 updated this revision to Diff 78224.Nov 16 2016, 10:56 AM

Simplify (as per Rafael's request).

ruiu added inline comments.Nov 16 2016, 11:12 AM
test/ELF/reproduce-backslash.s
5–6

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

12

Checking the existence of response.txt is extraneous.

ruiu added inline comments.Nov 16 2016, 11:15 AM
test/ELF/reproduce-backslash.s
5

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 added inline comments.Nov 16 2016, 11:24 AM
test/ELF/reproduce-backslash.s
5

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 updated this revision to Diff 78231.Nov 16 2016, 11:28 AM
davide added inline comments.
test/ELF/reproduce-backslash.s
5

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 accepted this revision.Nov 16 2016, 11:32 AM
ruiu added a reviewer: ruiu.

LGTM

test/ELF/reproduce-backslash.s
5

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.

This revision is now accepted and ready to land.Nov 16 2016, 11:32 AM
This revision was automatically updated to reflect the committed changes.
filcab added a subscriber: filcab.Nov 17 2016, 10:36 AM

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
8

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

ruiu added inline comments.Nov 17 2016, 10:52 AM
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.

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).

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 added inline comments.Nov 17 2016, 1:19 PM
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 added inline comments.Nov 17 2016, 1:22 PM
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 added inline comments.Nov 17 2016, 1:30 PM
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 added inline comments.Nov 17 2016, 1:38 PM
lld/trunk/lib/Core/Reproduce.cpp
56 ↗(On Diff #78236)

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

ruiu added inline comments.Nov 17 2016, 1:40 PM
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 added inline comments.Nov 17 2016, 1:43 PM
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 added inline comments.Nov 17 2016, 1:44 PM
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).