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

Fri Sep 30 04:14:36 PDT 2022

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added a comment.

Seems like a valid thing to do.

Is it a problem that maybe you read from address N to N+100 and the problem address is actually at N+50, not N? I think you might be handling that already but I didn't read all the logic just your additions. Maybe not an issue because a lot of this is done in pages of memory, unlikely to be a <10 byte gap where something fails (and you can manually flush the cache if that somehow happens).

> In this case, the elements often have the value 0, so lldb is trying to read memory at address 0, which fails, and the number of reads is quite large.

In this environment I guess they don't have a dynamic loader to have set the ranges? So in Mac OS userspace, this wouldn't be a problem because the zero page would be marked as always invalid, but they don't have that luxury.

And there's nothing stopping a bare metal program from mapping memory at 0, so one can't just say 0 means invalid 100% of the time.

> doesn't address the fact that m_invalid_ranges is intended to track inaccessible memory that is invariant during the process lifetime.

Please add this as a comment, will be good to know if/when there are two lists involved.

================
Comment at: lldb/source/Target/Memory.cpp:157
+  if (IsAddressUnreadable(addr)) {
+    error.SetErrorStringWithFormat("memory read failed for 0x%" PRIx64, addr);
+    return 0;
----------------
Should this be more specific like "failed because we know this is unreadable" or "has previously failed to read"?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134906