Re: [Patch] Fix multithread debugging problems on s390

[Andrew Cagney <ac131313@ges.redhat.com>]

> Hi,
> attached is a patch that fixes the multi-thread debugging problems on s390.
> The functions fill_gregset and fill_fpregset were broken, but seem to be
> used only since the gdb thread code has been revised in 5.1. I have
> attached the fix also as file in case there are any line mangling in the
> code below.

> 2002-07-12  Gerhard Tonn  <ton@de.ibm.com>
> 
>             * s390-nat.c (fill_gregset and fill_fpregset):
>               Call regcache_collect functions.
> 
> 
> 
> I would appreciate, if someone could commit it to trunk and 5.2 branch, if
> possible even for 5.2.1

I'll sit on 5.2.1 for slightly longer.  This one is a regression :-(

> Thanks
> 
> --- src/gdb/s390-nat.c  Tue Nov  6 22:18:13 2001
> +++ /home/ton/gdb/src/gdb/s390-nat.c      Wed Jul 10 10:15:26 2002
> @@ -273,14 +273,23 @@
>  void
>  fill_gregset (gregset_t * gregsetp, int regno)
>  {
> +  int regi;
>    greg_t *gregp = (greg_t *) gregsetp;
> 
>    if (regno >= S390_FIRST_CR && regno <= S390_LAST_CR)
> -    supply_register (regno, NULL);
> +    ;

Can you please add a comment indicating that the NO-OP is intentional 
and why.

>    else if (regno != -1)
> -    supply_register (regno, (char *) &gregp[regno]);
> -  else
> -    supply_gregset (gregsetp);
> +    regcache_collect (regno, (char *) &gregp[regno]);
> +  else {

GNU coding standards:

   else
     {
       regcache_collect (...,

> +    regcache_collect (S390_PSWM_REGNUM, (char *)

I don't believe that the cast is needed.  regcache_collect()'s second 
parameter is void.


> &gregp[S390_PSWM_REGNUM]);
> +    regcache_collect (S390_PC_REGNUM, (char *) &gregp[S390_PC_REGNUM]);
> +    for (regi = 0; regi < S390_NUM_GPRS; regi++)
> +      regcache_collect (S390_GP0_REGNUM + regi,
> +           (char *) &gregp[S390_GP0_REGNUM + regi]);

Indentation:

regcache_collect (S390_....,
                   &gregp[...]);

> +  if (regno == -1) {

GNU Coding style.  Also recommend ``< 0'' as safer:

   if (regno < 0)
     {

> +    regcache_collect (S390_FPC_REGNUM, (char *) &fpregsetp->fpc);
> +    for (regi = 0; regi < S390_NUM_FPRS; regi++)
> +      regcache_collect (S390_FP0_REGNUM + regi, (char *) &fpregsetp->fprs[regi]);
> +  }
>    else
> -    supply_register (regno,
> +    regcache_collect (regno,
>                  &((char *) fpregsetp)[REGISTER_BYTE (regno) -
>                                  REGISTER_BYTE (S390_FPC_REGNUM)]);

Can this be written in a way that doesn't use REGISTER_BYTE()? 
REGISTER_BYTE() is going to be removed.

>  }
> 
> (See attached file: gdb-5.2.1-s390.diff)

Andrew