Fix the following CAS related test for Windows #11475
Conversation
|
@swift-ci please test llvm |
|
Some existing build failure |
clang/lib/CAS/IncludeTree.cpp
Outdated
| // We store the file path into CAS here. Canonicalize it for Windows | ||
| // to avoid cache misses due to slash differences | ||
| SmallString<256> Storage(Filename); | ||
| llvm::sys::path::make_preferred(Storage); |
There was a problem hiding this comment.
Can you say more about how we are ending up with mismatched slashes in this case? The intent here was to keep the paths in the include tree matching the paths that are used naturally by the compiler so that caching doesn't change behaviour (e.g. spelling of paths in diagnostics) unless you opt in to prefix mapping. For example, that is why we don't call remove_dots here when we store the path, but only later when we construct the VFS. I'm not necessarily opposed to your change, but I want to understand it better first.
There was a problem hiding this comment.
In the include-tree-working-directory.c test, the incoming Filename in the first clang-scan-deps invocation is
C:/Users/hiroshi/cas/llvm-project/build-debug/tools/clang/test/ClangScanDeps/Output/include-tree-working-directory.c.tmp\t.c # Note the last separator is a backslash
and in the second clang-scan-deps invocation, it's
C:/Users/hiroshi/cas/llvm-project/build-debug/tools/clang/test/ClangScanDeps/Output/include-tree-working-directory.c.tmp/t.c # Note the last separator is a slash
These follow from the cdb json templates as below
//--- cdb.json.template
[{
"directory": "DIR/other",
"command": "clang -c t.c -I relative -working-directory DIR -o t.o -MD -serialize-diagnostics t.dia",
"file": "DIR/t.c"
}]
//--- cdb2.json.template
[{
"directory": "DIR/other",
"command": "clang -c DIR/t.c -I DIR/relative -working-directory DIR/other -o DIR/t2.o -MD -serialize-diagnostics DIR/t2.dia -fdebug-compilation-dir=DIR -fcoverage-compilation-dir=DIR",
"file": "DIR/t.c"
}]
In the first invocation/cdb json, t.c is a relative path from the working directory and path concatenation used the (windows natural) backslash, while in the second one, DIR/t.c is an absolute path with a slash as the last separator.
I guess, if we change DIR/t.c to ../t.c in the second json, we won't have a cache hit on Posix (right?)
There was a problem hiding this comment.
I guess, if we change DIR/t.c to ../t.c in the second json, we won't have a cache hit on Posix (right?)
Right, we don't canonicalize paths in most cases by default. Note it is technically incorrect to eliminate ../ from paths without checking if they resolve the same if they might contain symlinks. But your point is the same for ./ components, which we only strip them from the VFS and lookup side and not from the stored include tree so that we preserve the spelling of paths seen in the original compilation.
So we've made a tradeoff that favours preserving the same behaviour as non-caching builds (e.g. diagnostics and path macros have the right path spellings), but that could hurt cache hit rates. Maybe there is an argument to change that tradeoff? But we should be deliberate, and probably consistent about it between platforms (if we're going to turn / to \ then we probably also want to remove ./ components). @akyrtzi and @cachemeifyoucan may have opinions on this too.
Is it possible to only change the tests on Windows, or perhaps only change the tests and the VFS part of the IncludeTree code (similar to how we call remove_dots only in the VFS side of things)? That would be my preference at least as an immediate fix.
There was a problem hiding this comment.
How about simply disabling the include-tree-working-directory.c test for Windows and not make any other changes (the logic in IncludeTree.cpp nor ther other tests)? I think in real uses the spelling of the paths will be consistent and cache misses of this nature should be rare.
There was a problem hiding this comment.
Updated with a test-only fix for the include-tree-working-directory.c test for Windows. It only changes the test on Windows. Please take another look @benlangmuir @cachemeifyoucan
Clang :: ClangScanDeps/include-tree-working-directory.c
|
@swift-ci please test llvm |
| // RUN: clang-cas-test -cas %t/cas -print-include-tree @%t/tu.casid | %PathSanitizingFileCheck --sanitize PREFIX=%/t %s | ||
|
|
||
| // CHECK: [[PREFIX]]/t.c llvmcas:// | ||
| // CHECK: PREFIX{{/|\\}}t.c llvmcas:// |
There was a problem hiding this comment.
We have a number of tests that do something like pipe the output through sed 's:\\\\\?:/:g' to avoid needing regex for every CHECK line. Does that work here?
There was a problem hiding this comment.
Yes that would probably work but I had had a review comment that using PathSanitizingFileCheck and doing this way would be more precise so I'm following that style.
| ("%{fs-src-root}", pathlib.Path(sourcedir).anchor), | ||
| ("%{fs-tmp-root}", pathlib.Path(tmpBase).anchor), | ||
| ("%{fs-sep}", os.path.sep), | ||
| ("%{fs-sep-yaml}", "\\\\\\\\\\\\\\\\" if kIsWindows else "/"), |
There was a problem hiding this comment.
Four levels of backslash escaping - one for this python string, two for being quoted and being a replacement pattern in sed and one for being a json string.
Fix the following CAS related test for Windows
Clang :: ClangScanDeps/include-tree-working-directory.c