[PATCH] D135590: [ThinLTO][ELF] Add a ThinLTO warning if cache_size_bytes or cache_size_files is too small for the current link job.

[Ying Yi via Phabricator via llvm-commits]

MaggieYi added inline comments.


================
Comment at: lld/test/ELF/lto/cache-warnings.ll:29
+; RUN: cat %t.size1.txt \
+; RUN: | %python -c "import subprocess, sys; size = int(sys.stdin.read()); cache_dir=r'%t.cache'; subprocess.run([r'ld.lld', '--verbose', '--thinlto-cache-dir={}'.format(cache_dir), '--thinlto-cache-policy=prune_interval=0s:cache_size_bytes={}'.format(size), r'%t2.o', r'%t.o', '-o', r'%t3'], stderr=subprocess.STDOUT)" | FileCheck %s --implicit-check-not=warning:
+
----------------
tejohnson wrote:
> I'm not sure invoking lld via python subprocess is ideal for testing. I think this could be done much more simply via shell substitution via $() (you have to REQUIRE shell though). Even if you need python to compute the total file sizes initially you could then do something like
> 
> RUN: ld.lld .... --thinlto-cache-policy=prune_interval=0s:cache_size_bytes=$(($(cat size.txt) + 5)) ...
> 
> I know I have seen $(cat somefile.txt) used before, so I assume the evaluation described above should also work - can you give it a try to simplify a lot of this?
Thanks, I have simplified the tests following your suggestion.


================
Comment at: llvm/include/llvm/Support/CachePruning.h:83
+bool pruneCache(StringRef Path, CachePruningPolicy Policy,
+                const std::vector<std::unique_ptr<MemoryBuffer>> &files = {});
 } // namespace llvm
----------------
tejohnson wrote:
> If all clients are passing in files with your patch, probably just go ahead and remove the default parameter.
There is an exceptional case. In the 'lldb' project, in the constructor of 'DataFileCache', 'pruneCache' function is called before 'localCache' function, which is different from other cases. In this case, the cache pruning is not considered/caused by the current 'localCache'. The new warnings should not be given in this case. Therefore, I use the default parameter to cover the above situation.


================
Comment at: llvm/lib/Support/CachePruning.cpp:269
+  if (Policy.MaxSizeFiles && (files.size() > Policy.MaxSizeFiles))
+    llvm::errs() << "warning: ThinLTO cache pruning happens since the number "
+                    "of created files exceeds the maximum number of files; "
----------------
tejohnson wrote:
> Probably using the warning interface is better than errs(). You could probably pass in the ErrorHandler if needed.
> 
> Also, @MaskRay had an earlier suggestion about the format of the message that I think would be good to incorporate (including the number of files in the limit and the actual number of files that caused the limit to be exceeded): https://reviews.llvm.org/D135590#inline-1308533
Thanks, since warnings are moved from 'lld' to 'llvm/support', I have used llvm 'WithColor::warning()' (instead of 'lld::warn()') to generate the warning.


@MaskRay, sorry to miss your earlier suggestion. @tejohnson, thanks for your reminding.
I have updated the warning message following the suggestion. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135590/new/

https://reviews.llvm.org/D135590