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

[ben via Phabricator via llvm-commits]

bd1976llvm added a comment.

In https://reviews.llvm.org/D41231#956664, @labath wrote:

> In https://reviews.llvm.org/D41231#955453, @bd1976llvm wrote:
>
> > I like your proposal.
> >
> > However, as the cache code is shared between multiple api's and only the c api has this disabling behaviour for negative values a fix here is more appropriate..
>
>
> I don't quite agree with that conclusion -- the fact we have one upper layer that supports feature X means that the lower layer needs to support it as well (the other front-ends can just choose not to use it).


Sorry, I shouldn't have used the word disabling it has caused confusion. The intent of negative values in the c-api was always simply to set the interval to maximum not to disable (although practically there is no difference). 
That is why I don't want to change the backend. None of the api's including the c-api support disabling so we don't need to implement it.

> I don't quite agree with that conclusion -- the fact we have one upper layer that supports feature X means that the lower layer needs to support it as well (the other front-ends can just choose not to use it). If one front-end is relying on an undocumented quirk of a back-end to implement its feature (which the implicit signed-to-unsigned conversion was, and was probably the reason I did not notice this when doing the conversion) then you get problems.

The reliance on signed-to-unsigned is an implementation detail to achieve the documented effects.
I think that the reason you didn't spot this is that this area of code is untested. We have excised it as part of our release process which is why I am only flagging these problems now.

> That said, I don't think it's my place to ask you to do this extra work, and I agree that we should fix the regression. I just felt I should point out a potential issue with this approach, but I am leaving the final decision in the hands of the respective owner(s).
> 
>> 

Would you be happy to accept this patch to fix the regression? I will then author a further patch to address your concerns in the backend.

>> I have updated the patch to be more defensive, hopefully this addresses your concerns.
> 
> Somewhat, but the conversion here was not my primary concern. I am more worried about some code down the line writing
>  `auto deadline = system_clock::now() /*nanoseconds*/ + CacheOptions.Policy.Interval /*implicitly converted to nanoseconds*/;`

I agree that this is a concern - thanks for pointing it out - however, your suggested fix does not prevent the problem here as the api's all allow setting values for Interval that will overflow.
Overflow was also a concern before the change to use std::chrono.
I think that any additions in the backend should be made safer. However, I would like to do this in another change as it is not directly related to this regression.


https://reviews.llvm.org/D41231