Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape paths in depfiles. #6569

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pborsutzki
Copy link

Escape paths in generated depfiles.

Problems especially occur when using depfiles containing absolute paths on windows that include driver letters (e.g. c:\foo\bar) as the depfile format requires a colon between the output dependency and its input dependencies (output: input1 input2 ...). When Windows drive letters come into play this becomes ambiguous (c:\output: c:\input1 c:\input2). Therefore, certain characters should be escaped when generating the depfile (c\:\\output1: c\:\\input1 c\:\\input2). This PR adds a method for escaping the relevant characters.

@pborsutzki pborsutzki requested a review from a team as a code owner April 26, 2024 13:10
@pborsutzki
Copy link
Author

@microsoft-github-policy-service agree

Copy link
Member

@damyanp damyanp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

Thank you for your contribution.

We do ask that all changes come with adequate testing (please see https://github.com/microsoft/DirectXShaderCompiler/blob/main/CONTRIBUTING.md#contributing-code-and-content for more details). It would also be good to have an issue filed that describes the problem being addressed.

Some things I'm not sure about:

  • a quick test https://godbolt.org/z/s94s9P37G suggests that DXC is already converting \ characters to / characters in paths, so I'm not sure why escaping is needed here.
  • I'd like to know more about the scenario - which tools require the :s be escaped for example? Are there other tools that might not support escaped :s that would be broken by this change?
  • how was the escaping / not escaping of #, [, ] and $ chosen?

@pborsutzki
Copy link
Author

Hi,

thanks for coming back to me so quickly.

We do ask that all changes come with adequate testing (please see https://github.com/microsoft/DirectXShaderCompiler/blob/main/CONTRIBUTING.md#contributing-code-and-content for more details). It would also be good to have an issue filed that describes the problem being addressed.

I'll try to follow up on this. Do you have any pointers where I can find similar tests in this project from which I can start?

a quick test https://godbolt.org/z/s94s9P37G suggests that DXC is already converting \ characters to / characters in paths, so I'm not sure why escaping is needed here.

Oops, I actually didn't check for backslashes. In that case I'd then simply remove that branch.

I'd like to know more about the scenario - which tools require the :s be escaped for example?

The tool in question is the Ninja build system. So this fix/issue stems from trying to use DXC's depfiles together with Ninja in a CMake environment on Windows. When provided with a depfile containing an absolute path containing a driver letter for the output Ninja will assume everything before the first colon to be the output, everything afterwards be a list of input dependencies. This then leads to Ninja failing to recognize the output correctly and also will generate an invalid dependency:

A depfile containing c:/foo/bar.cpp: c:/foo/bar.cpp c:/foo/bar.hpp will wrongly result in a file named c that comes with three input dependencies that read /foo/bar.cpp:, c:/foo/bar.cpp and c:/foo/bar.hpp.

Are there other tools that might not support escaped :s that would be broken by this change?

Currently there is only one I know of, which is CMake using the CMP0116 policy, though this can be "worked around" by disabling the policy. But in case of colons in the output path, the CMake result is broken anyways, as CMake currently ignores the escaped colon and simply generates similar broken dependencies like Ninja does without having the colon escaped. I'll try to propose a similar change for CMake as soon as I find the time for it.

Actually I am currently not aware of any build systems out there that support depfiles other than Ninja.

how was the escaping / not escaping of #, [, ] and $ chosen?

As the format of depfiles does not seem to be clearly specified (it seems to be specified "like makefiles"), I looked at how other tools generate and parse depfiles and decided for the "maximum" of escaped characters. Though depending on the OS, some of them are illegal as part of pathes anyways.

  • Ninja: Supports escaped \, :, #, $ and whitespaces.
  • CMake parser does currently not support escaped colons, but escaped $, #, \ and whitespaces.
  • CMake generator currently only escapes backslashes and whitespaces.
  • Slang escapes #, $, :, \, [, ] and whitespaces.

I'd also be happy to only go for e.g. escaping only colons and whitespaces if you think that this would be the safer solution.

I hope that answers your questions.
Thank you for your time!

@damyanp
Copy link
Member

damyanp commented Apr 26, 2024

I'll try to follow up on this. Do you have any pointers where I can find similar tests in this project from which I can start?

This may be a good place to start:

https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/test/DXC/dump_dependency.hlsl

The tool in question is the Ninja build system [...]

This excellent context would be great to capture in the issue, and it does seem like you've done your due diligence here on the suitability of your fix.

@pborsutzki
Copy link
Author

pborsutzki commented Apr 29, 2024

This may be a good place to start:

Thanks for the hint!

a quick test https://godbolt.org/z/s94s9P37G suggests that DXC is already converting \ characters to / characters in paths, so I'm not sure why escaping is needed here.

I've had another look, and it looks like DXC is just dumping absolute include paths containing backslashes into depfiles. In my opinion, escaping the backslash is unavoidable.

I also put up an issue, find it here: #6570

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants