Pass PacketBuf as an argument of API

[bet4it]

Description

• Update the other qXfer-backed protocol extensions to use the (length: usize, offset: u64, buf: &mut [u8]) API
  • At the moment, I think this is just MemoryMap and TargetXmlOverride
• Update the vFile API to use the &mut [u8] PacketBuf instead of the current callback-based approach.
• Update the vFile API to use plain 'ol usize for length and offset parameters, instead of Target::Arch::Usize
  • Context: Support read exec-file #69 (comment)

Originally posted by @daniel5151 in #69 (comment)

API Stability

• This PR does not require a breaking API change

Checklist

• Implementation
  • cargo build compiles without errors or warnings
  • cargo clippy runs without errors or warnings
  • cargo fmt was run
  • All tests pass
• Documentation
  • rustdoc + approprate inline code comments
  • Updated CHANGELOG.md
  • (if appropriate) Added feature to "Debugging Features" in README.md
• If implementing a new protocol extension IDET
  • Included a basic sample implementation in examples/armv4t
  • Included output of running examples/armv4t with RUST_LOG=trace + any relevant GDB output under the "Validation" section below
  • Confirmed that IDET can be optimized away (using ./scripts/test_dead_code_elim.sh and/or ./example_no_std/check_size.sh)
  • OR Implementation requires adding non-optional binary bloat (please elaborate under "Description")
• If upstreaming an Arch implementation
  • I have tested this code in my project, and to the best of my knowledge, it is working as intended.

[daniel5151]

Nice, looking good. Definitely simplified a lot of the callback-code. Thanks again for pointing out that relevant line in the spec - I'm glad we can simplify this API!

Left a couple of inline comments, but on the whole, this is looking good.

[daniel5151]

interesting... it seems like in this case, it would actually be impossible to re-use the entire PacketBuf to send the response. This implies that #70 might not be possible for all cases.

[bet4it]

It may be not impossible with the current code structure, but we can always find a point where the data in buf is used and we don't start to use buf to prepare the response, so it's still possible?

If we are allowed to use multiple mutable reference ( like C ), we can assure it will safe to do it?

[daniel5151]

I'm not entirely sure what you mean... but the point I was trying to make is that so long as the API signature has a filename: &[u8] that points somewhere into the middle of the PacketBuf, buf: &mut [u8] cannot point to the start of the packet buf, as you would have an aliased mutable pointer, which would be very bad in Rust.

Off the top of my head, if we really wanted to enable this use-case, we could do something like:

struct ReadLinkPacket<'a> {
    filename_start: usize,
    filename_end: usize,
    buf: &'a mut [u8]
}

impl<'a> ReadLinkPacket<'a> {
    fn filename(&self) -> &[u8] {
        &self.buf[self.filename_start..self.filename_end]
    }

    fn into_buf(self) -> &'a mut [u8] {
        self.buf
    }
}

fn readlink(&mut self, filename_and_buf: ReadLinkPacket<'_>) -> HostIoResult<usize, Self> {
    let filename = filename_and_buf.filename();
    let link = std::fs::read_link(filename) // etc...
    let buf = filename_and_buf.into_buf();
    // compose response using buf
}

Also, while thinking about this, I stumbled across an interesting observation: if you check the man page for the underlying readlink syscall, you'll find that the filename buffer and the output buffer are actually marked restrict, which means it wouldn't be possible to read the resolved link path directly into the buffer containing the filename. i.e: you must either allocate a new buffer to read the resolved path into, or use memory disjoint from the filename buf, regardless if you were using Rust or C.

FWIW, I think the current implementation that doesn't use 100% of the PacketBuf is totally reasonable, especially since the underlying packet doesn't even have a mechanism of relaying "partial reads", and assumes that the packetbuf has enough space to fit the response.

[bet4it]

Let's take a look at how GDB implements it:

#if !defined (PATH_MAX) || (PATH_MAX > (PBUFSIZ / 2 + 1))
#  define HOSTIO_PATH_MAX (PBUFSIZ / 2 + 1)
#else
#  define HOSTIO_PATH_MAX PATH_MAX
#endif

static void
handle_readlink (char *own_buf, int *new_packet_len)
{
  char filename[HOSTIO_PATH_MAX], linkname[HOSTIO_PATH_MAX];
  char *p;
  int ret, bytes_sent;

  p = own_buf + strlen ("vFile:readlink:");

  if (require_filename (&p, filename)
      || require_end (p))
    {
      hostio_packet_error (own_buf);
      return;
    }

  if (hostio_fs_pid != 0 && the_target->multifs_readlink != NULL)
    ret = the_target->multifs_readlink (hostio_fs_pid, filename,
					linkname,
					sizeof (linkname) - 1);
  else
    ret = readlink (filename, linkname, sizeof (linkname) - 1);

  if (ret == -1)
    {
      hostio_error (own_buf);
      return;
    }

  bytes_sent = hostio_reply_with_data (own_buf, linkname, ret, new_packet_len);

  /* If the response does not fit into a single packet, do not attempt
     to return a partial response, but simply fail.  */
  if (bytes_sent < ret)
    sprintf (own_buf, "F-1,%x", FILEIO_ENAMETOOLONG);
}

So we have two ways to implement it:

1. Copy the filename into a temporary buf allocated on the stack and pass this buf to ops.readlink.
2. Allocate two global buf with the same size: PacketBufIn and PacketBufOut. This will cost twice of the memory, but the implementation will be very easy.

[daniel5151]

1. Copy the filename into a temporary buf allocated on the stack and pass this buf to ops.readlink.

This forces gdbstub to arbitrarily decide what a reasonable "max path" size would be, which is something I've avoided. This is actually one of those cases where a callback-based output function would be ideal, since it gives the calling code the ability to allocate a stack-allocated buffer of whatever size it deems appropriate, and pass it back into gdbstub code.

2. Allocate two global buf with the same size: PacketBufIn and PacketBufOut. This will cost twice of the memory, but the implementation will be very easy.

I feel like this is something I brought up in the past (or maybe I just thought of it and didn't write it down), but yet, this would be the easiest way to handle these sorts of cases. The gdbstub build could be modified to accept an optional PacketBufOut, and the handler code would check for it's existence when deciding which PacketBuf to pass to the target implementation.

[bet4it]

With sufficient plumbing, we could make this a bit "smarter" by making the gdbstub constructor take a const-generic value for the PacketBuf len, and then use that value as a stack allocated buf

Then I think it's not a bad idea? We can implement this way.

[daniel5151]

As I mentioned in #72 (comment), I would like to shelve this discussion for now, and if you feel strongly about this limitation, feel free to open a new tracking issue for it, linking back to this thread + summarizing the various options both you and I have proposed.

[daniel5151]

i.e: please give this comment a thumbs up if you're good to merge the PR as is, and I'll give it one final review to approve the code, at which point we'll merge this into dev/0.6.

[bet4it]

I think we can continue this discussion in #70?

[daniel5151]

Ahh, yes, I am a silly man who is reviewing code far too late at night. Yes, we can certainly just continue discussing this at #70

I'll review this code one last time tomorrow morning, and probably merge it then as well 👍

[bet4it]

Do I need to change the type of all the target_description_xml?

And I really don't like the copy_from_slice code all over the place. We may need some awesome macro or function to wrap it?

[daniel5151]

Do I need to change the type of all the target_description_xml?

Are you referring to the target_description_xml that's part of the Arch trait? If so, no, you don't, as that is a piece of static metadata associated with the Arch, and will be guaranteed to exist as a slice somewhere in the binary's static memory.

And I really don't like the copy_from_slice code all over the place. We may need some awesome macro or function to wrap it?

From what I gather, most instances of repetitive copy_from_slice code occurs in the armv4t example code, where you're reading from static buffers? If so, I think it's reasonable to add a helper function somewhere in the crate (e.g: in examples/armv4t/gdb/mod.rs) to wrap this common pattern.

Notably, I don't think we want this helper function to be part of the gdbstub crate API itself.

[daniel5151]

a few nits, but I think this is about ready to merge (feel free to un-draft)

[bet4it]

We don't pass the entire PacketBuf to readlink now, do we need to consider the situation that buf is not enough to hold the result of readlink?

[daniel5151]

We don't pass the entire PacketBuf to readlink now, do we need to consider the situation that buf is not enough to hold the result of readlink?

You've seen the reference gdbserver code yourself:

  /* If the response does not fit into a single packet, do not attempt
     to return a partial response, but simply fail.  */
  if (bytes_sent < ret)
    sprintf (own_buf, "F-1,%x", FILEIO_ENAMETOOLONG);

i.e: this detail is something the user should handle themselves.

...that said, I think this is an important point worth stressing in the readlink docs. I'll leave a suggested doc change as a review comment...

[bet4it]

The reference is about the situation that the length of readlink result is larger than PacketSize.
But now the length of buf argument may be much smaller than PacketSize. For example, PacketSize is set to 500, and filename is 490, there may no extra spaces in buf, so readlink will always fail, but the situation shouldn't fail in actually.

[daniel5151]

I see... this ties back into the discussion we are having here: #72 (comment)

Let's keep the conversation focused, and continue discussing this on that particular comment thread.

[bet4it]

What I want to say here is that if we don't use entire PacketBuf, the behavior of readlink is not suitable in some situations.

[daniel5151]

Yep, I totally get that. The question now becomes whether or not those situations are "reasonable", and whether this particular PR should try to address these situations.

IMO, I think the answer to both of those questions is "no", and that we ought to merge this PR as is.

If this is an edge case you're particularly worried about, we can continue this discussion over at #70 (summarizing our discussion in #72 (comment)) about this limitation, and reconsider how/whether this should be fixed at some later point (e.g: once a real-world example of the issue crops up, and/or at some point before committing to a hypothetical stable 1.0.0 API)

[daniel5151]

a few last minute things I noticed...

[daniel5151]

This LGTM!

I'll go ahead and merge it (unless you have any last-minute objections)