[commit] Adjust ia64_linux_xfer_partial following to_xfer_partial API change.

[commit] Adjust ia64_linux_xfer_partial following to_xfer_partial API change.

[Joel Brobecker]

Hello,

ia64-linux-nat.c no longer compiles because ia64_linux_xfer_partial
no longer matches the to_xfer_partial prototype.  This patch fixes
the problem by adjusting it accordingly.

gdb/ChangeLog:

        * ia64-linux-nat.c (ia64_linux_xfer_partial): Add function
        documentation.  Adjust prototype to match the target_ops
        to_xfer_partial method.  Adjust implementation accordingly.

The official testsuite badly crashed our ia64-linux machine :-(.
But I was able to run the AdaCore testsuite to test this change.

Pushed.

---
 gdb/ChangeLog        |    6 ++++++
 gdb/ia64-linux-nat.c |   28 +++++++++++++++++++++++-----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d3ae4f2..b2385c1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2014-02-25  Joel Brobecker  <brobecker@adacore.com>
+
+	* ia64-linux-nat.c (ia64_linux_xfer_partial): Add function
+	documentation.  Adjust prototype to match the target_ops
+	to_xfer_partial method.  Adjust implementation accordingly.
+
 2014-02-25  Hui Zhu  <hui@codesourcery.com>
 
 	* target.h (target_ops): Fix TARGET_DEFAULT_RETURN of
diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
index ccd55b2..c057b55 100644
--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -838,18 +838,36 @@ ia64_linux_store_registers (struct target_ops *ops,
 
 static target_xfer_partial_ftype *super_xfer_partial;
 
-static LONGEST 
+/* Implement the to_xfer_partial target_ops method.  */
+
+static enum target_xfer_status
 ia64_linux_xfer_partial (struct target_ops *ops,
 			 enum target_object object,
 			 const char *annex,
 			 gdb_byte *readbuf, const gdb_byte *writebuf,
-			 ULONGEST offset, ULONGEST len)
+			 ULONGEST offset, ULONGEST len,
+			 ULONGEST *xfered_len)
 {
-  if (object == TARGET_OBJECT_UNWIND_TABLE && writebuf == NULL && offset == 0)
-    return syscall (__NR_getunwind, readbuf, len);
+  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)
+	return TARGET_XFER_E_IO;
+      else if (xfered <= offset)
+	return TARGET_XFER_EOF;
+      else
+	{
+	  memcpy (readbuf, tmp_buf + offset, xfered - offset);
+	  *xfered_len = xfered - offset;
+	  return TARGET_XFER_OK;
+	}
+    }
 
   return super_xfer_partial (ops, object, annex, readbuf, writebuf,
-			     offset, len);
+			     offset, len, xfered_len);
 }
 
 /* For break.b instruction ia64 CPU forgets the immediate value and generates
-- 
1.7.9.5

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

[Pedro Alves]

On 02/25/2014 04:24 PM, Joel Brobecker wrote:
> +  if (object == TARGET_OBJECT_UNWIND_TABLE && readbuf != NULL)
> +    {
> +      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?
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

2291  * This system call copies the unwind data into the buffer pointed to by BUF and returns
2292  * the size of the unwind data.  If BUF_SIZE is smaller than the size of the unwind data
2293  * or if BUF is NULL, nothing is copied, but the system call still returns the size of the
2294  * unwind data.

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);

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.

> +	return TARGET_XFER_E_IO;
> +      else if (xfered <= offset)

Not sure I follow this one.

> +	return TARGET_XFER_EOF;
> +      else
> +	{
> +	  memcpy (readbuf, tmp_buf + offset, xfered - offset);
> +	  *xfered_len = xfered - offset;
> +	  return TARGET_XFER_OK;
> +	}
> +    }

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

---
 gdb/ia64-linux-nat.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
index c057b55..65a57d2 100644
--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -850,20 +850,30 @@ 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;
+      static long gate_table_size;
+      gdb_byte *tmp_buf;
+      long res;
 
-      xfered = syscall (__NR_getunwind, readbuf, offset + len);
-      if (xfered <= 0)
-	return TARGET_XFER_E_IO;
-      else if (xfered <= offset)
+      /* 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_EIO;
+
+      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);
+      if (syscall (__NR_getunwind, tmp_buf, gate_table_size) < 0)
+	return TARGET_XFER_EIO;
+      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.11.7

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 3382 bytes --]

> > +      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

[-- Attachment #2: 0001-Re-implement-ia64-linux-nat.c-ia64_linux_xfer_partia.patch --]
[-- Type: text/x-diff, Size: 2741 bytes --]

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

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

[Pedro Alves]

On 02/26/2014 02:14 AM, Joel Brobecker wrote:

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

I believe so.  Notice that create_gate_table is an __init function
(means it runs only once, at boot time).  I now grepped the
kernel tree for gate_table_size, and saw nowhere else changing it.

-- 
Pedro Alves

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

[Joel Brobecker]

> > BTW: Now that I have pushed the patch, I just realized something.
> > Are you sure the gate_table_size is a constant? >:-o
> 
> I believe so.  Notice that create_gate_table is an __init function
> (means it runs only once, at boot time).  I now grepped the
> kernel tree for gate_table_size, and saw nowhere else changing it.

Thanks! It wasn't completely obviouus to me - but then, it was
late in the day right during a time when jetlag is the worst :).
And thanks again for the patch.

-- 
Joel

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

[Pedro Alves]

BTW, I forgot to say, but -- thanks for writing the change
and commit logs!  :-)

-- 
Pedro Alves

On 02/26/2014 02:14 AM, Joel Brobecker wrote:
> 
> 0001-Re-implement-ia64-linux-nat.c-ia64_linux_xfer_partia.patch
> 
> 
> 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.