This is an archive of the discontinued LLVM Phabricator instance.

[Windows FS] Allow moving files in TempFile::keep
ClosedPublic

Authored by jmorse on Jul 31 2018, 3:39 AM.

Details

Summary

In r338216 and https://reviews.llvm.org/D49860 we permitted MoveFileEx to copy between volumes -- but before it can, the more posix-y rename_internal fails. Allow rename_handle to fall back on MoveFileEx for this specific error.

Diff Detail

Event Timeline

jmorse created this revision.Jul 31 2018, 3:39 AM
JDevlieghere accepted this revision.EditedJul 31 2018, 3:46 AM

LGTM, Thanks for fixing this!

This revision is now accepted and ready to land.Jul 31 2018, 3:46 AM

I don't have a strong opinion, but I wonder if it would be better to map ERROR_NOT_SAME_DEVICE to errc::cross_device_link and to map ERROR_CALL_NOT_IMPLEMENTED to errc::operation_not_supported. Then you could just say if (EC == errc::cross_device_link || EC == errc::operation_not_supported)

Tests are all good. Thanks!

pcc added a subscriber: pcc.Jul 31 2018, 12:01 PM

Can't we change dsymutil to create its temporary file in the same directory as the destination?

In D50048#1183101, @pcc wrote:

Can't we change dsymutil to create its temporary file in the same directory as the destination?

I'm not sure that's the best solution. Even if we fix it for dsymutil, this seems to be a more general problem of LLVM not supporting cross-device renames. We can't control where a user's temp dir will be or what drive they're running LLVM from. So I think it's important to handle this case.

pcc added a comment.Jul 31 2018, 12:16 PM

But if we change the TempFile API so that it always creates the temporary in the same directory as the destination, that becomes a non-problem, no?

jmorse added a comment.Aug 1 2018, 3:57 AM
In D50048#1183136, @pcc wrote:

But if we change the TempFile API so that it always creates the temporary in the same directory as the destination, that becomes a non-problem, no?

My understanding of the TempFile API is that it might not know whether a file is kept or the destination it might be kept to, at the time of creation.

IMHO, this comes down the support code not currently providing a "move", hence this patch. However with this patch unix-rename and windows-rename might behave differently.

The more technically consistent solution could be to mirror Jonas' cross-device copy in https://reviews.llvm.org/D49860 TempFile::keep for Windows. The problem is then just code duplication (and a lack of "move"), rather than functions behaving differently.

I'll upload a patch that does that in a moment to demonstrate.

jmorse updated this revision to Diff 158492.Aug 1 2018, 4:23 AM
jmorse retitled this revision from [Windows FS] Fall back to MoveFileEx for rename across volumes to [Windows FS] Allow moving files in TempFile::keep.

This revision works just as well as the previous, but keeps "rename" behaving the same. Some future refactor could introduce a "move" function, this is the minimum change for now.

This revision was automatically updated to reflect the committed changes.