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

2022-10-05 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment. In D134906#3835260 , @jasonmolenda wrote: > In D134906#3832642 , @labath wrote: > >> I don't know about debugserver, but both lldb-server and gdbserver currently >> return an error when

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

2022-10-04 Thread Jason Molenda via Phabricator via lldb-commits
jasonmolenda added a comment. In D134906#3835260 , @jasonmolenda wrote: > We ask for 32 bytes starting at 0x1017c, get back 4 bytes. Then we try > to read the remaining 28 bytes starting at 0x10180, and get an error. So > this is different

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

2022-10-04 Thread Jason Molenda via Phabricator via lldb-commits
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

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

2022-10-04 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment. I don't know about debugserver, but both lldb-server and gdbserver currently return an error when the memory is partially accessible, even though the protocol explicitly allows the possibility of truncated reads. It is somewhat hard to reproduce, because the caching

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

2022-10-03 Thread Jason Molenda via Phabricator via lldb-commits
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

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

2022-10-03 Thread David Spickett via Phabricator via lldb-commits
DavidSpickett added a comment. > 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

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

2022-09-30 Thread Jim Ingham via Phabricator via lldb-commits
jingham added a comment. LGTM I didn't see a more appropriate datatype than SmallSet in the llvm collection. I wondered about the same thing Dave asked - should the errors mention that this failed because we checked a negative cache - but I think that would be more confusing than helpful to

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

2022-09-30 Thread David Spickett via Phabricator via lldb-commits
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

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

2022-09-29 Thread Jason Molenda via Phabricator via lldb-commits
jasonmolenda created this revision. jasonmolenda added reviewers: jingham, JDevlieghere. jasonmolenda added a project: LLDB. Herald added a project: All. jasonmolenda requested review of this revision. Herald added a subscriber: lldb-commits. This patch is to address an issue hit by one of our