[RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[Joel Brobecker]

On ia64-linux, GDB sometimes prints the following error when trying
to switch to a different task:

    (gdb) task 3
    Register 0 is not available

This is a random failure that sometimes happens, sometimes does not.
We should be able to see with any program that uses thread, probably
when the debugger stops the program at a certain point (which conditions
trigger the problem have not been investigated).

The error comes from the fact that the libunwind library is requesting
the value of register 0 (zero): This eventually leads us to
ia64-linux-nat.c:ia64_linux_fetch_register.

This function relies on ia64_cannot_fetch_register to determine
whether or not we have access to the register's value.  The ptrace
interface does not provide the r0 value, and so we end up telling
the regcache that this register's value is not available.  And yet,
for r0, we do not need to ask ptrace for its value, since it is
always zero.

So, the fix was to add a special rule for supplying a nul value
when regnum == 0.

gdb/ChangeLog:

        * ia64-linux-nat.c (ia64_linux_fetch_register): Add special
        handling for r0.

Tested on ia64-linux. I also tested it against the program I used
to reproduce the problem. It used to consistently reproduce once
every 1-10 times. Now, it doesn't anymore even after trying 50 times.

OK?

---
 gdb/ia64-linux-nat.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
index 65e077b..f43e91e 100644
--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -681,6 +681,17 @@ ia64_linux_fetch_register (struct regcache *regcache, int regnum)
   PTRACE_TYPE_RET *buf;
   int pid, i;
 
+  /* r0 cannot be fetched but is always zero.  */
+  if (regnum == 0)
+    {
+      const char r0_value[8] = {0x00, 0x00, 0x00, 0x00,
+				0x00, 0x00, 0x00, 0x00};
+
+      gdb_assert (sizeof (r0_value) == register_size (gdbarch, regnum));
+      regcache_raw_supply (regcache, regnum, r0_value);
+      return;
+    }
+
   if (ia64_cannot_fetch_register (gdbarch, regnum))
     {
       regcache_raw_supply (regcache, regnum, NULL);
-- 
1.7.1

Re: [RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[Pedro Alves]

On Tuesday 25 October 2011 17:52:43, Joel Brobecker wrote:

>  gdb/ia64-linux-nat.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
> index 65e077b..f43e91e 100644
> --- a/gdb/ia64-linux-nat.c
> +++ b/gdb/ia64-linux-nat.c
> @@ -681,6 +681,17 @@ ia64_linux_fetch_register (struct regcache *regcache, int regnum)
>    PTRACE_TYPE_RET *buf;
>    int pid, i;
>  
> +  /* r0 cannot be fetched but is always zero.  */
> +  if (regnum == 0)

IA64_GR0_REGNUM?

> +    {
> +      const char r0_value[8] = {0x00, 0x00, 0x00, 0x00,
> +				0x00, 0x00, 0x00, 0x00};

FYI, in C, this is equivalent to:

        const char r0_value[8] = { 0 };

I'd write it as:

        const char zero[8] = { 0 };

> +
> +      gdb_assert (sizeof (r0_value) == register_size (gdbarch, regnum));
> +      regcache_raw_supply (regcache, regnum, r0_value);
> +      return;
> +    }
> +

I don't speak ia64, but this is the right direction.
I think we should make ia64_cannot_fetch_register return true for
gr0 too though.  Your new code in ia64_linux_fetch_register would
then be moved a bit further down after the ia64_cannot_fetch_register
check.  It may be there's no code that trip on the below issue,
but it's best to avoid leaving the issue latent.

-- 
Pedro Alves

Re: [RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[Pedro Alves]

On Tuesday 25 October 2011 18:18:42, Pedro Alves wrote:
> > +    {
> > +      const char r0_value[8] = {0x00, 0x00, 0x00, 0x00,
> > +                             0x00, 0x00, 0x00, 0x00};
> 
> FYI, in C, this is equivalent to:
> 
>         const char r0_value[8] = { 0 };
> 
> I'd write it as:
> 
>         const char zero[8] = { 0 };
> 

Actually, const s/char/gdb_byte/ too.  :-)

-- 
Pedro Alves

Re: [RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[Joel Brobecker]

Hi again Pedro,

I will make the small changes you suggested at the beginning of
the patch. I just wanted to confirm something:

> > +      gdb_assert (sizeof (r0_value) == register_size (gdbarch, regnum));
> > +      regcache_raw_supply (regcache, regnum, r0_value);
> > +      return;
> > +    }
> > +
> 
> I don't speak ia64, but this is the right direction.
> I think we should make ia64_cannot_fetch_register return true for
> gr0 too though.

I assume that you mean that ia64_cannot_fetch_register should return
False as well (meaning that we can in fact fetch the register - don't
you love double negatives?). I had originally interpreted the name
of that function very strictly, meaning that, no, the kernel does
not provide that register value, so we cannot fetch it.  But at the
same time, I think I see you point of having the fetch_register
routine saying we can get the value, while at the same time the
cannot_fetch_register function says we can't.

I will send a new patch...

-- 
Joel

Re: [RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[Pedro Alves]

On Tuesday 25 October 2011 21:40:52, Joel Brobecker wrote:
> Hi again Pedro,
> 
> I will make the small changes you suggested at the beginning of
> the patch. I just wanted to confirm something:
> 
> > > +      gdb_assert (sizeof (r0_value) == register_size (gdbarch, regnum));
> > > +      regcache_raw_supply (regcache, regnum, r0_value);
> > > +      return;
> > > +    }
> > > +
> > 
> > I don't speak ia64, but this is the right direction.
> > I think we should make ia64_cannot_fetch_register return true for
> > gr0 too though.
> 
> I assume that you mean that ia64_cannot_fetch_register should return
> False as well (meaning that we can in fact fetch the register - don't
> you love double negatives?). 

Bleh. :-)  Yes, I meant return false.

> I had originally interpreted the name
> of that function very strictly, meaning that, no, the kernel does
> not provide that register value, so we cannot fetch it.  But at the
> same time, I think I see you point of having the fetch_register
> routine saying we can get the value, while at the same time the
> cannot_fetch_register function says we can't.

Hmm, yeah, I looking at it from the perspective that the gdbarch
hook can be used outside the ia64 target/arch dependent code, and at
that point, it shouldn't matter if we're talking to ptrace or to
a core or to something else.  The core code calling the hook would
only be interested in knowing whether the value can be fetched or
not.  But that looks moot, as this is used to mean "ptrace can fetch
the register".

 alpha-nat.c:      if (gdbarch_cannot_fetch_register (gdbarch, regno))
 hppa-linux-nat.c:  if (gdbarch_cannot_fetch_register (gdbarch, regno))
 inf-ptrace.c:      || gdbarch_cannot_fetch_register (gdbarch, regnum))

These are all legacy, and all look broken by design to me.  I mean, what
ptrace can or not return is a target-y thing, not gdbarch thing, and
certainly the corresponding struct target can override the register
fetching hook to do the same thing.

I thought there were more uses of the hook...  where have they gone
to? :-D

Given that, I don't see reason to change the hook on ia64-linux.  In
fact, I don't think there's any need for the hook to be installed
on ia64-linux.  Oh, wait, it _isn't_ installed as a gdbarch hook,
is it?  Eh.  I'm tempted to go
s/gdbarch_cannot_fetch_register/deprecated_gdbarch_cannot_fetch_register/g. :-)

-- 
Pedro Alves

Re: [RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[Pedro Alves]

Hi Joel,

On 10/25/2011 09:40 PM, Joel Brobecker wrote:

> Hi again Pedro,
> 
> I will make the small changes you suggested at the beginning of
> the patch. I just wanted to confirm something:


<snip my own confusion>

> I will send a new patch...


Do you have that new patch handy?  Want to check it in?
I'm testing something on ia64-linux, and notice that gr0 shows up
as *unavailable*.

-- 
Pedro Alves

Re: [RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[Joel Brobecker]

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

> > I will make the small changes you suggested at the beginning of
> > the patch. I just wanted to confirm something:
>
> <snip my own confusion>
>
> > I will send a new patch...
>
> Do you have that new patch handy?  Want to check it in?
> I'm testing something on ia64-linux, and notice that gr0 shows up
> as *unavailable*.

Argh - I could have sworn I sent the new patch, but I cannot find it
anyway in my sent box, or my repository on my laptop. And bad luck
on the timing, as we've done an emergency shutdown of most of our
machines due to some construction going on in our building (TMI?).

Anyway, I didn't want you to wait, so I redid the changes. I just
cannot compile or test them. I will do that tomorrow, hoping that
the machines will be powered up again, and if that shows no
regression, I will check it in.

Given that the cannot_fetch_register gdbarch hook wasn't attached,
my understanding is that there isn't anything else that really needs
changing...

Sorry about the delay...
-- 
Joel

[-- Attachment #2: 0001-ia64-linux-Allow-libunwind-to-fetch-register-0.patch --]
[-- Type: text/x-diff, Size: 1921 bytes --]

From 1f2a3fee025f763f3e7c6f6147069b8a6008018e Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 25 Oct 2011 12:17:08 -0400
Subject: [PATCH] [ia64-linux] Allow libunwind to fetch register 0

On ia64-linux, GDB sometimes prints the following error when trying
to switch to a different task:

    (gdb) task 3
    Register 0 is not available

This is a random failure that sometimes happens, sometimes does not.
The error comes from the fact that the libunwind library is requesting
the value of register 0 (zero): This eventually leads us to
ia64-linux-nat.c:ia64_linux_fetch_register.

This function relies on ia64_cannot_fetch_register to determine
whether or not we have access to the register's value.  The ptrace
interface does not provide the r0 value, and so we end up telling
the regcache that this register's value is not available.  And yet,
for r0, we do not need to ask ptrace for its value, since it is
always zero.

So, the fix was to add a special rule for supplying a nul value
when regnum == 0.

gdb/ChangeLog:

        * ia64-linux-nat.c (ia64_linux_fetch_register): Add special
        handling for r0.
---
 gdb/ia64-linux-nat.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
index 19b827f..60b873b 100644
--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -680,6 +680,16 @@ ia64_linux_fetch_register (struct regcache *regcache, int regnum)
   PTRACE_TYPE_RET *buf;
   int pid, i;
 
+  /* r0 cannot be fetched but is always zero.  */
+  if (regnum == IA64_GR0_REGNUM)
+    {
+      const gdb_byte zero[8] = { 0 };
+
+      gdb_assert (sizeof (zero) == register_size (gdbarch, regnum));
+      regcache_raw_supply (regcache, regnum, zero);
+      return;
+    }
+
   if (ia64_cannot_fetch_register (gdbarch, regnum))
     {
       regcache_raw_supply (regcache, regnum, NULL);
-- 
1.7.1

Re: [RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[Pedro Alves]

On 03/28/2012 12:03 AM, Joel Brobecker wrote:

>>> I will make the small changes you suggested at the beginning of
>>> > > the patch. I just wanted to confirm something:
>> >
>> > <snip my own confusion>
>> >
>>> > > I will send a new patch...
>> >
>> > Do you have that new patch handy?  Want to check it in?
>> > I'm testing something on ia64-linux, and notice that gr0 shows up
>> > as *unavailable*.
> Argh - I could have sworn I sent the new patch, but I cannot find it
> anyway in my sent box, or my repository on my laptop. 


> And bad luck on the timing, as we've done an emergency shutdown of most of our

> machines due to some construction going on in our building (TMI?).


( :-) )

> 
> Anyway, I didn't want you to wait, so I redid the changes. I just
> cannot compile or test them. I will do that tomorrow, hoping that
> the machines will be powered up again, and if that shows no
> regression, I will check it in.


I've tested them on ia64-unknown-linux-gnu (debian 6).  No regressions,
and a few progressions.

> 
> Given that the cannot_fetch_register gdbarch hook wasn't attached,
> my understanding is that there isn't anything else that really needs
> changing...


Yeah.  Please check it in.

I see that $f0 (always 0.0) and $f1 (always 1.0) could/should be given
similar treatment:

(gdb) info register $f0 $f1
f0             *value not available*
f1             *value not available*

$p0 also has one bit that should always be read as 1, but it
seems something (the kernel?) is already handling that:

(gdb) info register $p0
p0             0x1      1

gcore.exp failures related to these issues.  It shows the $f0/$f1 issue, and
also, that $ec is read back from the core as 0, but it is read as
*unavailable* when debugging a live process.  The latter is because
have (ia64-linux-nat.c):

static int u_offsets[] =
  {
...
    PT_AR_LC,
    -1,		/* Not available: EC, the Epilog Count register.  */

But in ia64-linux-nat.c:supply_gregset:

  regcache_raw_supply (regcache, IA64_LC_REGNUM, regp + 53);
  regcache_raw_supply (regcache, IA64_EC_REGNUM, regp + 54);

which is suspicious (the registers is not retrievable with ptrace, but
it's in the core?).  Indeed, on this system's /usr/include/asm/ptrace_offsets.h
I see:

#define PT_AR_EC                0x0800
#define PT_AR_LC                0x0808

So it does look like it is available with ptrace.

I'm testing patches for these issues.

GDBserver also needs the same treatment for these things.  (e.g.,
you can see gdb.server/ tests failing on ia64-linux).

-- 
Pedro Alves

[PATCH] IA64: EC, the Epilog Count register, is available in ptrace.

[Pedro Alves]

> gcore.exp failures related to these issues.  It shows the $f0/$f1 issue, and
> also, that $ec is read back from the core as 0, but it is read as
> *unavailable* when debugging a live process.  The latter is because
> have (ia64-linux-nat.c):
>
> static int u_offsets[] =
>   {
> ...
>     PT_AR_LC,
>     -1,		/* Not available: EC, the Epilog Count register.  */
>
> But in ia64-linux-nat.c:supply_gregset:
>
>   regcache_raw_supply (regcache, IA64_LC_REGNUM, regp + 53);
>   regcache_raw_supply (regcache, IA64_EC_REGNUM, regp + 54);
>
> which is suspicious (the registers is not retrievable with ptrace, but
> it's in the core?).  Indeed, on this system's /usr/include/asm/ptrace_offsets.h
> I see:
>
> #define PT_AR_EC                0x0800
> #define PT_AR_LC                0x0808
>
> So it does look like it is available with ptrace.

Git blame on the kernel's sources shows the define always existed for
at least as long as there's git history (1da177e, 2.6.12-rc2).  I
haven't checked further back...

WDYT?

2012-03-28  Pedro Alves  <palves@redhat.com>

	* ia64-linux-nat.c (u_offsets): Map IA64_EC_REGNUM to PT_AR_EC.
---
 gdb/ia64-linux-nat.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
index 60b873b..24bde2d 100644
--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -272,7 +272,7 @@ static int u_offsets[] =
     -1, -1, -1, -1, -1, -1, -1, -1, -1,
     PT_AR_PFS,
     PT_AR_LC,
-    -1,		/* Not available: EC, the Epilog Count register.  */
+    PT_AR_EC,
     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,

Re: [PATCH] IA64: EC, the Epilog Count register, is available in ptrace.

[Joel Brobecker]

> Git blame on the kernel's sources shows the define always existed for
> at least as long as there's git history (1da177e, 2.6.12-rc2).  I
> haven't checked further back...
> 
> WDYT?
> 
> 2012-03-28  Pedro Alves  <palves@redhat.com>
> 
> 	* ia64-linux-nat.c (u_offsets): Map IA64_EC_REGNUM to PT_AR_EC.

Looks reasonable. I kind of doubt it will happen, but if anyone actually
comes across a system where it does not exist, we can add some code that
defines PT_AR_EC to -1 which would restore the previous behavior...

-- 
Joel

Re: [PATCH] IA64: EC, the Epilog Count register, is available in ptrace.

[Pedro Alves]

On 03/28/2012 06:12 PM, Joel Brobecker wrote:

>> Git blame on the kernel's sources shows the define always existed for
>> at least as long as there's git history (1da177e, 2.6.12-rc2).  I
>> haven't checked further back...
>>
>> WDYT?
>>
>> 2012-03-28  Pedro Alves  <palves@redhat.com>
>>
>> 	* ia64-linux-nat.c (u_offsets): Map IA64_EC_REGNUM to PT_AR_EC.
> 
> Looks reasonable. I kind of doubt it will happen, but if anyone actually
> comes across a system where it does not exist, we can add some code that
> defines PT_AR_EC to -1 which would restore the previous behavior...


Yeah.  I've applied this.  Thanks.

-- 
Pedro Alves

[PATCH] IA64: $fr0==0.0, $fr1==1.0

[Pedro Alves]

> I see that $f0 (always 0.0) and $f1 (always 1.0) could/should be given
> similar treatment:
>
> (gdb) info register $f0 $f1
> f0             *value not available*
> f1             *value not available*
>

(...)

> gcore.exp failures related to these issues.

(because when debugging a live process "info registers" shows 'value
not available', but when debugging a core, "info registers" shows 0.0
for those registers.

(I was lazy and got the raw bytes of 1.0 by using "info float" after
writing 1 to $fp2...)

With yours, and these patches applied, gcore.exp passes cleanly
on ia64-unknown-linux-gnu.  Before, we had:

Running ../../../src/gdb/testsuite/gdb.base/gcore.exp ...
FAIL: gdb.base/gcore.exp: corefile restored general registers
FAIL: gdb.base/gcore.exp: corefile restored all registers

No regressions.  WDYT?

2012-03-28  Pedro Alves  <palves@redhat.com>

	* ia64-linux-nat.c (supply_fpregset, ia64_linux_fetch_register):
	Always supply $fr0 as 0.0 and $fr1 as 1.0.
---
 gdb/ia64-linux-nat.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
index 24bde2d..237f2c7 100644
--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -447,8 +447,20 @@ supply_fpregset (struct regcache *regcache, const fpregset_t *fpregsetp)
 {
   int regi;
   const char *from;
+  const gdb_byte f_zero[16] = { 0 };
+  const gdb_byte f_one[16] =
+    { 0, 0, 0, 0, 0, 0, 0, 0x80, 0xff, 0xff, 0, 0, 0, 0, 0, 0 };
 
-  for (regi = IA64_FR0_REGNUM; regi <= IA64_FR127_REGNUM; regi++)
+  /* Kernel generated cores have fr1==0 instead of 1.0.  Older GDBs
+     did the same.  So ignore whatever might be recorded in fpregset_t
+     for fr0/fr1 and always supply their expected values.  */
+
+  /* fr0 is always read as zero.  */
+  regcache_raw_supply (regcache, IA64_FR0_REGNUM, f_zero);
+  /* fr1 is always read as one (1.0).  */
+  regcache_raw_supply (regcache, IA64_FR1_REGNUM, f_one);
+
+  for (regi = IA64_FR2_REGNUM; regi <= IA64_FR127_REGNUM; regi++)
     {
       from = (const char *) &((*fpregsetp)[regi - IA64_FR0_REGNUM]);
       regcache_raw_supply (regcache, regi, from);
@@ -690,6 +702,27 @@ ia64_linux_fetch_register (struct regcache *regcache, int regnum)
       return;
     }
 
+  /* fr0 cannot be fetched but is always zero.  */
+  if (regnum == IA64_FR0_REGNUM)
+    {
+      const gdb_byte f_zero[16] = { 0 };
+
+      gdb_assert (sizeof (f_zero) == register_size (gdbarch, regnum));
+      regcache_raw_supply (regcache, regnum, f_zero);
+      return;
+    }
+
+  /* fr1 cannot be fetched but is always one (1.0).  */
+  if (regnum == IA64_FR1_REGNUM)
+    {
+      const gdb_byte f_one[16] =
+	{ 0, 0, 0, 0, 0, 0, 0, 0x80, 0xff, 0xff, 0, 0, 0, 0, 0, 0 };
+
+      gdb_assert (sizeof (f_one) == register_size (gdbarch, regnum));
+      regcache_raw_supply (regcache, regnum, f_one);
+      return;
+    }
+
   if (ia64_cannot_fetch_register (gdbarch, regnum))
     {
       regcache_raw_supply (regcache, regnum, NULL);

Re: [PATCH] IA64: $fr0==0.0, $fr1==1.0

[Joel Brobecker]

> No regressions.  WDYT?
> 
> 2012-03-28  Pedro Alves  <palves@redhat.com>
> 
> 	* ia64-linux-nat.c (supply_fpregset, ia64_linux_fetch_register):
> 	Always supply $fr0 as 0.0 and $fr1 as 1.0.

FWIW, it looks good to me.

-- 
Joel

Re: [PATCH] IA64: $fr0==0.0, $fr1==1.0

[Pedro Alves]

On 03/28/2012 06:14 PM, Joel Brobecker wrote:

>> No regressions.  WDYT?
>>
>> 2012-03-28  Pedro Alves  <palves@redhat.com>
>>
>> 	* ia64-linux-nat.c (supply_fpregset, ia64_linux_fetch_register):
>> 	Always supply $fr0 as 0.0 and $fr1 as 1.0.
> 
> FWIW, it looks good to me.


I've applied it then.  Thanks.  GDBserver patch testing...

-- 
Pedro Alves

Re: [RFA/commit/ia64-linux] Allow libunwind to fetch register 0

[Joel Brobecker]

> I've tested them on ia64-unknown-linux-gnu (debian 6).  No
> regressions, and a few progressions. [...] Please check it in.

Thanks, Pedro. Now checked in.

I'm looking at your patches now...

-- 
Joel