[Lldb-commits] [PATCH] D134906: Have MemoryCache cache addresses that were unreadable, so duplicate read attempts can be suppressed

Mon Oct 3 09:31:02 PDT 2022

jasonmolenda added a comment.

Jim & David, thanks so much for the comments.  The most important question of correctness that David asks is whether we might try to read 32k from an address and mark that address as inaccessible because a page 24k into the memory read failed (for instance).  I tried to be conservative and only mark an address as inaccessible when we were able to read 0 bytes (ReadMemory returns partial reads).

I think adding a little hint in the error message, "(cached)" would make it clearer to an lldb maintainer what happened, and not distract the user with too much.

In D134906#3830290 <https://reviews.llvm.org/D134906#3830290>, @DavidSpickett wrote:

>> If we wanted to track these decisions it would be more appropriate to log them, but I'm not sure even that is necessary.
>
> Yeah this is a better idea, if we do it at all.
>
> Let me rephrase the question. If I had a memory read failing and I suspected that the cache was marking it as unreadable how would I confirm that? If there's a way to do that when it's really needed, then we're good.

When I was testing/debugging this, I did it by turning the packet log on :)

> Perhaps log only when a new address is added to the unreadable list? Then with the memory log plus the packet log you could figure out the issue, even if you didn't know that the cache had this unreadable address feature before you started investigating.

Yes, I can add a log message to Process when I add an address, good idea.

Let me go through the patch again carefully to make sure I'm correctly handling partially-successful reads (not putting anything in the inaccessible memory cache) and tweak that error message a tad & add a bit of logging.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134906