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

[Chen Gang <gang dot chen dot 5i5j at gmail dot com>]

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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed