Skip to content

Conversation

@hjyamauchi
Copy link

Fix the following CAS related tests for Windows

Clang :: ClangScanDeps/cas-fs-prefix-mapping.c
Clang :: ClangScanDeps/include-tree-prefix-mapping-pch-remap.c
Clang :: ClangScanDeps/include-tree-prefix-mapping.c
Clang :: ClangScanDeps/modules-cas-fs-prefix-mapping-caching.c
Clang :: ClangScanDeps/modules-cas-fs-prefix-mapping.c
Clang :: ClangScanDeps/modules-include-tree-prefix-map.c
Clang :: ClangScanDeps/modules-pch-cas-fs-prefix-mapping-caching.c
Clang :: ClangScanDeps/modules-pch-cas-fs-prefix-mapping.c

@hjyamauchi
Copy link
Author

@swift-ci please test llvm

@hjyamauchi
Copy link
Author

Existing failures (eg. https://ci.swift.org/job/pr-apple-llvm-project-llvm-linux/987/consoleText)

Failed Tests (3):
Clang :: CodeGenCXX/ptrauth-explicit-vtable-pointer-control.cpp
Clang :: Modules/GH154840.cpp
LLVM :: Transforms/PhaseOrdering/X86/excessive-unrolling.ll

@hjyamauchi hjyamauchi marked this pull request as ready for review September 9, 2025 18:53
@hjyamauchi
Copy link
Author

@swift-ci please test llvm

Copy link

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

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

I feel like the complexity of paths is making developers who do not have access to a windows machine really hard to write tests.

Maybe we can even look into deleting some of the implementations from the branch before we found better ways to support them on windows.

Choose a reason for hiding this comment

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

Use namespace llvm::cas

Copy link
Author

Choose a reason for hiding this comment

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

Done

Choose a reason for hiding this comment

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

I don't see we use this file scope overwrite in LLVM. I will just use the correct namespace when calling.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Choose a reason for hiding this comment

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

I don't like how much conversion we have in the sort function.

We should canonicalize the path style before creating PrefixMapper. Maybe during option parsing? Or make MappedPrefix canonicalize.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Choose a reason for hiding this comment

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

What I mean is that there are extra canonicalization when getTreePath, so it doesn't matter the form of the MappedPrefix is, so PrefixMapper should contain canonicalize MappedPrefix from the beginning, not to convert them when need to sort them.

Copy link
Author

Choose a reason for hiding this comment

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

I see your point. I think this, current way feels overall better to me because of the following: sorting here also requires case-insensitivity in addition to slash canonicalization (we want to sort C:\... and c:\... case-insensitively).

This is the original, to-be-erased comment on sort()

  // FIXME: Only works for posix right now since it doesn't handle case- and
  // separator-insensitivity.

I think it'd be simpler to handler slash and case together in one location. We could have a slash canonicalization and change cases when PrefixMapper is populated but it would be problematic because a) whatever is put into PrefixMapper will be passed down as the -fdepscan-prefix-map flags to subinvocations and it'd be visible and surprising to the users, and b) there aren't nice ways to make lit tests matching case-insensitive (not even PathSanitizingFileCheck doesn't do). I think this way fits better with the current general way that Clang doesn't modify flag value input paths (mostly if not always) and yet being able to do the longer prefix-first prefix mapping as I think this intends. I think canonicalization here is done once, not per each comparator call and isn't too bad.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Choose a reason for hiding this comment

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

I really hope we can cut down the number of root representation here.

Can we make tree path working the same way as normal path using \\ on Windows as separator?

I am even ok with throwing out CachingOnDiskFileSystem and TreePathPrefixMapper from Windows for now since we don't really use that for clang/swift caching.

Copy link
Author

Choose a reason for hiding this comment

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

Cut down to the two that are currently used. It's good to know we have that option of throwing those out.

@hjyamauchi
Copy link
Author

@swift-ci please test llvm

Copy link

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

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

Mostly fine. Hope we can simply further.

@compnerd
Copy link
Member

I feel like the complexity of paths is making developers who do not have access to a windows machine really hard to write tests.

This complexity is the desired state IMO. The problem is that paths on Windows are significantly different from Unix paths. It is very easy to get an improper path representation if you are working with only Unix paths. An absolute path on Unix is a relative path on Windows. There is no concept of a drive letter and Windows has infinite roots at the same time, something which is not a concept on Unix. A similar problem would exist if we were trying to support HFS style paths (e.g. Macintosh HD:Applications:CorelDraw.app). As long as paths participate in the CAS, I think that would be an issue. I think that a middle ground approach might be to require all paths be created through indirection, e.g. %path(identifier, component, component, component) and use lit to formulate the path and create a macro which can be used to reference the path.

Clang :: ClangScanDeps/cas-fs-prefix-mapping.c
Clang :: ClangScanDeps/include-tree-prefix-mapping-pch-remap.c
Clang :: ClangScanDeps/include-tree-prefix-mapping.c
Clang :: ClangScanDeps/modules-cas-fs-prefix-mapping-caching.c
Clang :: ClangScanDeps/modules-cas-fs-prefix-mapping.c
Clang :: ClangScanDeps/modules-include-tree-prefix-map.c
Clang :: ClangScanDeps/modules-pch-cas-fs-prefix-mapping-caching.c
Clang :: ClangScanDeps/modules-pch-cas-fs-prefix-mapping.c
@hjyamauchi
Copy link
Author

@swift-ci please test llvm

@hjyamauchi
Copy link
Author

Existing failures

Clang :: Misc/warning-flags.c
Clang :: Modules/GH154840.cpp
Clang-Unit :: ./AllClangUnitTests/DependencyScanningCAPITests/DependencyScanningFSCacheOutOfDate
LLVM :: Transforms/PhaseOrdering/X86/excessive-unrolling.ll

Eg. https://ci.swift.org/job/pr-apple-llvm-project-llvm-linux/1010/consoleText

@hjyamauchi hjyamauchi merged commit 8de16d1 into swiftlang:next Sep 18, 2025
0 of 2 checks passed
hjyamauchi added a commit that referenced this pull request Oct 9, 2025
Clang :: ClangScanDeps/cas-fs-prefix-mapping.c
Clang :: ClangScanDeps/include-tree-prefix-mapping-pch-remap.c
Clang :: ClangScanDeps/include-tree-prefix-mapping.c
Clang :: ClangScanDeps/modules-cas-fs-prefix-mapping-caching.c
Clang :: ClangScanDeps/modules-cas-fs-prefix-mapping.c
Clang :: ClangScanDeps/modules-include-tree-prefix-map.c
Clang :: ClangScanDeps/modules-pch-cas-fs-prefix-mapping-caching.c
Clang :: ClangScanDeps/modules-pch-cas-fs-prefix-mapping.c

(Cherry picked from commit 8de16d1)
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

Successfully merging this pull request may close these issues.

3 participants