search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.65k stars 11.84k forks source link

Clang format creates unwanted temporary files when batching processing on windows #26499

Closed llvmbot closed 7 years ago

llvmbot commented 8 years ago
Bugzilla Link 26125
Resolution FIXED
Resolved on Feb 27, 2017 15:13
Version trunk
OS Windows NT
Reporter LLVM Bugzilla Contributor
CC @nico

Extended Description

I am on windows 7 with clang-format built from source with visual studio 2013, clang-format works fine when executed on a single file, however when using the below shell script to execute on a batch of files

FOR /R %%k in (.cpp .h *) DO C:\llvm\build\Release\bin\clang-format.exe -i %%k

the console output shows

​0 0x000000013f7d9497 (C:\llvm\build\Release\bin\clang-format.exe+0x29497)

​1 0x000000013f7dad6a (C:\llvm\build\Release\bin\clang-format.exe+0x2ad6a)

​2 0x000000013f81a833 (C:\llvm\build\Release\bin\clang-format.exe+0x6a833)

​3 0x000000013f81ae57 (C:\llvm\build\Release\bin\clang-format.exe+0x6ae57)

​4 0x000000013f7b560f (C:\llvm\build\Release\bin\clang-format.exe+0x560f)

​5 0x000000013f7b6f1a (C:\llvm\build\Release\bin\clang-format.exe+0x6f1a)

​6 0x000000013f85dacf (C:\llvm\build\Release\bin\clang-format.exe+0xadacf)

​7 0x0000000077a95a4d BaseThreadInitThunk (C:\Windows\system32\kernel32.dll+0x15a4d)

​8 0x0000000077ccb831 RtlUserThreadStart (C:\Windows\SYSTEM32\ntdll.dll+0x2b831)

and some temporary files got created

git status shows

include/test1.h-c27f6afe src/test1.cpp~RF1f2aeaee.TMP src/test2.cpp~RF1f2aeb1c.TMP src/test3.cpp~RF1f2aeb3c.TMP

A quick google tells me it's because clang format is not exiting correctly therefore the tmp files were not deleted. Any thoughts?

Thanks, Jason

nico commented 7 years ago

http://llvm.org/viewvc/llvm-project?view=revision&revision=296408 , maybe it'll stick this time :-)

llvmbot commented 7 years ago

Read the MS docs, now I understand what's happening. Didn't expect Win to throw an extra temp file into the mix, sorry for the noise.

llvmbot commented 7 years ago

Later when trying to overwrite the changed file, kernel!ReplaceFile() renames the original file to ~RFxxxx.TMP, creates a new file with

, copies formatted content to it, and finally deletes ~RFxxxx.TMP. Because there is still a open file mapping section to the original file (now with new file name ~RFxxxx.TMP), delete fails with 0xc0000121 - STATUS_CANNOT_DELETE, leaving the temp file untouched after the clang-format operation.

I'm somewhat confused - the code looks like what it should do is:

If anything I'd expect that step to fully fail. What am I missing?

nico commented 7 years ago

Trying this again at https://reviews.llvm.org/D30385

nico commented 7 years ago

Fix got reverted here: http://llvm.org/viewvc/llvm-project?view=revision&revision=296237 with reason

"""

Revert r296166, "clang-format: Don't leave behind temp files in -i mode on Windows, llvm/llvm-project#26499 ", and r296171.

(MemoryBuffer)Code.reset() was too early.

==26912== Invalid read of size 1 ==26912== at 0x437E1D: llvm::MemoryBuffer::init(char const, char const, bool) (MemoryBuffer.cpp:47) ==26912== by 0x438013: (anonymous namespace)::MemoryBufferMem::MemoryBufferMem(llvm::StringRef, bool) (MemoryBuffer.cpp:86) ==26912== by 0x438128: llvm::MemoryBuffer::getMemBuffer(llvm::StringRef, llvm::StringRef, bool) (MemoryBuffer.cpp:112) ==26912== by 0x4E189D: clang::vfs::detail::(anonymous namespace)::InMemoryFileAdaptor::getBuffer(llvm::Twine const&, long, bool, bool) (VirtualFileSystem.cpp:443) ==26912== by 0x4DF5BA: clang::vfs::FileSystem::getBufferForFile(llvm::Twine const&, long, bool, bool) (VirtualFileSystem.cpp:94) ==26912== by 0x4B72EC: clang::FileManager::getBufferForFile(clang::FileEntry const, bool, bool) (FileManager.cpp:443) ==26912== by 0x4C1F81: clang::SrcMgr::ContentCache::getBuffer(clang::DiagnosticsEngine&, clang::SourceManager const&, clang::SourceLocation, bool) const (SourceManager.cpp:98) ==26912== by 0x4C50E5: clang::SourceManager::getBufferData(clang::FileID, bool*) const (SourceManager.cpp:689) ==26912== by 0x58E794: clang::Rewriter::getEditBuffer(clang::FileID) (Rewriter.cpp:230) ==26912== by 0x407297: clang::format::format(llvm::StringRef) (ClangFormat.cpp:311) ==26912== by 0x4078D7: main (ClangFormat.cpp:363) """

http://llvm-cs.pcc.me.uk/tools/clang/lib/Rewrite/Rewriter.cpp#446 is what writes the reformatted file, and AtomicallyMovedFile right above does the actual work. Resetting the mapping at the start in that dtor would probably be the right place.

nico commented 7 years ago

I landed your fix together with a test in http://llvm.org/viewvc/llvm-project?view=revision&revision=296166 . Thanks for the patch!

nico commented 7 years ago

Note to self: To trigger this, the input file needs to be at least 16385 byte (2**14 + 1) (else MapViewOfFile() will probably just read the whole input into memory), and clang-format -i must perform some change (else clang-format won't try to write the file).

llvmbot commented 7 years ago

Fix for bug 26125 [clang-format] Remove temp file on Windows

Summary

A file mapping object prevents the temp file from being deleted. This is a 100% repro on Windows.

Details

When formatting a file in place, the original file is mapped into memory for reading.

0:000> kcL
 # Call Site
00 KERNELBASE!MapViewOfFile
01 clang_format!llvm::sys::fs::mapped_file_region::init [llvm\src\lib\Support\Windows\Path.inc]
02 clang_format!llvm::sys::fs::mapped_file_region::mapped_file_region
03 clang_format!'anonymous namespace'::MemoryBufferMMapFile::MemoryBufferMMapFile
04 clang_format!getOpenFileImpl
05 clang_format!getFileAux
06 clang_format!llvm::MemoryBuffer::getFile
07 clang_format!llvm::MemoryBuffer::getFileOrSTDIN
08 clang_format!clang::format::format
09 clang_format!main

Later when trying to overwrite the changed file, kernel!ReplaceFile() renames the original file to ~RFxxxx.TMP, creates a new file with , copies formatted content to it, and finally deletes ~RFxxxx.TMP. Because there is still a open file mapping section to the original file (now with new file name ~RFxxxx.TMP), delete fails with 0xc0000121 - STATUS_CANNOT_DELETE, leaving the temp file untouched after the clang-format operation.

0:000> kcL
 # Call Site
00 KERNELBASE!DeleteFileW
01 KERNELBASE!ReplaceFileExInternal
02 KERNELBASE!ReplaceFileW
03 clang_format!llvm::sys::fs::rename
04 clang_format!`anonymous namespace'::AtomicallyMovedFile::~AtomicallyMovedFile
05 clang_format!clang::Rewriter::overwriteChangedFiles
06 clang_format!clang::format::format
07 clang_format!main
0:000> du @​rcx
000001f2`2c2e6b90  "util.h~RF2e251ab9.TMP"
0:000> gu
KERNELBASE!ReplaceFileExInternal+0x1290:
0:000> !gle
LastErrorValue: (Win32) 0x5 (5) - Access is denied.
LastStatusValue: (NTSTATUS) 0xc0000121 - An attempt has been made to remove a file or directory that cannot be deleted.

Fix

Releasing the file mapping object early allows deletefile to proceed

llvmbot commented 8 years ago

The interesting question is - why doesn't it exit correctly?