[RFA] gdb/gdbserver/linux-ppc-low.c: Wrong return value in ppc_cannot_store_register()

[RFA] gdb/gdbserver/linux-ppc-low.c: Wrong return value in ppc_cannot_store_register()

[Markus Deuling]

Hi,

ppc_cannot_store_register() from linux-ppc-low.c returns 2 instead of 1.
This seems to be broken. In linux-low.c:usr_store_inferior_registers() there is following comparison:

      if ((*the_low_target.cannot_store_register) (regno) == 1)
        return;

This won't work with ppc low target. The patch returns 1 instead of 2.

Is it ok to apply ?


ChangeLog:

        * linux-ppc-low.c (ppc_cannot_store_register): Return
        1 instead of 2.


====================================================

diff -urN src/gdb/gdbserver/linux-ppc-low.c dev/gdb/gdbserver/linux-ppc-low.c
--- src/gdb/gdbserver/linux-ppc-low.c   2007-01-10 15:48:11.000000000 +0100
+++ dev/gdb/gdbserver/linux-ppc-low.c   2007-01-18 13:27:50.000000000 +0100
@@ -53,7 +53,7 @@
 {
   /* Some kernels do not allow us to store fpscr.  */
   if (regno == find_regno ("fpscr"))
-    return 2;
+    return 1;

   return 0;
 }


Regards,
Markus

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com 

Re: [RFA] gdb/gdbserver/linux-ppc-low.c: Wrong return value in ppc_cannot_store_register()

[Daniel Jacobowitz]

On Thu, Jan 18, 2007 at 01:37:21PM +0100, Markus Deuling wrote:
> Hi,
> 
> ppc_cannot_store_register() from linux-ppc-low.c returns 2 instead of 1.
> This seems to be broken. In linux-low.c:usr_store_inferior_registers() 
> there is following comparison:
> 
>      if ((*the_low_target.cannot_store_register) (regno) == 1)
>        return;
> 
> This won't work with ppc low target. The patch returns 1 instead of 2.

That's the whole point - the return value of two exists to avoid that
check.

Why did you want this patch, i.e. does it fix a problem you
encountered?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] gdb/gdbserver/linux-ppc-low.c: Wrong return value in ppc_cannot_store_register()

[Markus Deuling]

Hi, 

Daniel Jacobowitz schrieb:
> On Thu, Jan 18, 2007 at 01:37:21PM +0100, Markus Deuling wrote:
>> Hi,
>>
>> ppc_cannot_store_register() from linux-ppc-low.c returns 2 instead of 1.
>> This seems to be broken. In linux-low.c:usr_store_inferior_registers() 
>> there is following comparison:
>>
>>      if ((*the_low_target.cannot_store_register) (regno) == 1)
>>        return;
>>
>> This won't work with ppc low target. The patch returns 1 instead of 2.
> 
> That's the whole point - the return value of two exists to avoid that
> check.
> 
> Why did you want this patch, i.e. does it fix a problem you
> encountered?
> 

it's not a fix. I didn't realize that there's a "return 2" to avoid that check.

As far as I've seen it, *the_low_target.cannot_store_register is called two
times in linux-low.c and only there. One time result is compared with 1, the other time with 0.

So no one works with that returning 2. If the check has to be avoided in ppc, 
then the three lines below can re removed, can't they? Then it's like in linux-ppc64-low always returning 0.
I think it's not useful to return 2.

static int
ppc_cannot_store_register (int regno)
{
-  /* Some kernels do not allow us to store fpscr.  */
-  if (regno == find_regno ("fpscr"))
-    return 2;

  return 0;
}

What do you think about that ? 


Regards,
Markus

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com 

Re: [RFA] gdb/gdbserver/linux-ppc-low.c: Wrong return value in ppc_cannot_store_register()

[Daniel Jacobowitz]

On Fri, Jan 19, 2007 at 12:27:12PM +0100, Markus Deuling wrote:
> So no one works with that returning 2. If the check has to be avoided in 
> ppc, then the three lines below can re removed, can't they? Then it's like 

No.  Please think about the comment you're deleting.  This exists so
that gdbserver will not print error messages on kernels which do not
allow us to modify fpscr; there are no ppc64 kernels with this problem,
but ppc32 kernels with the bug were prevalent.

-- 
Daniel Jacobowitz
CodeSourcery