This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Simplify temporary file handling.
ClosedPublic

Authored by JDevlieghere on Jul 26 2018, 8:34 AM.

Details

Summary

Dsymutil's update functionality was broken on Windows because we tried to rename a file while we're holding open handles to that file. TempFile provides a solution for this through its keep(Twine) method. This patch changes dsymutil to make use of that functionality.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Jul 26 2018, 8:34 AM
JDevlieghere added inline comments.Jul 26 2018, 8:44 AM
llvm/lib/Support/Path.cpp
1163

Should we have something similar for Windows?

jmorse accepted this revision.Jul 26 2018, 9:45 AM

LGTM, I get 1 Unsupported (due to Darwin) and 44 successful passes when running lit over the dsymutil\X86\ tests, Windows 10 with a MSVC 2015 build environment. No failures.

Not clear what to do about having MoveFileEx move between volumes, see inline comment, it might best to add the "copy allowed" flag in a follow up commit that's reviewed by someone more familiar with LLVMs filesystem handling.

llvm/lib/Support/Path.cpp
1163

The Windows implementation uses MoveFileEx and explicitly supports that kind of rename:

https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-movefileexa

But it wants a flag, "MOVEFILE_COPY_ALLOWED" in the call to MoveFileEx in that case. Adding that to the MoveFileExW call in Windows' Path.inc would be sufficient: off the top of my head I can't imagine circumstances where this interfered with other LLVM code.

This revision is now accepted and ready to land.Jul 26 2018, 9:45 AM

That's 1 Unsupported 43 Passes (apparently I can't count).

This revision was automatically updated to reflect the committed changes.

All three tests started failing in our environment after this change:

Failing Tests (3):

LLVM :: tools/dsymutil/X86/accelerator.test
LLVM :: tools/dsymutil/X86/update-one-CU.test
LLVM :: tools/dsymutil/X86/update.test

The error for all three is:

Error : while keeping C:\Windows\SERVIC~2\NETWOR~1\AppData\Local\Temp\lit_tmp_l55xwfv6\dsym.tmp18368.dwarf as E:\_work\14\b\LLVMBuild\test\tools\dsymutil\X86\Output\accelerator.test.tmp.apple.dSYM\Contents\Resources\DWARF\basic.macho.x86_64: The system cannot move the file to a different disk drive.

By the way, our tests on this particular machine run on the E: drive and it looks like the temporary file is created on the C: drive. This is the command that ends up causing the failure:

"E:\_work\14\b\LLVMBuild\Release\bin\dsymutil.EXE" "--update" "E:\_work\14\b\LLVMBuild\test\tools\dsymutil\X86\Output\update.test.tmp.dir/basic.dSYM"

That sounds to me like a bug in LLVM support libraries. TempFile should
be able to work across drives. It's also a Windows specific bug, so
perhaps the best thing to do is to fix this bug in LLVM.

Thanks for the report, it looks like just adding the "copy between volumes" flag to windows' rename was insufficient -- a preceding rename attempt (with slightly different semantics) needs to fall through to it.

I've uploaded https://reviews.llvm.org/D50048 which appears to fix this locally for me (Win10), @stella.stamenova if you could confirm whether it fixes it for you?