[PATCH] Fix dependency file escaping

Thu May 14 07:09:54 PDT 2015

Nico, this breaks Clang's depfiles to match gcc's broken depfiles.  I still think we should produce depfiles that GNU Make and ninja can process correctly, rather than produce depfiles that are demonstrably broken but that match what gcc produces.  That is, I think making known existing widely used tools work properly is more important than speculating about some hypothetical tool that might possibly someday figure out how to reverse-engineer gcc's brokenness.
But you get the last word on this.  (If nothing else, it means I'll shut up about it!)
Thanks,
--paulr

From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Robinson, Paul
Sent: Wednesday, May 13, 2015 3:38 PM
To: Sean Silva
Cc: reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org; cfe-commits at cs.uiuc.edu
Subject: RE: [PATCH] Fix dependency file escaping

Fine.  r237304.  Gave me a chance to clean up the comment a bit, anyway.
--paulr

From: Sean Silva [mailto:chisophugis at gmail.com]
Sent: Wednesday, May 13, 2015 2:55 PM
To: Robinson, Paul
Cc: Nico Weber; reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org<mailto:reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org>; cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: Re: [PATCH] Fix dependency file escaping

On Wed, May 13, 2015 at 2:23 PM, Robinson, Paul <Paul_Robinson at playstation.sony.com<mailto:Paul_Robinson at playstation.sony.com>> wrote:
r237296.  I believe I've answered all the questions, there have been no new comments for two days, and Sean already gave approval.

One person's approval does not override another person's concerns. Please back out the `#` part until Nico's concerns have been fully addressed.

Thanks!
--paulr

From: cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu> [mailto:cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu>] On Behalf Of Robinson, Paul
Sent: Monday, May 11, 2015 11:17 AM
To: Nico Weber

Cc: reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org<mailto:reviews%2BD9208%2Bpublic%2B0ac91376ffbd3e34 at reviews.llvm.org>; cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: RE: [PATCH] Fix dependency file escaping

One would naively think that GNU Make ought to be the program that can successfully parse gcc-produced depfiles, but it demonstrably does not (in the obviously obscure corner-case of a filename that has a '#' immediately after a '\' which, one must admit, is not inconceivable on Windows).
So, shall we imitate gcc behavior that even the gcc people think doesn't look right (gcc.gnu.org/bugzilla/show_bug.cgi?id=65852<http://gcc.gnu.org/bugzilla/show_bug.cgi?id=65852>) and can't be handled correctly even by GNU Make?  Or shall we implement behavior that GNU Make can parse correctly, even though it's not exactly what gcc produces?

I am more inclined to produce a depfile that can be handled correctly by current tools than to produce one that cannot because of what even the gcc people admit looks wrong.  The definition of what a Makefile looks like more properly belongs to GNU Make than to GCC.
--paulr

From: thakis at google.com<mailto:thakis at google.com> [mailto:thakis at google.com] On Behalf Of Nico Weber
Sent: Monday, May 11, 2015 10:21 AM
To: Robinson, Paul
Cc: Sean Silva; reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org<mailto:reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org>; cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: Re: [PATCH] Fix dependency file escaping

I don't think ninja's current behavior is interesting. My point is more that it should be possible to write one program that can parse .d files produced by both gcc and clang. If gcc and clang agree on some "escaping", that's possible, else it isn't. If this patch makes clang match gcc's output for '\'s in path names that's cool. I'm skeptical if it's worth diverging on '#' characters.

On Mon, May 11, 2015 at 8:19 AM, Robinson, Paul <Paul_Robinson at playstation.sony.com<mailto:Paul_Robinson at playstation.sony.com>> wrote:
Okay, I worked out how to persuade ninja to read a depfile.  And in fact ninja 1.3.4 on Ubuntu 14.04 will treat
    tspace.o: tspace.c bar\\#foo.h
and
    tspace.o: tspace.c bar\\\#foo.h
as both expressing a dependency on the file named "bar\#foo.h" so the clang patch works for ninja.
--paulr

From: cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu> [mailto:cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu>] On Behalf Of Robinson, Paul
Sent: Monday, May 11, 2015 7:58 AM
To: Sean Silva
Cc: reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org<mailto:reviews%2BD9208%2Bpublic%2B0ac91376ffbd3e34 at reviews.llvm.org>; cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: RE: [PATCH] Fix dependency file escaping

Is this right? GCC and the current patch coincide on the handling of `\ `. The only deviation is in the `#` case, no?

Sorry, you are correct, GCC was getting "\ " right but "\#" wrong.  Clang was doing both wrong.
So, for Nico's example, "\ " would be converted to "\\\ " by gcc, but not by Clang, and that's being fixed by the patch.
But GCC would emit "\#" as "\\#" which GNU make would interpret as "\" followed by a comment, and if that failed, try to interpret it as "\\" followed by a comment, neither of which would look for the correct file.  The patch causes Clang to emit this as "\\\#" which lets GNU make find the right file.

Regarding ninja, I don't think ninja can directly read Make-style dependency files?  Its syntax naively looks different enough that I'm not really sure that's relevant here.
--paulr

From: Sean Silva [mailto:chisophugis at gmail.com]
Sent: Friday, May 08, 2015 6:40 PM
To: Robinson, Paul
Cc: Nico Weber; reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org<mailto:reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org>; cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: Re: [PATCH] Fix dependency file escaping

On Fri, May 8, 2015 at 5:57 PM, Robinson, Paul <Paul_Robinson at playstation.sony.com<mailto:Paul_Robinson at playstation.sony.com>> wrote:
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.

Is this right? GCC and the current patch coincide on the handling of `\ `. The only deviation is in the `#` case, no?

-- Sean Silva

(Remember that the string you hand to #include is NOT a normal C string; it has no escaping in it.)
--paulr

From: cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu> [mailto:cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu>] On Behalf Of Nico Weber
Sent: Friday, May 08, 2015 4:36 PM
To: reviews+D9208+public+0ac91376ffbd3e34 at reviews.llvm.org<mailto:reviews%2BD9208%2Bpublic%2B0ac91376ffbd3e34 at reviews.llvm.org>
Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: Re: [PATCH] Fix dependency file escaping

On Fri, May 8, 2015 at 4:01 PM, Paul Robinson <Paul_Robinson at playstation.sony.com<mailto:Paul_Robinson at playstation.sony.com>> wrote:
In http://reviews.llvm.org/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?

'course not, but that's not the case here. 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 "\ ". So this is breaking backwards compat of clang with itself.

_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150514/ecadda0d/attachment.html>