Page MenuHomePhabricator

Simplify unlinkAsync
ClosedPublic

Authored by rafael on Nov 6 2017, 6:12 PM.

Details

Reviewers
ruiu
Summary

I started to take a look at our use of temporary files, with the eventual goal of never leaving a temporary file in case of crash.

The first thing I am looking at is where we use the version of createUniqueFile that doesn't open the file. In general that can race. In lld I think there is only one use and it is simple to avoid.

It is used to create a temp name to pass to rename, but there are simpler ways of keeping a reference to a file than renaming it. In fact, just opening the file will do it.

I tested that the close is indeed where the time is spent by wrapping both in std::chrono::system_clock::now. The remove timed at about 5.065000e-06 seconds, the close at 3.852581e-01 seconds.

Diff Detail

Event Timeline

rafael created this revision.Nov 6 2017, 6:12 PM
ruiu added inline comments.Nov 7 2017, 11:04 AM
ELF/Filesystem.cpp
20

Does all system have unistd.h?

61–65

Is this safe on Windows? On Windows, you can't remove opened files.

ruiu edited edge metadata.Nov 7 2017, 12:58 PM

Do you want to replace runBackground with std::thread([=] { ::close(FD) }).detach()?

rnk added a subscriber: rnk.Nov 7 2017, 1:08 PM

Check out https://bugs.llvm.org/show_bug.cgi?id=34070, I wanted to push as much of this down into FileOutputBuffer and ToolOutputFile as possible and reduce our reliance on RemoveFileOnSignal, but I'm not actively working on it.

ruiu accepted this revision.Nov 7 2017, 1:57 PM

LGTM

ELF/Filesystem.cpp
47

I think it is better to replace this with #else to avoid a possible "unreachable code after return" warning on Windows.

This revision is now accepted and ready to land.Nov 7 2017, 1:57 PM
ruiu added a comment.Nov 7 2017, 2:02 PM

Wait, I think we should remove a given file on Windows as well to make this function's behavior consistent on all platforms.

rafael updated this revision to Diff 121989.Nov 7 2017, 2:51 PM

Also unlink on windows.

ruiu added a comment.Nov 7 2017, 2:53 PM

Please go ahead and submit.

mati865 added inline comments.
ELF/Filesystem.cpp
17

It broke lld stand-alone builds:

../ELF/Filesystem.cpp:17:32: fatal error: llvm/Config/config.h: No such file or directory
 #include "llvm/Config/config.h"
                                ^
compilation terminated.
espindola closed this revision.Mar 14 2018, 3:33 PM
espindola added a subscriber: espindola.

317631