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

Tue Oct 4 23:42:17 PDT 2022

labath added a comment.

In D134906#3835260 <https://reviews.llvm.org/D134906#3835260>, @jasonmolenda wrote:

> In D134906#3832642 <https://reviews.llvm.org/D134906#3832642>, @labath wrote:
>
>> 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 mechanism in lldb aligns memory reads, and the cache "line" size is usually smaller than the page size -- which is probably why this behavior hasn't bothered anyone so far. Nonetheless, I would say that this behavior (not returning partial contents) is a (QoI) bug, but the fact that two stubs have it makes me wonder how many other stubs do the same as well..
>
> Hi Pavel, thanks for pointing this out.  I did a quick check with debugserver on darwin, using `memory region` to find the end of an accessible region in memory, and starting a read request a little earlier, in readable memory:
>
>   (lldb) sett set target.process.disable-memory-cache true
>   
>   (lldb) mem region 0x0000000101800000
>    <  31> send packet: $qMemoryRegionInfo:101800000#ce
>    <  34> read packet: $start:101800000;size:6a600000;#00
>   [0x0000000101800000-0x000000016be00000) ---
>   
>   (lldb) mem region 0x0000000101800000-4
>    <  31> send packet: $qMemoryRegionInfo:1017ffffc#d8
>    < 122> read packet: $start:101000000;size:800000;permissions:rw;dirty-pages:101000000,101008000,10100c000,1017fc000;type:heap,malloc-small;#00
>   [0x0000000101000000-0x0000000101800000) rw-
>   
>   (lldb) x/8wx 0x0000000101800000-4
>   
>    <  17> send packet: $x1017ffffc,20#ca
>    <   8> read packet: $00000000#00
>   
>    <  17> send packet: $x101800000,1c#f2
>    <   7> read packet: $E80#00
>   
>   0x1017ffffc: 0x00000000 0x00000000 0x00000000 0x00000000
>   0x10180000c: 0x00000000 0x00000000 0x00000000 0x00000000
>   warning: Not all bytes (4/32) were able to be read from 0x1017ffffc.
>   (lldb) 
>
> We ask for 32 bytes starting at 0x1017ffffc, get back 4 bytes.  Then we try to read the remaining 28 bytes starting at 0x101800000, and get an error. So this is different behavior from other stubs where you might simply get an error for the request to read more bytes than are readable.

Yes, that's pretty much what I did, except that I was not able to read any data with the caching turned off.

In D134906#3835291 <https://reviews.llvm.org/D134906#3835291>, @jasonmolenda wrote:

> to be clear, I think I'll need to abandon this.

I don't think this is necessarily a lost cause. I mean, the debugserver behavior (truncated reads) is definitely better here, and the caching of negative acesses makes sense. And, as the example above shows, the current behavior with the non-truncating stubs is already kind of broken, because you cannot read the areas near the end of mapped space without doing some kind of a bisection on the size of the read (we could implement the bisection in lldb, but... ewww).

The safest way to pursue this would be to have the stub indicate (maybe via qSupported) its intention to return truncated reads, and then key the behavior off of that. However, if that's not possible (it seems you have some hardware stub here), I could imagine just enabling this behavior by default. We can definitely change the lldb-server behavior, and for the rest, we can tell them to fix their stubs.

That is, if they even notice this. The memory read alignment hides this problem fairly well. To demonstrate this, we've had to turn the caching off -- but that would also turn off the negative cache, and avoid this problem. So, if someone can't fix their stub, we can always tell them to turn the cache off as a workaround.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134906