[v2] gdb/hppa-tdep.c: Fix logical working flow typo issue

  inst_saves_gr() wants to recognize 'st??' instruction, according to the
code we know it include 'stb' (for store byte), 'stw(m)' (for store
word), and 'std' (for store double word).

They should be in the same format, and have neighbour numbers:
especially, 'stw(m)' need be in the middle of 'stb' and 'std'.

 - for ((inst >> 26) != 0x3):

   stb: 0x18, or 0x19,
   stw: 0x1a, stwm: 0x1b,
   std: 0x1c.

 - else ((inst >> 26) == 0x3), need check:

   stb: 0x08, or 0x09,
   stw: 0x0a,
   std: 0x0b.

For clearer reason, not combine the logical comparation code together.

2014-10-22  Chen Gang  <gang.chen.5i5j@gmail.com>

        * hppa-tdep.c (inst_saves_gr): Fix logical working flow typo
        issue.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 gdb/hppa-tdep.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)
  

On 10/22/2014 11:38 PM, Chen Gang wrote:
> inst_saves_gr() wants to recognize 'st??' instruction, according to the
> code we know it include 'stb' (for store byte), 'stw(m)' (for store
> word), and 'std' (for store double word).
> 
> They should be in the same format, and have neighbour numbers:
> especially, 'stw(m)' need be in the middle of 'stb' and 'std'.
> 
>  - for ((inst >> 26) != 0x3):
> 
>    stb: 0x18, or 0x19,
>    stw: 0x1a, stwm: 0x1b,
>    std: 0x1c.
> 
>  - else ((inst >> 26) == 0x3), need check:
> 
>    stb: 0x08, or 0x09,
>    stw: 0x0a,
>    std: 0x0b.
> 
> For clearer reason, not combine the logical comparation code together.
> 
> 2014-10-22  Chen Gang  <gang.chen.5i5j@gmail.com>
> 
>         * hppa-tdep.c (inst_saves_gr): Fix logical working flow typo
>         issue.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  gdb/hppa-tdep.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
> index 627f31a..4363ab4 100644
> --- a/gdb/hppa-tdep.c
> +++ b/gdb/hppa-tdep.c
> @@ -1376,16 +1376,35 @@ is_branch (unsigned long inst)
>  }
>  
>  /* Return the register number for a GR which is saved by INST or
> -   zero it INST does not save a GR.  */
> +   zero it INST does not save a GR.
> +
> +   inst_saves_gr() wants to recognize 'st??' instruction, it include 'stb' (for
> +   store byte), 'stw(m)' (for store word), and 'std' (for store double word).
> +
> +   They should be in the same format, and have neighbour numbers: especially,
> +   'stw(m)' need be in the middle of 'stb' and 'std'.
> +
> +    - for ((inst >> 26) != 0x3):
> +
> +      stb: 0x18, or 0x19,
> +      stw: 0x1a, stwm: 0x1b,
> +      std: 0x1c.
> +
> +    - else ((inst >> 26) == 0x3), need check :

Oh sorry, need remove the whithe space: use "need check:" instead of
"need check :".

> +
> +      stb: 0x08, or 0x09,
> +      stw: 0x0a,
> +      std: 0x0b.
> +*/
>  
>  static int
>  inst_saves_gr (unsigned long inst)
>  {
> -  /* Does it look like a stw?  */
> +  /* Does it look like a stw(m)?  */
>    if ((inst >> 26) == 0x1a || (inst >> 26) == 0x1b
>        || (inst >> 26) == 0x1f
> -      || ((inst >> 26) == 0x1f
> -	  && ((inst >> 6) == 0xa)))
> +      || ((inst >> 26) == 0x3
> +	  && ((inst >> 6) & 0xf) == 0xa))
>      return hppa_extract_5R_store (inst);
>  
>    /* Does it look like a std?  */
> @@ -1394,16 +1413,12 @@ inst_saves_gr (unsigned long inst)
>  	  && ((inst >> 6) & 0xf) == 0xb))
>      return hppa_extract_5R_store (inst);
>  
> -  /* Does it look like a stwm?  GCC & HPC may use this in prologues.  */
> -  if ((inst >> 26) == 0x1b)
> -    return hppa_extract_5R_store (inst);
> -
>    /* Does it look like sth or stb?  HPC versions 9.0 and later use these
>       too.  */
>    if ((inst >> 26) == 0x19 || (inst >> 26) == 0x18
>        || ((inst >> 26) == 0x3
>  	  && (((inst >> 6) & 0xf) == 0x8
> -	      || (inst >> 6) & 0xf) == 0x9))
> +	      || ((inst >> 6) & 0xf) == 0x9)))
>      return hppa_extract_5R_store (inst);
>  
>    return 0;
>
  

Hello all:

Is this patch OK, it is part of checking saving registers in the stack,
it tries to recognize the saving instructions "stb, stw(m), or std", so
can continue checking.

Excuse me, I have no related environments for a test, I assumed that the
original author knew about the working flow, then wrote the related
function.

If need additional analyzing, please let me know (better to let me know
what I need try, next).


Thanks.

On 10/22/14 23:38, Chen Gang wrote:
> inst_saves_gr() wants to recognize 'st??' instruction, according to the
> code we know it include 'stb' (for store byte), 'stw(m)' (for store
> word), and 'std' (for store double word).
> 
> They should be in the same format, and have neighbour numbers:
> especially, 'stw(m)' need be in the middle of 'stb' and 'std'.
> 
>  - for ((inst >> 26) != 0x3):
> 
>    stb: 0x18, or 0x19,
>    stw: 0x1a, stwm: 0x1b,
>    std: 0x1c.
> 
>  - else ((inst >> 26) == 0x3), need check:
> 
>    stb: 0x08, or 0x09,
>    stw: 0x0a,
>    std: 0x0b.
> 
> For clearer reason, not combine the logical comparation code together.
> 
> 2014-10-22  Chen Gang  <gang.chen.5i5j@gmail.com>
> 
>         * hppa-tdep.c (inst_saves_gr): Fix logical working flow typo
>         issue.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  gdb/hppa-tdep.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
> index 627f31a..4363ab4 100644
> --- a/gdb/hppa-tdep.c
> +++ b/gdb/hppa-tdep.c
> @@ -1376,16 +1376,35 @@ is_branch (unsigned long inst)
>  }
>  
>  /* Return the register number for a GR which is saved by INST or
> -   zero it INST does not save a GR.  */
> +   zero it INST does not save a GR.
> +
> +   inst_saves_gr() wants to recognize 'st??' instruction, it include 'stb' (for
> +   store byte), 'stw(m)' (for store word), and 'std' (for store double word).
> +
> +   They should be in the same format, and have neighbour numbers: especially,
> +   'stw(m)' need be in the middle of 'stb' and 'std'.
> +
> +    - for ((inst >> 26) != 0x3):
> +
> +      stb: 0x18, or 0x19,
> +      stw: 0x1a, stwm: 0x1b,
> +      std: 0x1c.
> +
> +    - else ((inst >> 26) == 0x3), need check :
> +
> +      stb: 0x08, or 0x09,
> +      stw: 0x0a,
> +      std: 0x0b.
> +*/
>  
>  static int
>  inst_saves_gr (unsigned long inst)
>  {
> -  /* Does it look like a stw?  */
> +  /* Does it look like a stw(m)?  */
>    if ((inst >> 26) == 0x1a || (inst >> 26) == 0x1b
>        || (inst >> 26) == 0x1f
> -      || ((inst >> 26) == 0x1f
> -	  && ((inst >> 6) == 0xa)))
> +      || ((inst >> 26) == 0x3
> +	  && ((inst >> 6) & 0xf) == 0xa))
>      return hppa_extract_5R_store (inst);
>  
>    /* Does it look like a std?  */
> @@ -1394,16 +1413,12 @@ inst_saves_gr (unsigned long inst)
>  	  && ((inst >> 6) & 0xf) == 0xb))
>      return hppa_extract_5R_store (inst);
>  
> -  /* Does it look like a stwm?  GCC & HPC may use this in prologues.  */
> -  if ((inst >> 26) == 0x1b)
> -    return hppa_extract_5R_store (inst);
> -
>    /* Does it look like sth or stb?  HPC versions 9.0 and later use these
>       too.  */
>    if ((inst >> 26) == 0x19 || (inst >> 26) == 0x18
>        || ((inst >> 26) == 0x3
>  	  && (((inst >> 6) & 0xf) == 0x8
> -	      || (inst >> 6) & 0xf) == 0x9))
> +	      || ((inst >> 6) & 0xf) == 0x9)))
>      return hppa_extract_5R_store (inst);
>  
>    return 0;
>
  

Chen,

> Is this patch OK, it is part of checking saving registers in the stack,
> it tries to recognize the saving instructions "stb, stw(m), or std", so
> can continue checking.
> 
> Excuse me, I have no related environments for a test, I assumed that the
> original author knew about the working flow, then wrote the related
> function.
> 
> If need additional analyzing, please let me know (better to let me know
> what I need try, next).

Thank you for the patch, but lack of access to an environment where
you can test is an issue.  I would be an entirely different context
if the fix was obvious, but it isn't unless perhaps you start
opening HP-PA assembly manuals; and I don't honestly see much interest
in HP-PA anymore.

And if you do not have access to an environment for a test, how did
you come about this issue? Does the lack of evironment mean lack of
the software (tcl, expect, dejagnu), or lack of hardware?
  

On 11/23/2014 02:38 PM, Joel Brobecker wrote:
> Chen,
> 
>> Is this patch OK, it is part of checking saving registers in the stack,
>> it tries to recognize the saving instructions "stb, stw(m), or std", so
>> can continue checking.
>>
>> Excuse me, I have no related environments for a test, I assumed that the
>> original author knew about the working flow, then wrote the related
>> function.
>>
>> If need additional analyzing, please let me know (better to let me know
>> what I need try, next).
> 
> Thank you for the patch, but lack of access to an environment where
> you can test is an issue.  I would be an entirely different context
> if the fix was obvious, but it isn't unless perhaps you start
> opening HP-PA assembly manuals; and I don't honestly see much interest
> in HP-PA anymore.
> 

I shall try to find the related assembly reference (although it is not
quite easy for me, and may be fail to get it), or get parisc related
members confirmation.

Hope I can get related proof (related documents, or related members
confirmation) within next month (2014-12-31), although I am not quite
sure whether I can finish or not.

> And if you do not have access to an environment for a test, how did
> you come about this issue? Does the lack of evironment mean lack of
> the software (tcl, expect, dejagnu), or lack of hardware?
> 

It is lack of hardware and related software. And I found it firstly by
gcc5 compiler warning, then read through the code (together with some
gdb members), found it should be an issue.


Thanks.
  

After consult parisc kernel members, I found the related document for
it, I guess, it can be as our proof for this patch:

  https://parisc.wiki.kernel.org/images-parisc/7/73/Parisc2.0.pdf

For me, my original comments need be improved: need mention about STH,
and maybe also need consider about additional related instructions of
parisc 2.0 (STDA, STWA, STBY, STDBY) in our source code.

Welcome any suggestions, ideas, and completions.

Thanks.

On 11/23/14 21:40, Chen Gang wrote:
> On 11/23/2014 02:38 PM, Joel Brobecker wrote:
>> Chen,
>>
>>> Is this patch OK, it is part of checking saving registers in the stack,
>>> it tries to recognize the saving instructions "stb, stw(m), or std", so
>>> can continue checking.
>>>
>>> Excuse me, I have no related environments for a test, I assumed that the
>>> original author knew about the working flow, then wrote the related
>>> function.
>>>
>>> If need additional analyzing, please let me know (better to let me know
>>> what I need try, next).
>>
>> Thank you for the patch, but lack of access to an environment where
>> you can test is an issue.  I would be an entirely different context
>> if the fix was obvious, but it isn't unless perhaps you start
>> opening HP-PA assembly manuals; and I don't honestly see much interest
>> in HP-PA anymore.
>>
> 
> I shall try to find the related assembly reference (although it is not
> quite easy for me, and may be fail to get it), or get parisc related
> members confirmation.
> 
> Hope I can get related proof (related documents, or related members
> confirmation) within next month (2014-12-31), although I am not quite
> sure whether I can finish or not.
> 
>> And if you do not have access to an environment for a test, how did
>> you come about this issue? Does the lack of evironment mean lack of
>> the software (tcl, expect, dejagnu), or lack of hardware?
>>
> 
> It is lack of hardware and related software. And I found it firstly by
> gcc5 compiler warning, then read through the code (together with some
> gdb members), found it should be an issue.
> 
> 
> Thanks.
>
  

> After consult parisc kernel members, I found the related document for
> it, I guess, it can be as our proof for this patch:
> 
>   https://parisc.wiki.kernel.org/images-parisc/7/73/Parisc2.0.pdf
> 
> For me, my original comments need be improved: need mention about STH,
> and maybe also need consider about additional related instructions of
> parisc 2.0 (STDA, STWA, STBY, STDBY) in our source code.
> 
> Welcome any suggestions, ideas, and completions.

OK - please send us an updated patch, and we will review.

Thank you,
  

On 12/8/14 11:14, Joel Brobecker wrote:
>> After consult parisc kernel members, I found the related document for
>> it, I guess, it can be as our proof for this patch:
>>
>>   https://parisc.wiki.kernel.org/images-parisc/7/73/Parisc2.0.pdf
>>
>> For me, my original comments need be improved: need mention about STH,
>> and maybe also need consider about additional related instructions of
>> parisc 2.0 (STDA, STWA, STBY, STDBY) in our source code.
>>
>> Welcome any suggestions, ideas, and completions.
> 
> OK - please send us an updated patch, and we will review.
> 

OK, thanks, I shall try to send patch v3 within this week (2014-12-14).
If the time point is too late, please let me know, and I shall try to
send it as soon as possible.

Thanks.
  

> OK, thanks, I shall try to send patch v3 within this week (2014-12-14).
> If the time point is too late, please let me know, and I shall try to
> send it as soon as possible.

_I_ am not in a hurry! :-)