Page MenuHomePhabricator

Simplify unlinkAsync

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



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

Does all system have unistd.h?


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, 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



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.

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.