Re: [commit] Adjust ia64_linux_xfer_partial following to_xfer_partial API change.

[Joel Brobecker <brobecker at adacore dot com>]

> > +      gdb_byte *tmp_buf = alloca (offset + len);
> > +      ULONGEST xfered;
> > +
> > +      xfered = syscall (__NR_getunwind, readbuf, offset + len);
> 
> It seems this should have passed tmp_buf instead of readbuf?

Yes, indeed. Hasty implementation which, as you found out,
gets luck (at best).

> Also, I don't think the "offset + len" size is the
> right size to pass down to the syscall.
> 
> From:
> 
> http://lxr.free-electrons.com/source/arch/ia64/kernel/unwind.c#L2313

I thought it was OK. Basically, we were asked to get len bytes at
offset. The idea was that, since the syscall doesn't handle offsets,
we have to fetch from zero again, which means getting offset bytes
(which were going to be thrown away) + len bytes (which we'll return).

> So it looks like this code is getting lucky and the caller
> is requesting enough bytes:
> 
>   x = target_read_alloc (&current_target, TARGET_OBJECT_UNWIND_TABLE,
> 			 NULL, buf_p);

Right. I noticed that also, because I couldn't explain at the beginning
why the syscall was only performed with offset == 0 in the previous
implementation.

> But to_xfer_partial implementations should not assume that.  E.g.,
> change target_read_alloc_1 to start by requesting a byte at a time
> instead of 4096, and this will fail.
> 
> > +      if (xfered <= 0)
> 
> Note that because xfered is ULONGEST, this won't actually
> detect errors.

Indeed. I had assumed that a return value of 0 indicated an error
already, and wrote it that way; and then I thought, hey, negative
values should never happen either, right? Let's handle that too.
Duh...

> > +      else if (xfered <= offset)
> > +	return TARGET_XFER_EOF;
>
> Not sure I follow this one.

The theory was the following:

   1. First xfer request, ask ~4k at offset zero,
      return xxx bytes and OK

   2. The caller finds an OK, but only xxx bytes < 4k.
      So round #2, asks for 4k - xxx bytes at offset xxx.
      We fetch unwind table again, found that xferd is xxx again.
      Since offset is xxx, it means we're done. Return EOF.

> I suggest something along the lines of the below instead.
> Should handle partial requests, and clearer to read, I think.
> Completely untested, though.

It does look better indeed. A little more elaborate, and also
a little more cogniscent of what the syscall actually does.
I based my implementation a lot on the previous one, with
the idea in the back of my mind that I wouldn't be spending
too much time on it, because we would probably want to remove
that code one day - as it's based on a deprecated feature.
(and for good measure, I don't believe in ia64's future either)

Attached is what I checked in. There were a couple of tweaks
I needed to make (TARGET_XFER_EIO -> TARGET_XFER_E_IO, and
res was not assigned). I'm a little nervous about the gdb_assert,
as I usually prefer to use them only when everything is under
our control and we can't recover from the situation. I was
almost ready to see what would happen if we returned E_IO
instead of brutally interruping the debugging session. But
we'll see what happens with the current code.

BTW: Now that I have pushed the patch, I just realized something.
Are you sure the gate_table_size is a constant? >:-o

gdb/ChangeLog:

        * ia64-linux-nat.c (ia64_linux_xfer_partial): Reimplement
        handling of object == TARGET_OBJECT_UNWIND_TABLE.

Thanks,
-- 
Joel

>From d16461aeef555da47e358b0f81c75912e4ea07e2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Feb 2014 20:45:50 -0500
Subject: [PATCH] Re-implement ia64-linux-nat.c::ia64_linux_xfer_partial

[description of this patch and ChangeLog entry by Joel Brobecker]
The recent implementation was questionable, and if it worked, it was
only by chance because the requested length is large enough that only
one read was sufficient.  Note that the implementation before that
also made that assumption, in the form of only handling
TARGET_OBJECT_UNWIND_TABLE xfer requests when offset was zero.

gdb/ChangeLog:

        * ia64-linux-nat.c (ia64_linux_xfer_partial): Reimplement
        handling of object == TARGET_OBJECT_UNWIND_TABLE.
---
 gdb/ChangeLog        |    5 +++++
 gdb/ia64-linux-nat.c |   35 +++++++++++++++++++++++------------
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e60ec1c..16f4619 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2014-02-25  Pedro Alves  <palves@redhat.com>
+
+	* ia64-linux-nat.c (ia64_linux_xfer_partial): Reimplement
+	handling of object == TARGET_OBJECT_UNWIND_TABLE.
+
 2014-02-25  Stan Shebs  <stan@codesourcery.com>
 
 	* defs.h: Annotate comments for Doxygen.
diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
index c057b55..3cfbd76 100644
--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -850,20 +850,31 @@ ia64_linux_xfer_partial (struct target_ops *ops,
 {
   if (object == TARGET_OBJECT_UNWIND_TABLE && readbuf != NULL)
     {
-      gdb_byte *tmp_buf = alloca (offset + len);
-      ULONGEST xfered;
-
-      xfered = syscall (__NR_getunwind, readbuf, offset + len);
-      if (xfered <= 0)
+      static long gate_table_size;
+      gdb_byte *tmp_buf;
+      long res;
+
+      /* Probe for the table size once.  */
+      if (gate_table_size == 0)
+        gate_table_size = syscall (__NR_getunwind, NULL, 0);
+      if (gate_table_size < 0)
 	return TARGET_XFER_E_IO;
-      else if (xfered <= offset)
+
+      if (offset >= gate_table_size)
 	return TARGET_XFER_EOF;
-      else
-	{
-	  memcpy (readbuf, tmp_buf + offset, xfered - offset);
-	  *xfered_len = xfered - offset;
-	  return TARGET_XFER_OK;
-	}
+
+      tmp_buf = alloca (gate_table_size);
+      res = syscall (__NR_getunwind, tmp_buf, gate_table_size);
+      if (res < 0)
+	return TARGET_XFER_E_IO;
+      gdb_assert (res == gate_table_size);
+
+      if (offset + len > gate_table_size)
+	len = gate_table_size - offset;
+
+      memcpy (readbuf, tmp_buf + offset, len);
+      *xfered_len = len;
+      return TARGET_XFER_OK;
     }
 
   return super_xfer_partial (ops, object, annex, readbuf, writebuf,
-- 
1.7.9.5