This is an archive of the discontinued LLVM Phabricator instance.

LTO: Try to open cache files before renaming them.
ClosedPublic

Authored by pcc on Sep 1 2017, 5:56 PM.

Details

Summary

It appears that a potential race between the cache client and the cache
pruner that I thought was unlikely actually happened in practice [1].
Try to avoid the race condition by opening the temporary file before
renaming it. Do this only on non-Windows platforms because we cannot
rename open files on Windows.

[1] https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FLinux_CFI%2F1610%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout

Event Timeline

pcc created this revision.Sep 1 2017, 5:56 PM
zturner added a subscriber: zturner.Sep 1 2017, 5:59 PM

I think you actually can rename open files on Windows. Or at least, you can try to. I'll have to dig in when I have a bit more time though to see if there's any weird things we need to be aware of when doing it, or casse where it won't work.

But basically, what you do is call SetFileInformationByHandle and set the FileInformationClass parameter to FileRenameInfo, and fill out a FILE_RENAME_INFO structure to pass for the 3rd argument.

mehdi_amini edited edge metadata.Sep 1 2017, 10:40 PM

LGTM

llvm/include/llvm/LTO/Caching.h
27

The comment is not accurate I believe: the Path didn't exist when when the memory buffer was created, and it was never opened using this path.

pcc added a comment.Sep 5 2017, 12:52 PM

I think you actually can rename open files on Windows. Or at least, you can try to. I'll have to dig in when I have a bit more time though to see if there's any weird things we need to be aware of when doing it, or casse where it won't work.

But basically, what you do is call SetFileInformationByHandle and set the FileInformationClass parameter to FileRenameInfo, and fill out a FILE_RENAME_INFO structure to pass for the 3rd argument.

Thanks, I've left a FIXME about renaming the file in the way you've suggested.

llvm/include/llvm/LTO/Caching.h
27

Reworded this comment.

This revision was automatically updated to reflect the committed changes.