Description
This is another instance of the same root cause of #83046
On Windows, if a file is queried with GetFileAttributesW
after it has been marked for deletion (but not yet deleted), the query will fail with the error code being set to ERROR_ACCESS_DENIED
.
Apple's symbol index generation code writes index unit files out with temporary names, and then calls rename
to place the file in the final location. Multiple clang processes may produce the same index unit file, and they race to write the file out to the final destination (overwriting is okay, because the unit file is the same). But before generating index information, the final destination file is first checked to see if it's up-to-date which would avoid doing work to generate duplicate information. This check is done by calling fs::status
on the final destination file, and if this occurs while another clang process has just called to rename its temporary index file to the final destination file, there is a small window where the destination file is marked for deletion and fs::status
returns an unexpected permission_denied
error.
Ideally, I think the status
function should detect this case on Windows and return file_not_found
instead of permission_denied
, but I initially had trouble finding a way to do this. I filed swiftlang#8577 to treat permission_denied
as file_not_found
in this specific case (which fixes the build failures), but this solution is not very satisfying in that it leaks Windows-specific idiosyncrasies.
However, a colleague pointed out that the underlying NTSTATUS
code actually does disambiguate this case with 0xC0000056 STATUS_DELETE_PENDING
(which is mapped to Win32 ERROR_ACCESS_DENIED
). Querying this error code is not super straightforward (at least I couldn't find a straightforward way), but I was able to identify and predicate on this case in Windows-specific code with the following patch:
diff --git a/llvm/lib/Support/CMakeLists.txt b/llvm/lib/Support/CMakeLists.txt
index fbbb27a7c133..54d6d288e017 100644
--- a/llvm/lib/Support/CMakeLists.txt
+++ b/llvm/lib/Support/CMakeLists.txt
@@ -40,7 +40,7 @@ endif()
if( MSVC OR MINGW )
# libuuid required for FOLDERID_Profile usage in lib/Support/Windows/Path.inc.
# advapi32 required for CryptAcquireContextW in lib/Support/Windows/Path.inc.
- set(system_libs ${system_libs} psapi shell32 ole32 uuid advapi32)
+ set(system_libs ${system_libs} psapi shell32 ole32 uuid advapi32 ntdll)
elseif( CMAKE_HOST_UNIX )
if( HAVE_LIBRT )
set(system_libs ${system_libs} rt)
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 1b157fa52bad..10af4905e26c 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -24,10 +24,18 @@
// These two headers must be included last, and make sure shlobj is required
// after Windows.h to make sure it picks up our definition of _WIN32_WINNT
+#define WIN32_NO_STATUS
#include "llvm/Support/Windows/WindowsSupport.h"
+#undef WIN32_NO_STATUS
+
+#include <winternl.h>
+#include <ntstatus.h>
#include <shellapi.h>
#include <shlobj.h>
+extern "C" NTSYSAPI NTSTATUS NTAPI RtlGetLastNtStatus();
+
#undef max
// MinGW doesn't define this.
@@ -769,8 +777,13 @@ std::error_code status(const Twine &path, file_status &result, bool Follow) {
return ec;
DWORD attr = ::GetFileAttributesW(path_utf16.begin());
- if (attr == INVALID_FILE_ATTRIBUTES)
+ if (attr == INVALID_FILE_ATTRIBUTES) {
+ NTSTATUS last_nt_status = RtlGetLastNtStatus();
+ if (last_nt_status == STATUS_DELETE_PENDING)
+ return mapWindowsError(ERROR_FILE_NOT_FOUND);
return getStatus(INVALID_HANDLE_VALUE, result);
+ }
DWORD Flags = FILE_FLAG_BACKUP_SEMANTICS;
// Handle reparse points.
I prefer this solution to the alternative because the additional complexity is nicely encapsulated, but it is not without caveats, namely having to link ntdll
and a lack of confidence in compatibility w.r.t RtlGetLastNtStatus
(I couldn't find any Microsoft documentation on this function).
Maybe I'm missing something and there's an easy way to inspect this error code? Is the cost of this solution as it stands too high?
cc @compnerd