[Lldb-commits] [PATCH] D77790: [NFC] Add a test for the inferior memory cache (mainly L1)

Fri Apr 10 16:09:52 PDT 2020

clayborg added a comment.

In D77790#1974047 <https://reviews.llvm.org/D77790#1974047>, @jarin wrote:

> Regarding the callback idea, I have bad experience with callbacks because they break if the code is not crafted for re-entrancy and they are much harder to understand. That change feels out of scope for just adding a test and fixing an unrelated bug.

The code is already there and working. Not sure what the worry about re-entrancy is from?

> Adding the SetCacheLineSize method sounds good, but if we want to keep this patch making NFC (which is what I would prefer), it would have to be called at exactly the same places as Clear. How about Pavel's idea to rename Clear -> Reset, and leave the refactoring to SetCacheLineSize for later?

We are changing the code so that just to make it NFC we have to check it in some half baked state? I don't agree with this. Clear() didn't take a parameter before, nor should it have to now.

> It appears it is really hard to reach agreement about this, so another alternative is I submit a bug report about the L1 <https://reviews.llvm.org/L1> invalidation problem and leave it to you to figure this out. In the mean time, we will fix the bug only in our private fork of lldb. Greg, perhaps you would prefer that?

I don't see this as really that hard to fix correctly. Feel free to do what you need to if this is too much.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77790