`-verify-additional-file` is confused by Windows path separators

[j-hui]

Description

When we have a test that does something like

// RUN: %target-swift-frontend -verify -I %t/Inputs -verify-additional-file %t/Inputs/header.h

The verify test fails on Windows, which uses \ as the default path separator instead of /.

This is because -I %t/Inputs is expanded to -I C:\Path\To/Inputs, and when a header file is imported its path is constructed using the native path separator, giving us C:\Path\To/Inputs\header.h.

Meanwhile, -verify-additional-file %t/Inputs/header.h is expanded to C:\Path\To/Inputs/header.h.

The two, uncanonicalized paths refer to the same file, but map to different buffer ID numbers in SourceManager’s BufIdentIDMap. Those buffer IDs are used look for expected-{error,warning,note} comments in header.h, but since the imported file has a different file than the -verify-additional-file file, the diagnostics are not successfully associated with one another.

——

I discovered this by pushing some logging to #80549

The important piece of information is here:

# .---command stderr------------
# | Got additional file: [[42]]::C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift\test\Interop\Cxx\foreign-reference/Inputs/inheritance.h
# | Captured error in [[2]]::C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift\test\Interop\Cxx\foreign-reference/Inputs\inheritance.h: 125
…

where 42 is the buffer ID of the -verify-additional-file path and 2 is the buffer ID of the imported file path.

Reproduction

// RUN: %target-swift-frontend -verify -I %s/Inputs -verify-additional-file %s/Inputs/header.h

where %s/Inputs/header.h contains // expected-error {{pattern}} annotations

Expected behavior

I expect -verify-additional-file to work as expected on Windows.

Environment

Current revision at ToT is 8e19f3f at time of writing.

This only affects Windows platform.

Additional information

For now, I converted the C++ interop test cases using -verify-additional-file to use %{fs-sep} to work around this issue. However, it makes the command invocation quite ugly.

[compnerd]

I think that this is just a matter of ensuring that we use llvm::sys::path::native before we pass the paths to the SourceManager. Do you happen to have a pointer to where the file path is being passed to SourceManager? I'm not sure that I'd classify this as a windows specific issue. It is just that the paths happen to match on macOS/Linux. If you use something like HFS or zFS (not to be confused with ZFS), there might be path representation issues as well.

[j-hui]

I'm not sure that I'd classify this as a windows specific issue. It is just that the paths happen to match on macOS/Linux. If you use something like HFS or zFS (not to be confused with ZFS), there might be path representation issues as well.

Yeah this is a good point. In those cases, it should be confused by case-insensitivity, right?

Do you happen to have a pointer to where the file path is being passed to SourceManager?

Yeah, that's happening here:

swift/lib/Frontend/DiagnosticVerifier.cpp

Lines 1291 to 1310 in 82bccb5

	for (auto path : AdditionalFilePaths) {
	auto bufferID = SM.getIDForBufferIdentifier(path);
	if (!bufferID) {
	// Still need to scan this file for expectations.
	auto result = SM.getFileSystem()->getBufferForFile(path);
	if (!result) {
	auto message = SM.GetMessage(
	SourceLoc(), llvm::SourceMgr::DiagKind::DK_Error,
	llvm::Twine("unable to open file in '-verify-additional-file ") +
	path + "': " + result.getError().message(), {}, {});
	printDiagnostic(message);
	Result.HadError |= true;
	continue;
	}
	bufferID = SM.addNewSourceBuffer(std::move(result.get()));
	}
	if (bufferID) {
	additionalBufferIDs.push_back(*bufferID);
	}
	}

[compnerd]

I'm not sure that I'd classify this as a windows specific issue. It is just that the paths happen to match on macOS/Linux. If you use something like HFS or zFS (not to be confused with ZFS), there might be path representation issues as well.

Yeah this is a good point. In those cases, it should be confused by case-insensitivity, right?

Yes, case sensitivity is another issue as well.

Do you happen to have a pointer to where the file path is being passed to SourceManager?

Yeah, that's happening here:

swift/lib/Frontend/DiagnosticVerifier.cpp

Lines 1291 to 1310 in 82bccb5

for (auto path : AdditionalFilePaths) {
auto bufferID = SM.getIDForBufferIdentifier(path);
if (!bufferID) {
// Still need to scan this file for expectations.
auto result = SM.getFileSystem()->getBufferForFile(path);
if (!result) {
auto message = SM.GetMessage(
SourceLoc(), llvm::SourceMgr::DiagKind::DK_Error,
llvm::Twine("unable to open file in '-verify-additional-file ") +
path + "': " + result.getError().message(), {}, {});
printDiagnostic(message);
Result.HadError |= true;
continue;
}
bufferID = SM.addNewSourceBuffer(std::move(result.get()));
}
if (bufferID) {
additionalBufferIDs.push_back(*bufferID);
}
}

Okay, I think that we should be able to just do a native path conversion at L1292.