Page MenuHomePhabricator

[Support] PR42623: Avoid setting the delete-on-close bit if a TempFile doesn't reside on a local drive
Needs ReviewPublic

Authored by rdwampler on Jun 14 2020, 6:28 AM.

Details

Summary

On Windows, after commit 881ba104656c40098d4bc90c52613c08136f0fe1, tools
using TempFile would error with "bad file descriptor" when writing the file
on a network drive. It appears that setting the delete-on-close bit via
SetFileInformationByHandle/FileDispositionInfo prevented it from
accessing the file on network drives, and although using FILE_DISPOSITION_INFO
seems to work, it causes other troubles.

Diff Detail

Event Timeline

rdwampler created this revision.Jun 14 2020, 6:28 AM

I noticed this first when using llvm-ar (LLVM 10), which worked on mapped network drive with LLVM 6.0 and bisected it to the commit in the message. I'm not familiar with Windows api so maybe there is a better way?

rdwampler updated this revision to Diff 270642.Jun 14 2020, 3:43 PM

That commit (which was reviewed here https://reviews.llvm.org/D48051) suggests this wasn't about the occasional temp file not getting deleted. It was to work around surprising behavior on Windows when a temp file is mapped into memory, which, as I recall, was the underlying cause in what appeared to be cache corruption for LTO.

I think it best if @pcc and/or @inglorion weigh in.

BTW, is it common to create temp files on a network drive? Is that done in distributed build systems? For a single-machine build, it would seem counter productive to use a network drive for a temp file.

That commit (which was reviewed here https://reviews.llvm.org/D48051) suggests this wasn't about the occasional temp file not getting deleted. It was to work around surprising behavior on Windows when a temp file is mapped into memory, which, as I recall, was the underlying cause in what appeared to be cache corruption for LTO.

I think it best if @pcc and/or @inglorion weigh in.

BTW, is it common to create temp files on a network drive? Is that done in distributed build systems? For a single-machine build, it would seem counter productive to use a network drive for a temp file.

I have a similar setup as described in here https://bugs.llvm.org/show_bug.cgi?id=42623 for local development. That bug report also includes a link to a GitHub issue where others where having the same issue. So, it's probably not uncommon to build on a drive attached as a network drive to a VM.

rnk requested changes to this revision.Sep 15 2020, 11:32 AM

The situation where a temp file won't be delete without the delete-on-close bit set seems rare so just avoid setting it entirely.

In my experience, it's actually very common for LLVM tools to leak temp files, so we really need to preserve this functionality, it can't just be removed.

We could accept a more targeted fix. Does the call to CreateFile fail with the delete on close disposition? If so, we could retry without the disposition if it fails.

This revision now requires changes to proceed.Sep 15 2020, 11:32 AM

We could accept a more targeted fix. Does the call to CreateFile fail with the delete on close disposition? If so, we could retry without the disposition if it fails.

The call to CreateFile does succeed, but it’s the subsequent writes to the file handle that fails. Perhaps, we can try to detect if we are creating a file on a network drive using GetDriveType and if so avoid setting delete on close?

rdwampler retitled this revision from [Support] PR42623: Avoid setting the delete-on-close bit for TempFile to [Support] PR42623: Avoid setting the delete-on-close bit if a TempFile doesn't reside on a local drive.Wed, Sep 30, 8:24 AM
rdwampler edited the summary of this revision. (Show Details)
rdwampler updated this revision to Diff 295298.Wed, Sep 30, 8:31 AM

Reverted change that removed code setting the file disposition.
Instead, added check to see if we are on a non-local filesystem (i.e., network drive), if so the disposition isn't set.