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

Strange #include/file system behaviour #157

Closed
simontaylor81 opened this issue Mar 28, 2017 · 1 comment
Closed

Strange #include/file system behaviour #157

simontaylor81 opened this issue Mar 28, 2017 · 1 comment
Assignees

Comments

@simontaylor81
Copy link

I'm getting some strange behaviour with #includeing files from the file system using the dxc commandline app, which are somwhat hard to describe.

First, the repro:

First, download this file, rename .zip (github doesn't like zip files apparently), and extract somewhere. OddIncludes.txt
The repro relies on using absolute paths, so I can't just give a command line to run directly, but assuming the zip was extracted to c:\temp, run the following command (assumes dxc.exe is on the path):

dxc /T ps_6_0 c:\temp\OddIncludes\main.hlsl /I c:\temp

This should generate an error:

c:\temp\OddIncludes\main.hlsl:1:10: fatal error: 'include.hlsl' file not found
#include "include.hlsl"
         ^

This is erroneous -- include.hlsl is in the same directory as main.hlsl, so this should work fine. However, you can "fix" this error by changing the case of the directories. So this command works (note capital T):

dxc /T ps_6_0 c:\temp\OddIncludes\main.hlsl /I c:\Temp

Ok, so now the explanation. Apologies if this is very confusing! The first thing to note is that the (in this case superfluous) additional include directory specification is critical to the repro. Additional include directories are process before any source files (this matters because the file system has lots of caching). When it tries to get the file info for this path, it ends up in DxcArgsFileSystem::CreateFileW. That code does a substring search of the path against the input filename. If it matches (in this case it does), it assumes it is the parent of the source and returns the special handle SourceParentDirHandle. This is added to the cache with the path c:\temp.

Later on, we come to look up the directory for including the file. This is also a parent of the source file, and so also matches the substring match, and so also returns SourceParentDirHandle. This is then a cache hit, so instead of returning the directory asked for, it returns c:\temp again. Include.hlsl file is not found there, giving the error.

Now, the reason changing the case "fixes" things is that all the substring comparisons in dxcompilerobj.cpp are case sensitive! So by changing the case, the substring comparisons fail, and it falls through to just looking up the file in the file system, and things work correctly.

Hopefully that made some sense! Basically, I think the logic surrounding SourceParentDirHandle is broken. I do not know the best way to fix it, though, I'm afraid.

@marcelolr
Copy link
Contributor

Thanks for reporting this, this is an awesome bug. There are a bunch of issues to consider, including providing distinct handle values, absolute-vs-relative references (plus, relative-to-what considerations), case sensitivity, and how much complexity / sophistication we push onto include handlers. I'm working on a fix right now, hopefully I'll have a fix along with some clarifications on design and expectations in the near future.

@marcelolr marcelolr self-assigned this Apr 5, 2017
marcelolr added a commit to marcelolr/DirectXShaderCompiler that referenced this issue Apr 5, 2017
.

Normalizes API-provided names to include at least a current-relative
directory, simplifying file lookups. There are still restrictions on
how expressive include handlers enable the virtual file system to be,
these should be documented on the project wiki.

Handle some errors in BeginSourceFile paths where include handlers may
cause early errors, before compilation execution can begin.

Removes the prior scheme for directory handles, where the same handle
would be returned for any parent directory.

Future improvements would be to expand dxc paths before calling into
dxcompiler, and/or to fully implement OS API calls from console apps.
antiagainst pushed a commit to antiagainst/DirectXShaderCompiler that referenced this issue Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants