This is an archive of the discontinued LLVM Phabricator instance.

Remove temoprary files.
ClosedPublic

Authored by ruiu on Sep 1 2016, 5:45 PM.

Details

Summary

Previously, we created temporary files using llvm::sys::fs::createTemporaryFile
and remove them using llvm::FileRemover. This is error-prone as it is easy to
forget creating FileRemover instances after creating temporary files.
There is actually a temporary file leak bug.

This patch introduces a new class, TemporaryFile, to manage temporary files
in the RAII style.

Diff Detail

Event Timeline

ruiu updated this revision to Diff 70116.Sep 1 2016, 5:45 PM
ruiu retitled this revision from to Remove temoprary files..
ruiu updated this object.
ruiu added a reviewer: rnk.
ruiu added a subscriber: llvm-commits.
ruiu updated this revision to Diff 70117.Sep 1 2016, 5:53 PM
  • Fixed undefined behavior.
ruiu updated this object.Sep 1 2016, 6:10 PM
rnk edited edge metadata.Sep 1 2016, 6:27 PM

I don't think this works as in Windows

COFF/DriverUtils.cpp
286

Let's thread through the other parameter, so that we get "resource-NNN.obj". That was somewhat helpful for my debugging purposes, and it will show up in the debug info too.

300

Can we assert success here?

589

Won't this fail on Windows, where you cannot remove a file which has an open mapping?

ruiu updated this revision to Diff 70128.Sep 1 2016, 7:14 PM
ruiu edited edge metadata.
  • Updated as per rnk's comments.
COFF/DriverUtils.cpp
286

Done.

300

Done.

589

Surprisingly enough, it can actually deletes files. I double-checked that. Don't have a good explanation yet though.

rnk added inline comments.Sep 2 2016, 8:29 AM
COFF/DriverUtils.cpp
589

I think the explanation is that MemoryBuffer only uses MapViewOfFile when shouldUseMmap returns true (if the file is small or its size is a multiple of 4K and RequiresNullTerminator is set). I suspect if you arrange for it to return true, then this removal will fail.

ruiu updated this revision to Diff 70176.Sep 2 2016, 9:37 AM
  • Add TemporaryFile::getMemBufferCopy to create a copy of a memory buffer.

I know that in theory, TemporaryFile is very simple and unlikely to fail. In practice, though, Windows can be very finicky about deleting files, so it might be nice to add a test for TemporaryFile. I'm surprised LLVM doesn't already have such a class that's already well-tested.

COFF/DriverUtils.cpp
286

I don't think the explicit does anything here, since the constructor requires two parameters.

300

I'm concerned the assertion could fail sporadically on Windows, since there's a delay between asking for the file to be deleted and the actual deletion of the file.

ruiu updated this revision to Diff 70178.Sep 2 2016, 9:44 AM
  • Address Adrian's comments.
ruiu added a comment.Sep 2 2016, 9:47 AM

I think TemporaryFile class is well exercised by existing lit tests, but I can add a unit test if you want.

COFF/DriverUtils.cpp
286

I forgot to remove it when I added the second parameter. Removed.

300

I think the right thing to do here is to check the return value of remove(), so did I.

rnk added inline comments.Sep 2 2016, 9:51 AM
COFF/DriverUtils.cpp
307

Rather than doing map & copy, you can force MemoryBuffer::getFile to use read/ReadFile the file by passing IsVolatileSize=true. That parameter should probably be named DontUseMmap or ForceReadFile. At the time I think people were trying to document *why* you might want to not use mmap.

You should probably pass the extra parameters anyway, because we definitely don't need RequiresNullTerminator=true for an object file.

ruiu updated this revision to Diff 70181.Sep 2 2016, 10:12 AM
  • Address rnk's comments.
rnk accepted this revision.Sep 2 2016, 10:39 AM
rnk edited edge metadata.

lgtm

COFF/DriverUtils.cpp
300

If this turns out to not be reliable, we can reconsider the error check later

307

s/no Windows/on Windows/

309

s/enforces/forces/

This revision is now accepted and ready to land.Sep 2 2016, 10:39 AM
This revision was automatically updated to reflect the committed changes.