This is an archive of the discontinued LLVM Phabricator instance.

LTO: don't fatal when value for cache key already exists
ClosedPublic

Authored by inglorion on Nov 9 2017, 3:34 PM.

Details

Summary

LTO/Caching.cpp uses file rename to atomically set the value for a
cache key. On Windows, this fails when the destination file already
exists. Previously, LLVM would report_fatal_error in such
cases. However, because the old and the new value for the cache key
are supposed to be equivalent, it actually doesn't matter which one we
keep. This change makes it so that failing the rename when an openable
file with the desired name already exists causes us to report success
instead of fataling.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Nov 9 2017, 3:34 PM
pcc added inline comments.Nov 9 2017, 4:09 PM
llvm/lib/LTO/Caching.cpp
83 ↗(On Diff #122346)

I think this can race with the cache pruner.

Maybe in this case you could create a copy of MBOrErr in memory, then close and delete the temporary file and return the copy to the client.

inglorion updated this revision to Diff 122369.Nov 9 2017, 4:43 PM

changes requested by @pcc

inglorion updated this revision to Diff 122370.Nov 9 2017, 4:45 PM

no need to inclune Support/Process.h anymore

pcc accepted this revision.Nov 9 2017, 6:04 PM

LGTM

llvm/lib/Support/Windows/Path.inc
737–738 ↗(On Diff #122370)

This is part of an unrelated change, right?

This revision is now accepted and ready to land.Nov 9 2017, 6:04 PM
inglorion updated this revision to Diff 122451.Nov 10 2017, 8:59 AM

rebased to remove unrelated changes

This revision was automatically updated to reflect the committed changes.