[PATCH] D37993: [ThinLTO/gold] Implement ThinLTO cache pruning support

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In https://reviews.llvm.org/D37993#918918, @tejohnson wrote:

> In https://reviews.llvm.org/D37993#912428, @pcc wrote:
>
> > In https://reviews.llvm.org/D37993#893466, @pcc wrote:
> >
> > > Is it a good idea to support cache pruning in the gold plugin? As I mentioned in https://reviews.llvm.org/D31063 there are some unavoidable race conditions due to limitations in the plugin API.
> >
> >
> > Ping... not sure whether it's a good idea to ship a known racy feature.
> >
> > If you really want this feature in gold I'd suggest extending the gold plugin API so that it accepts file descriptors, and then have the plugin support this feature conditionally on whether the gold version is new enough.
>
>
> Hi Peter, thanks for pointing this out, and sorry for the delay in responding. Wondering whether instead of removing cache pruning support from gold-plugin for now, we could do something else: write the MemoryBuffer to another temp file and use that path (setting IsTemporary to true so it gets cleaned up). This would obviously require more space on disk, but maybe worth it to enable the pruning safely.


Copying to a temporary file sounds fine to me. And if gold ever gets support for file descriptors we could always use the copying code path as a fallback for older versions.



================
Comment at: llvm/trunk/tools/gold/gold-plugin.cpp:980
+  // Prune cache
+  if (!options::cache_policy.empty()) {
+    CachePruningPolicy policy = check(parseCachePruningPolicy(options::cache_policy));
----------------
tejohnson wrote:
> pcc wrote:
> > I think this should check `cache_dir`, not `cache_policy`.
> It seems fine since pruneCache will return immediately if cache_dir is empty. But we could save some work if both were checked. I assume parseCachePruningPolicy will also work if cache_policy is empty, but it seems fine to have the checking.
I was more thinking about the behaviour when a cache directory is specified and no cache policy is specified. In that case the behaviour of the other linkers is to prune with the default policy, so it's probably best to be consistent with that.


Repository:
  rL LLVM

https://reviews.llvm.org/D37993