Fix dependency file escaping

[probinson]

Summary

When writing a dependency (.d) file, if space or # is immediately
preceded by backslashes, escape the backslashes as well as the space
or # character.
This straddles the fence between BSD Make (which does no escaping at
all, and does not support space or # in filespecs) and GNU Make (which
does support escaping, but will fall back to the filespec as-written
if the escaping doesn't match an existing file).

[silvas]

lib/Frontend/DependencyFile.cpp

363

Why # also? We are trying to be gcc-compatible. See https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libcpp/mkdeps.c;h=78bbf09a0cc8c3ba0fe974b55e1ededf02c66665;hb=HEAD#l47

[probinson]

lib/Frontend/DependencyFile.cpp

363

No, we're trying to produce something that GNU Make can use. What gcc produces is broken by any definition.

$ cat tspace.c
#include "foo\ bar.h"
#include "foo\#bar.h"
int i;
$ ls
foo\ bar.h foo\#bar.h tspace.c
$ gcc -c -MMD tspace.c
$ cat tspace.d
tspace.o: tspace.c foo\\\ bar.h foo\\#bar.h
$ make -f tspace.d tspace.o
make: *** No rule to make target foo\', needed by tspace.o'. Stop.
$

[silvas]

lib/Frontend/DependencyFile.cpp

363

Please file a bug with gcc.

Also, curiously that .d file seems to work for me on windows under cygwin:

Sean@Sean-PC ~
$ cat >tspace.c
#include "foo\ bar.h"
#include "foo\#bar.h"
int i;

Sean@Sean-PC ~
$ mkdir foo

Sean@Sean-PC ~
$ cd foo

Sean@Sean-PC ~/foo
$ mkdir ' bar.h'

Sean@Sean-PC ~/foo
$ mkdir '#bar.h'

Sean@Sean-PC ~/foo
$ cd ..

Sean@Sean-PC ~
$ cat >tspace.d
tspace.o: tspace.c foo\\\ bar.h foo\\#bar.h

Sean@Sean-PC ~
$ make -f tspace.d
cc    -c -o tspace.o tspace.c
make: cc: Command not found
<builtin>: recipe for target 'tspace.o' failed
make: *** [tspace.o] Error 127

Also, clang's current output seems to work fine on cygwin. In what cases is clang's output currently breaking? Is it just the case of filenames containing backslashes followed by space/hash on unix?

The set of strings that can be correctly escaped in make is inherently limited, so we are inherently compromising on what file names can be properly represented. Does it make sense to extend the set of properly representable filenames to files containing backslashes in their name?

E.g. include "foo\ bar.h". On unix we would be instructing make to look for a file 'foo\ bar.h' in the current directory but on windows in my testing above we would be instructing make to look for a file ' bar.h' in directory 'foo'. Any project for which that would actually be correct behavior is insane. I think that realistically, backslashes are only going to end up in includes on windows where they will be intended as directory separators, and will result in compilation errors on other platforms. I believe that clang's current output actually works correctly in that case.

We might be overthinking this. What case are we fixing? And does it make sense to fix that case?

[probinson]

lib/Frontend/DependencyFile.cpp

363

Cygwin is not doing the right thing for me. Using Clang's depfile:
{code}
probinson@PROBINSON-T3610 ~
$ /cygdrive/e/upstream/build/debug/bin/clang -c -MMD tspace.c

probinson@PROBINSON-T3610 ~
$ cat tspace.d
tspace.o: tspace.c foo\\ bar.h foo\\#bar.h

probinson@PROBINSON-T3610 ~
$ touch 'foo\#bar.h'

probinson@PROBINSON-T3610 ~
$ make -f tspace.d tspace.o
make: *** No rule to make target 'bar.h', needed by 'tspace.o'. Stop.
{code}
Escaping the space is no good unless you escape the preceding backslash.

With gcc's depfile:
{code}
probinson@PROBINSON-T3610 ~
$ cat tspace.d
tspace.o: tspace.c foo\\\ bar.h foo\\#bar.h

probinson@PROBINSON-T3610 ~/foo
$ touch 'foo/#bar.h'

probinson@PROBINSON-T3610 ~
$ make -f tspace.d tspace.o
make: 'tspace.o' is up to date.
{code}
I suspect it's looking at directory foo, and treating #bar.h as a comment.

But if I add the extra backslash manually, it works correctly.
{code}
probinson@PROBINSON-T3610 ~
$ cat tspace.d
tspace.o: tspace.c foo\\\ bar.h foo\\\#bar.h

probinson@PROBINSON-T3610 ~
$ touch 'foo/#bar.h'

probinson@PROBINSON-T3610 ~
$ make -f tspace.d tspace.o
cc -c -o tspace.o tspace.c
{code}

So, what am I fixing? If we're going to do anything special with space and hash, then we should do a special thing that causes them to work properly with a tool that knows how to interpret the special thing. The only tool I have handy that is willing to do a special thing with space and hash is GNU Make (on Linux or Windows). The special thing to do is backslash-quote those characters. But that doesn't work in GNU Make unless you also backslash-quote the preceding backslashes; and that characteristic is the same for quoting either space or hash.

Therefore, what clang produces won't work at all, and what gcc produces will work only for space and not for hash. My fix makes Clang's output work for both space and hash. Provably.

I freely admit--always have, I think--that this is a rather obscure corner case that just isn't going to happen in practice. But it's provably wrong, and my fix provably corrects it. This is a problem?

Alternatively, we can just abandon any special processing for space and hash, and say Don't Do That. But leaving this thing in its current provably wrong state just leaves me professionally offended.

[probinson]

lib/Frontend/DependencyFile.cpp

363

And why I want to fix this... aside from the professionally-offended part... :-)

We are looking at modifying the PS4 toolchain's builder to move away from double-quotes, to doing things the GNU-Make style way. But I don't want to require our builder to correctly interpret broken depfiles; Clang should be producing correctly formed depfiles, and our builder should be able to depend on depfiles being correctly formed.

[probinson]

Please file a bug with gcc.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65852
I traded comments on this with somebody yesterday; not actually Confirmed, but he also didn't reject it.

[probinson]

Rebase on top of the other depfile change.

[probinson]

Ping.

[probinson]

Ping. Sean, I filed a gcc bug as noted previously, which two people have commented on and admitted it looks funny.

[silvas]

Please add a lengthy comment describing exactly how to reproduce the case where GCC's output is "broken" and our deviation from its behavior results in things not being broken. Also, in the test file, please isolate all testing of this behavioral deviation and identify it as such. Other than that, this LGTM.

[thakis]

Does gcc intend to fix this soon? Isn't being compatible with gcc important
than the other things?

[probinson]

In D9208#169446, @thakis wrote:

Does gcc intend to fix this soon? Isn't being compatible with gcc important
than the other things?

If gcc emitted an incorrect relocation, would you argue that it's important to be compatible with gcc? Even if you could not point to any linker that handled that buggy relocation in a reasonable way?

[probinson]

Update the test to isolate the not-like-gcc behavior, and incorporate a Lengthy Comment on the function implementing this behavior as requested by reviewer.

[probinson]

If a program (say, ninja) tries to be compatible with gnu make's depfile parsing, it would previously convert "\ " to a space from what I understand. Now it's going to get "\\\ " and think that that's "\ ".

You're mixing things up. #include "\ " would be converted by gcc to "\\ " (because it escapes the space but not the backslash) which would be de-escaped by GNU Make as "\" followed by a space delimiter.
Now Clang will give it "\\\ " which will be handled as "\ " which is correct.
(Remember that the string you hand to #include is NOT a normal C string; it has no escaping in it.)
--paulr

From: [e-mail] [mailto:[e-mail]] On Behalf Of Nico Weber
Sent: Friday, May 08, 2015 4:36 PM
To: [e-mail]
Cc: [e-mail]
Subject: Re: [PATCH] Fix dependency file escaping

[silvas]

Paul, can you go ahead and commit the non-controversial part of this patch? It sounds like we're getting really held up on the escaping of # when the other part of this patch is pure win.

[probinson]

Looks like we timed-out simultaneously... if people really insist that the # part is a problem, I can undo that as a post-commit fixup.

lib/Frontend/DependencyFile.cpp

void DFGImpl::InclusionDirective(SourceLocation HashLoc,
   }
 }
 
 void DFGImpl::AddFilename(StringRef Filename) {
   if (FilesSet.insert(Filename).second)
     Files.push_back(Filename);
 }
 
-/// PrintFilename - GCC escapes spaces, # and $, but apparently not ' or " or
-/// other scary characters. NMake/Jom has a different set of scary characters,
-/// but wraps filespecs in double-quotes to avoid misinterpreting them;
+/// Print the filename, with escaping or quoting that accommodates the three
+/// most likely tools that use dependency files: GNU Make, BSD Make, and
+/// NMake/Jom.
+///
+/// BSD Make is the simplest case: It does no escaping at all.  This means
+/// characters that are normally delimiters, i.e. space and # (the comment
+/// character) simply aren't supported in filenames.
+///
+/// GNU Make does allow space and # in filenames, but to avoid being treated
+/// as a delimiter or comment, these must be escaped with a backslash. Because
+/// backslash is itself the escape character, if a backslash appears in a
+/// filename, it should be escaped as well.  (As a special case, $ is escaped
+/// as $$, which is the normal Make way to handle the $ character.)
+/// For compatibility with BSD Make and historical practice, if GNU Make
+/// un-escapes characters in a filename but doesn't find a match, it will
+/// retry with the unmodified original string.
+///
+/// GCC tries to accommodate both Make formats by escaping any space or #
+/// characters in the original filename, but not escaping any backslash
+/// characters.  That way, filenames with backslashes will be handled
+/// correctly by BSD Make, and by GNU Make in its fallback mode of using the
+/// unmodified original string; filenames with # or space characters aren't
+/// supported by BSD Make at all, but will be handled correctly by GNU Make
+/// due to the escaping.
+///
+/// A corner case that GCC does not handle is when the original filename has
+/// a backslash immediately followed by # or space. It will therefore take a
+/// dependency from a directive such as
+///     #include "a\#b.h"
+/// and emit it as
+///     a\\#b.h
+/// which GNU Make will interpret as
+///     a\
+/// followed by a comment. Failing to find this file, it will fall back to the
+/// original string, and look for
+///     a\\#b.h
+/// which probably doesn't exist either; in any case it won't find
+///     a\#b.h
+/// which is the actual filename specified by the include directive.
+///
+/// Clang escapes space, # and $ like GCC does, but also handles the case of
+/// backslash immediately preceding space or # by doubling those backslashes.
+/// This means Clang will emit the dependency from
+///     #include "a\#b.h"
+/// as
+///     a\\\#b.h
+/// which GNU Make will un-escape into
+///     a\#b.h
+/// which is the correct original filename.
+///
+/// NMake/Jom has a different set of scary characters, but wraps filespecs in
+/// double-quotes to avoid misinterpreting them; see
 /// https://msdn.microsoft.com/en-us/library/dd9y37ha.aspx for NMake info,
 /// https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
 /// for Windows file-naming info.
 static void PrintFilename(raw_ostream &OS, StringRef Filename,
                           DependencyOutputFormat OutputFormat) {
   if (OutputFormat == DependencyOutputFormat::NMake) {
     // Add quotes if needed. These are the characters listed as "special" to
     // NMake, that are legal in a Windows filespec, and that could cause
     // misinterpretation of the dependency string.
     if (Filename.find_first_of(" #${}^!") != StringRef::npos)
       OS << '\"' << Filename << '\"';
     else
       OS << Filename;
     return;
   }
   for (unsigned i = 0, e = Filename.size(); i != e; ++i) {
-    if (Filename[i] == ' ' || Filename[i] == '#')
+    if (Filename[i] == ' ' || Filename[i] == '#') {
       OS << '\\';
-    else if (Filename[i] == '$') // $ is escaped by $$.
+      unsigned j = i;
+      while (j > 0 && Filename[--j] == '\\')
+        OS << '\\';
+    } else if (Filename[i] == '$') // $ is escaped by $$.
       OS << '$';
     OS << Filename[i];
   }
 }
 
 void DFGImpl::OutputDependencyFile() {
   if (SeenMissingHeader) {
     llvm::sys::fs::remove(OutputFile);

test/Frontend/dependency-gen-escaping.c

 // NMAKE-NOT: "
 // NMAKE: normal.h
 // NMAKE-NOT: "
 
 #include "    .h"
 #include "$$.h"
 #include "##.h"
 #include "normal.h"
+
+// Backslash followed by # or space is handled differently than GCC does,
+// because GCC doesn't emit this obscure corner case the way GNU Make wants it.
+// CHECK: a\b\\\#c\\\ d.h
+// These combinations are just another case for NMAKE.
+// NMAKE: "a\b\#c\ d.h"
+
+#include "a\b\#c\ d.h"