Page MenuHomePhabricator

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

Repository
rL LLVM

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 ↗(On Diff #70117)

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 ↗(On Diff #70117)

Can we assert success here?

584 ↗(On Diff #70117)

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 ↗(On Diff #70117)

Done.

300 ↗(On Diff #70117)

Done.

584 ↗(On Diff #70117)

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
585 ↗(On Diff #70128)

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 ↗(On Diff #70128)

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

300 ↗(On Diff #70128)

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 ↗(On Diff #70178)

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

300 ↗(On Diff #70178)

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 ↗(On Diff #70176)

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 ↗(On Diff #70181)

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

307 ↗(On Diff #70181)

s/no Windows/on Windows/

309 ↗(On Diff #70181)

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.