Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Thanks for working on adding this missing feature! Please add a test and include an update to the info on where cache pruning is supported in ThinLTO.rst (for examples of both, see https://reviews.llvm.org/D37607, where it was added to lld).
llvm/trunk/tools/gold/gold-plugin.cpp | ||
---|---|---|
229 ↗ | (On Diff #115746) | Did you consider using the same option name as lld for consistency? |
llvm/trunk/tools/gold/gold-plugin.cpp | ||
---|---|---|
229 ↗ | (On Diff #115746) | I named this to be consistent with gold plugin's name for cache directory, "cache-dir". |
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.
llvm/trunk/tools/gold/gold-plugin.cpp | ||
---|---|---|
980 ↗ | (On Diff #115746) | 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. |
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.
llvm/trunk/tools/gold/gold-plugin.cpp | ||
---|---|---|
980 ↗ | (On Diff #115746) | 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. |
Yi, can you address these suggestions?
llvm/trunk/tools/gold/gold-plugin.cpp | ||
---|---|---|
980 ↗ | (On Diff #115746) | Oh, I see what you are saying. Yes, this makes sense. |
Ping.
llvm/trunk/tools/gold/gold-plugin.cpp | ||
---|---|---|
980 ↗ | (On Diff #115746) | Please at least address this. |