DavidSpickett added inline comments.

================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1357
+
+  if (error.Fail())
+    return error;
----------------
omjavaid wrote:
> DavidSpickett wrote:
> > omjavaid wrote:
> > > DavidSpickett wrote:
> > > > omjavaid wrote:
> > > > > DavidSpickett wrote:
> > > > > > DavidSpickett wrote:
> > > > > > > DavidSpickett wrote:
> > > > > > > > omjavaid wrote:
> > > > > > > > > ptrace request is a success if number of tags requested is 
> > > > > > > > > not equal to no of tags read? If not then this and following 
> > > > > > > > > condition may be redundant.
> > > > > > > > Well ptracewrapper doesn't check the iovec, but I'll check the 
> > > > > > > > kernel source to see if it's actually possible for it to fail 
> > > > > > > > that way.
> > > > > > > In `linux/arch/arm64/kernel/mte.c` `__access_remote_tags` there 
> > > > > > > is a comment:
> > > > > > > ```
> > > > > > > +/*
> > > > > > > + * Access MTE tags in another process' address space as given in 
> > > > > > > mm. Update
> > > > > > > + * the number of tags copied. Return 0 if any tags copied, error 
> > > > > > > otherwise.
> > > > > > > + * Inspired by __access_remote_vm().
> > > > > > > + */
> > > > > > > ```
> > > > > > > 
> > > > > > > *any tags* being the key words.
> > > > > > > 
> > > > > > > So the scenario is:
> > > > > > > * ask to read from addr X in page 0, with length of pagesize+some 
> > > > > > > so the range spills into page 1
> > > > > > > * kernel can access page 0, reads tags until the end of the page
> > > > > > > * tries to access page 1 to read the rest, fails, returns 0 
> > > > > > > (success) since *some* tags were read
> > > > > > > * we see the ptrace call succeeded but with less tags than we 
> > > > > > > expected
> > > > > > > 
> > > > > > > I don't see it's worth dealing with this corner case here since 
> > > > > > > lldb will look before it leaps. It would have errored much 
> > > > > > > earlier here because either page 1 isn't in the tracee's memory 
> > > > > > > regions or it wasn't MTE enabled.
> > > > > > > 
> > > > > > > 
> > > > > > Added a comment in the code too.
> > > > > This means emitting less than requested number of tags is legit. 
> > > > > However we have set tags vector size equal to whatever we have 
> > > > > requested. We set error string which is actually not being used 
> > > > > anywhere and can be removed in favor of a log message to help with 
> > > > > debugging if needed.
> > > > > 
> > > > > Also we need to resize the vector after ptrace request so we use this 
> > > > > size in gdb remote reply.
> > > > I'll log that error in in GDBRemoteCommunicationServerLLGS.cpp.
> > > > 
> > > > I'll do what you suggested to support partial read on the server side. 
> > > > Then lldb client can error if it doesn't get what it expected.
> > > > (testing partial reads on the lldb side is going to be very difficult 
> > > > anyway since we'd need a valid smaps entry to even issue one)
> > > If we are following an approach similar to m/M gdb remote packets for 
> > > tags then its ok to read partial data in case a part memory in requested 
> > > address range was inaccessible. May be make appropriate adjustment for 
> > > command output I dont recall what currently emit out for partial memory 
> > > reads but should be something like <tags not available>
> > > 
> > > 
> > I did some digging and lldb-server does not return partial data when a read 
> > fails. 
> > ```
> >   for (bytes_read = 0; bytes_read < size; bytes_read += remainder) {
> >     Status error = NativeProcessLinux::PtraceWrapper(
> >         PTRACE_PEEKDATA, GetID(), (void *)addr, nullptr, 0, &data);
> >     if (error.Fail())
> >       return error;
> > 
> >     remainder = size - bytes_read;
> >     <...>
> >     dst += k_ptrace_word_size;
> >   }
> >   return Status();
> > ```
> > 
> > I was able to test partial writes too. There too we don't attempt to 
> > restore if we only wrote a smaller amount, writing less than the total is a 
> > failure.
> > 
> > However, it is true that I'm not handling the syscall properly. I need to 
> > loop like readmemory does. So I'm going to do that.
> > Loop until we've read all tags, or return the error we get.
> Considering gdb remote protocol this is not complying with 'm' packet whick 
> says:
> "The reply may contain fewer addressable memory units than requested if the 
> server was able to read only part of the region of memory."
> 
> May be we can fix this in a separate patch where LLDB should emit proper 
> error code based on which either we can completely fail or send partial read 
> data. 
> What do you think ?
That would be the ideal thing to do, however I was wrong about lldb not 
supporting it at all. In fact memory read can do partial results:
```
<step to clear caches>
(lldb) memory read 0x0000fffff7ff7000-16
lldb             <  34> send packet: $qMemoryRegionInfo:fffff7ff9010#df
lldb             <  55> read packet: 
$start:fffff7ff9000;size:1000;permissions:rw;flags:;#fa
lldb             <  50> send packet: 
$Mfffff7ff9010,f:000000000000000000000000000000#84
lldb             <   6> read packet: $OK#9a
lldb             <  34> send packet: $qMemoryRegionInfo:fffff7ff9010#df
lldb             <  55> read packet: 
$start:fffff7ff9000;size:1000;permissions:rw;flags:;#fa
lldb             <  36> send packet: $Mfffff7ff9010,8:0000000000000000#b6
lldb             <   6> read packet: $OK#9a
lldb             <  34> send packet: $qMemoryRegionInfo:fffff7ff9010#df
lldb             <  55> read packet: 
$start:fffff7ff9000;size:1000;permissions:rw;flags:;#fa
lldb             <  36> send packet: $Mfffff7ff9010,8:f06ffff7ffff0000#a9
lldb             <   6> read packet: $OK#9a
lldb             <  21> send packet: $xfffff7ff9000,200#00
lldb             < 516> read packet: $<512 bytes>#53
lldb             <  21> send packet: $xfffff7ff6e00,200#32
lldb             < 516> read packet: $<512 bytes>#0a
lldb             <  21> send packet: $xfffff7ff7000,200#fe    <<<< Fails to 
read the last 16 bytes
lldb             <   7> read packet: $E08#ad
0xfffff7ff6ff0: 01 02 03 04 00 00 00 00 00 00 00 00 00 00 00 00  
................
0xfffff7ff7000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
................
warning: Not all bytes (16/32) were able to be read from 0xfffff7ff6ff0.
```

Except we're not doing it by getting a smaller reply (not in this example 
anyway), it's working because we split up the reads in such a way that they 
tend to line up with the failing addresses.

So yeah it's probably worth fixing from a correctness point of view but for 
lldb to lldb-server we've got an equivalent already.

As for MTE would you be ok with not allowing partial reads, since the spec as 
proposed does not mention them?
(what I said before but just to be sure)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to