microsoft / DirectXShaderCompiler

This repo hosts the source for the DirectX Shader Compiler which is based on LLVM/Clang.
Other
3.02k stars 676 forks source link

Pathes in depfiles are not correctly escaped #6570

Open pborsutzki opened 3 months ago

pborsutzki commented 3 months ago

Description When generating a depfile through any of the -M* switches, the generated output does not contain escaped pathes. Due to the depfile format (output: input0 input1 ...) this can lead to ambiguities in tools that parse these files like the Ninja build system.

Steps to Reproduce

Actual Behavior

DXC returns this result: e:\foo.hlsl: e:\foo.hlsl

This result is ambiguous due to the additional colons :. To me, the format of depfiles does not seem to be clearly specified (it seems to be specified "like makefiles"). But other tools (e.g. Ninja build, Slang) avoid the ambiguity by escaping/expecting escaped pathes in depfiles. So a correct output in my opinion would be this: e\:\\foo.hlsl: e\:\\foo.hlsl

More problems can occur when using other problematic characters for makefiles like whitespace, #, $, \, [ and ].

Environment

damyanp commented 3 months ago

Assigned to @pborsutzki as they have a PR out for this already. It'd be good to double check what Clang does with this as well.

llvm-beanz commented 3 months ago

For a trivial example on Windows clang:

#include "test.h"
#include "test space.h"

The output in the dep file is:

test.o: C:\Users\cbieneman\dev\scratch\test.cpp \
  C:\Users\cbieneman\dev\scratch\test.h \
  C:\Users\cbieneman\dev\scratch\test\ space.h

That escapes the space, but not the other slashes. We should match that behavior in DXC, but not escape all the slashes.

llvm-beanz commented 3 months ago

Running the above example through DXC produces:

test.dxbc: C:\Users\cbieneman\dev\scratch\test.cpp \
 C:\Users\cbieneman\dev\scratch/test.h \
 C:\Users\cbieneman\dev\scratch/test space.h

Looks like we're generating pathsep incorrectly and not escaping the space. Both are reasonable fixes.

pborsutzki commented 3 months ago

Thanks for your investigation.

About clang, I am not sure how correct its output is. I tried this on a Linux using clang 15.0.7 with a file named 'foo :[]\#$.cpp containing int main() {}:

$ clang -MD foo\ \:\[\]\\#\$.cpp
$ cat foo\ \:\[\]\\#\$.d
foo\ :[]\\#$$.o: foo\ :[]/\#$$.cpp
$ make -Bdnf foo\ \:\[\]\\#\$.d | -E "Considering|Successfully"
 Considering target file 'foo :[]\#$.d'.
Considering target file 'foo '.
  Considering target file '[]\'.
make: *** No rule to make target '[]\', needed by 'foo '.  Stop.

I am trying to use make as a validation tool here, as depfiles should contain suitable rules for make. So in this, make finds the target files foo and []\ which both weren't the ones intended. To get the correct result, I must escape :, \, #, $ and the whitespace but not [ and ]. When using this as input in the depfile: foo\ \:[]\\\#$$.o: foo\ \:[]\\\#$$.cpp make gives me this:

$ make -Bdnf foo\ \:\[\]\\#\$.d | grep -E "Considering|Successfully"
 Considering target file 'foo :[]\#$.d'.
Considering target file 'foo :[]\#$.o'.
  Considering target file 'foo :[]\#$.cpp'.
Successfully remade target file 'foo :[]\#$.o'.

Which looks about correct to me. This probably is a bit different on Windows, where : and \ are illegal as part of file/directory names anyways.

In the end, after digging through some makefile documentation I found that using "windows drive letters" (e.g. c:) without escaping the colon is legit usage when using a make build on Windows and also supported by the Ninja build system itself. I then found, that the problems I've had were actually caused by other parts of my build system (mainly: using a CMake version pre 3.29.2 with a Ninja generator, absolute paths and CMP0116).

This conclusion makes this issue/PR currently a non-issue for me - but maybe not for you, as you still might be interested in a fix for the uncovered problems. So feel free to close this or tell me if you want me to reshape the PR in any direction.