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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

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.



================
Comment at: llvm/trunk/tools/gold/gold-plugin.cpp:980
+  // Prune cache
+  if (!options::cache_policy.empty()) {
+    CachePruningPolicy policy = check(parseCachePruningPolicy(options::cache_policy));
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D37993