[PATCH] D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning

ben via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 21 10:35:29 PST 2017


bd1976llvm added inline comments.


================
Comment at: include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:154
   void setCachePruningInterval(int Interval) {
-    if (Interval)
-      CacheOptions.Policy.Interval = std::chrono::seconds(Interval);
+      static_assert(std::is_same<decltype(CacheOptions.Policy.Interval),
+          std::chrono::seconds>::value,
----------------
kromanova wrote:
> Ben, how does this assert avoids risk of overflow? I thought this check is only to make sure that CacheOptions.Policy.Interval has type std::chrono::seconds.  
> 
> "Interval", which is a member of struct CachePruningPolicy, has type std::chrono::seconds. If we write something to Interval, the value will be implicitly converted to std::chrono::seconds, so the assert will never be triggered. Am I wrong?
> 
> Or are you adding this assert simply to prevent someone from changing the type of Interval in CachePruningPolicy without paying attention to this functionl? In this case, I still confused about "risk of overflow" comment.
Risk of overflow is later on if someone does something like:

auto deadline = system_clock::now() /*nanoseconds*/ + CacheOptions.Policy.Interval /*implicitly converted to nanoseconds*/;

By asserting the type is seconds here and using decltype else where I was trying to force every chrono type into seconds to avoid risk of overflow.


================
Comment at: lib/Support/CachePruning.cpp:158
   sys::fs::file_status FileStatus;
-  const auto CurrentTime = system_clock::now();
+  const auto CurrentTime =
+      time_point_cast<decltype(Policy.Interval)>(system_clock::now());
----------------
kromanova wrote:
> kromanova wrote:
> > Ben, 
> > In this conversion (and in the one below) you explicitly convert std::chrono::nanoseconds (system_clock::now()) to std::chrono::seconds
> > (Policy.Interval), and as a result you have CurrentTime in seconds. You do a similar explicit conversion to have TimeStampModTime (line 172) and TimeStampAge (line 174) in seconds, in order to avoid the overflow on the line 175, where Policy.Interval() could overflow when there is a max value stored in it, since "max" seconds will be converted to nanoseconds for comparison. Is my understanding of your "overflow prevention" right?
> > 
> > I personally think that these conversions make the code harder to read, especially a few lines later there is a similar conversion happening, but since there is no danger of overflow, you didn't change it. 
> > const auto FileAccessTime = StatusOrErr->getLastAccessedTime();
> > These inconsistences and tricks with conversions to avoid overflow make the code harder to understand.
> > 
> > Have you considered Pavel's suggestion about setting the interval to std::chrono::*nanoseconds*::max()? It's plenty sufficient, and we won't need explicit conversions.  
> > 
> > 
> Or maybe instead of using max(), you could define your own constant (in seconds), that is large enough to ensure that pruning won't happen in the next century, but won't overflow when converted to nanoseconds? 
> 
> Or (I know already I will be hanged for his suggestion) to add one Boolean member to CachePruningPolicy struct, that will be set to false if we don't want ever run the pruner.  
Accepted. I have now reworked the patch.


================
Comment at: test/ThinLTO/X86/cache.ll:38
 ; Verify that enabling caching is working with llvm-lto2
 ; RUN: rm -Rf %t.cache
+; RUN: llvm-lto2 run -o %t.o %t2.bc %t.bc -cache-dir %t.cache \
----------------
kromanova wrote:
> This is not directly related to Ben's review, I was just wondering why here, for llvm-lto2 we don't test that the pruner only removes files matching the pattern "llvmcache-*" how we do in the check just above with llvm-lto?
llvm-lto2 does not expose the pruning api. Instead this is checked in the linker (gold, lld) specific system tests for thin LTO.


Repository:
  rL LLVM

https://reviews.llvm.org/D41231





More information about the llvm-commits mailing list