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:
> > > > 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.


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