[v4] gdb/hppa-tdep.c: Fix logical working flow issues and check additional store instructions

  Original working flow has several issues:

 - typo issue: "(inst >> 26) == 0x1f && ..." for checking 'stw(m)'.

 - compiler warning: "(inst >> 6) & 0xf) == 0x9" for checking 'sth'.

 - "(inst >> 6) == 0xa" needs to be "((inst >> 6) & 0xf) == 0xa".

And also need check additional store instructions:

 - For absolute memory: 'stby', 'stdby'.

 - For unaligned: 'stwa', 'stda'.

The original code also can be improved:

 - Remove redundant double check "(inst >> 26) == 0x1b" for 'stwm'.

 - Use 2 'switch' statements instead of all 'if' statements.

2014-12-14  Chen Gang  <gang.chen.5i5j@gmail.com>

	* hppa-tdep.c (inst_saves_gr): Fix logical working flow issues
	and check additional store instructions.
---
 gdb/hppa-tdep.c | 111 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 85 insertions(+), 26 deletions(-)
  

> 2014-12-14  Chen Gang  <gang.chen.5i5j@gmail.com>
> 
> 	* hppa-tdep.c (inst_saves_gr): Fix logical working flow issues
> 	and check additional store instructions.

Review full disclosure: This is only a spot-check review, since
I no longer have much clue nor interest in hppa. I normally find
that it's fine to just trust the submitter, but then usually
the submitter has a way to actually test the patch, which is not
your case IIUC. Regardless, I will trust you that your patch does
the right thing (tm)...

> ---
>  gdb/hppa-tdep.c | 111 +++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 85 insertions(+), 26 deletions(-)
> 
> diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
> index 627f31a..4f52e43 100644
> --- a/gdb/hppa-tdep.c
> +++ b/gdb/hppa-tdep.c
> @@ -1376,37 +1376,96 @@ 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.

While changing this, can you change "zero it" -> "zero if".

> +     Store a word or doubleword using an absolute memory address formed
> +     using short or long displace- ment or indexed
                            ^^^^^^^^^^^^^^

"displace- ment" -> "displacement".

> +static int
> +inst_saves_gr (unsigned long inst)
> +{
> +  switch ((inst >> 26) & 0x0f)
> +    {
> +      case 0x03:
> +	switch ((inst >> 6) & 0x0f)
> +	  {
> +	    case 0x08:
> +	    case 0x09:
> +	    case 0x0a:
> +	    case 0x0b:
> +	    case 0x0c:
> +	    case 0x0d:
> +	    case 0x0e:
> +	    case 0x0f:
> +	      break;
> +	    default:
> +	      return 0;
> +	  }
> +	/* Fall through */
> +      case 0x18:
> +      case 0x19:
> +      case 0x1a:
> +      case 0x1b:
> +      case 0x1c:
> +      /* no 0x1d or 0x1e -- according to parisc 2.0 document */
> +      case 0x1f:
> +	return hppa_extract_5R_store (inst);
> +      default:
> +	return 0;

Quite honestly, I find your code quite confusing because of
the fall-through. It's up to you, but wouldn't it be clearer
if you had "hppa_extract_5R_store (inst)" instead of "break" +
the "/* Fall through */" ?
  

On 12/15/14 03:35, Joel Brobecker wrote:
>> 2014-12-14  Chen Gang  <gang.chen.5i5j@gmail.com>
>>
>> 	* hppa-tdep.c (inst_saves_gr): Fix logical working flow issues
>> 	and check additional store instructions.
> 
> Review full disclosure: This is only a spot-check review, since
> I no longer have much clue nor interest in hppa. I normally find
> that it's fine to just trust the submitter, but then usually
> the submitter has a way to actually test the patch, which is not
> your case IIUC. Regardless, I will trust you that your patch does
> the right thing (tm)...
> 

Excuse me, I have no related test environments for parisc either. Maybe
I can try to construct the related virtual environments for it, but I am
still not quite sure whether it is enough.

Can we change to another way for it:

 - try to keep the original code no touch.

 - only fix the related typo issues according to the related documents,
   do not add new features (e.g. do not check stby, stbdy, stwa, stda,
   only give the related comments for them).

 - let it pass compiling.

Or, can we find related members in our gdb mailing list which has parisc
environments?

>> ---
>>  gdb/hppa-tdep.c | 111 +++++++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 85 insertions(+), 26 deletions(-)
>>
>> diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
>> index 627f31a..4f52e43 100644
>> --- a/gdb/hppa-tdep.c
>> +++ b/gdb/hppa-tdep.c
>> @@ -1376,37 +1376,96 @@ 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.
> 
> While changing this, can you change "zero it" -> "zero if".
> 

OK, thanks.

>> +     Store a word or doubleword using an absolute memory address formed
>> +     using short or long displace- ment or indexed
>                             ^^^^^^^^^^^^^^
> 
> "displace- ment" -> "displacement".
> 

OK, thanks.

>> +static int
>> +inst_saves_gr (unsigned long inst)
>> +{
>> +  switch ((inst >> 26) & 0x0f)
>> +    {
>> +      case 0x03:
>> +	switch ((inst >> 6) & 0x0f)
>> +	  {
>> +	    case 0x08:
>> +	    case 0x09:
>> +	    case 0x0a:
>> +	    case 0x0b:
>> +	    case 0x0c:
>> +	    case 0x0d:
>> +	    case 0x0e:
>> +	    case 0x0f:
>> +	      break;
>> +	    default:
>> +	      return 0;
>> +	  }
>> +	/* Fall through */
>> +      case 0x18:
>> +      case 0x19:
>> +      case 0x1a:
>> +      case 0x1b:
>> +      case 0x1c:
>> +      /* no 0x1d or 0x1e -- according to parisc 2.0 document */
>> +      case 0x1f:
>> +	return hppa_extract_5R_store (inst);
>> +      default:
>> +	return 0;
> 
> Quite honestly, I find your code quite confusing because of
> the fall-through. It's up to you, but wouldn't it be clearer
> if you had "hppa_extract_5R_store (inst)" instead of "break" +
> the "/* Fall through */" ?
> 

OK, thanks, if still use the switch statement for path v2, I will remove
the /* Fall through */.

Thanks.
  

> Excuse me, I have no related test environments for parisc either. Maybe
> I can try to construct the related virtual environments for it, but I am
> still not quite sure whether it is enough.
> 
> Can we change to another way for it:
> 
>  - try to keep the original code no touch.
> 
>  - only fix the related typo issues according to the related documents,
>    do not add new features (e.g. do not check stby, stbdy, stwa, stda,
>    only give the related comments for them).
> 
>  - let it pass compiling.
> 
> Or, can we find related members in our gdb mailing list which has parisc
> environments?

I think finding people who can test your change would be ideal,
but is not required. I was just making it clear what my review
could bring, or rather not bring. I think it's fine to increase
the scope of the routine as you've been doing, as I think the risk
of disturbing currently supported instructions is small.

I am curious - and you do not have to reply if you do not want to -,
how did you get involved in this change? If you have no testing
environment, it probably means you're not using the code.  What
made you decide to try to fix it?
  

On 12/15/2014 08:49 PM, Joel Brobecker wrote:
>> Excuse me, I have no related test environments for parisc either. Maybe
>> I can try to construct the related virtual environments for it, but I am
>> still not quite sure whether it is enough.
>>
>> Can we change to another way for it:
>>
>>  - try to keep the original code no touch.
>>
>>  - only fix the related typo issues according to the related documents,
>>    do not add new features (e.g. do not check stby, stbdy, stwa, stda,
>>    only give the related comments for them).
>>
>>  - let it pass compiling.
>>
>> Or, can we find related members in our gdb mailing list which has parisc
>> environments?
> 
> I think finding people who can test your change would be ideal,
> but is not required. I was just making it clear what my review
> could bring, or rather not bring. I think it's fine to increase
> the scope of the routine as you've been doing, as I think the risk
> of disturbing currently supported instructions is small.
> 

OK, thanks.

> I am curious - and you do not have to reply if you do not want to -,
> how did you get involved in this change? If you have no testing
> environment, it probably means you're not using the code.  What
> made you decide to try to fix it?
> 

When I compiling binutils, and find the compiler warning, then try to
fix it with other members. I have no environments, not familiar with
parisc, no enough time resources ... but I should still try my best.

Originally, I worked for coredump analysing (kdb for Linux kernel, gdb
for user mode C++ program) -- which have no test environments, either
not familiar many things, but the end time point is sensitive ...

At present, after I really touch the open source in details, I know:

 - Most of persons and companies (include me) have already get benefit
   from open source, especially fundamental software (e.g. binutils,
   gdb), but most of them do not contribute back to open source.

 - But open source still continue, and still benefit to most of persons
   and companies.

 - What I have done is just only try to contribute back to open source.


Thanks.