Again here is a new patch with source code clean up as you pointed out.

Jim Blandy wrote:
> On 1/5/06, Jie Zhang <jzhang918@gmail.com> wrote:
> 
>>>Am I reading bfin_frame_prev_register correctly when I conclude that
>>>the only saved registers it can find are the PC and the FP?  How are
>>
>>It can also find other registers besides the PC and the FP. It looks like:
>>
>>if(regnum == BFIN_PC_REGNUM)
>>   {
>>     ...
>>   }
>>else if (regnum == BFIN_FP_REGNUM)
>>   {
>>     ...
>>   }
>>else
>>   read_memory (...);
> 
> 
> Right, but that's all under the control of:
> 
>   if (regnum < BFIN_NUM_REGS && cache->saved_regs[regnum] != -1)
> 
> and if I'm reading the rest of the code right, cache->saved_regs[R]
> can only be != -1 when R is BFIN_PC_REGNUM or BFIN_FP_REGNUM.  But
> those test suite results look pretty good.  Does the Blackfin GCC port
> produce Dwarf CFI?  If so, then you're probably not exercising the
> tdep unwinding code much.
> 
Yes, you are right. BFIN_PC_REGNUM and BFIN_FP_REGNUM are the only two
registers which we can easily find in the frame. Blackfin GCC port
produces DWARF CFI.

> Either way, this is a quality of implementation issue, not a coding
> convention or interface usage issue, so if it's not a problem for you
> (or someone willing to pay you), I don't think it needs to be
> addressed for the port to go in.
> 
Anyway, prologue analysis and frame unwinding are two things among
others that Blackfin port needs further improvement. I'll do it after
the port is accepted.

> So, now we can get to picky stuff:
> 
> /* ???  */
> #define UPPER_LIMIT			(40)
> #define RETS_OFFSET			4
> 
> That comment should probably be filled in.
> 
Replaced with a more meaningful comment.

>   /* This would come after the LINK instruction in the ret_from_signal ()
>      function, hence the frame id would be SP + 8.  */
>   this_id = frame_id_build (sp + 8, frame_pc_unwind(next_frame));
> 
> GNU coding standards call for a space before the opening parenthesis
> of a function call argument list.
> 
> 	  /* Read the value in from memory.  */
> 
> 	  if(regnum == BFIN_PC_REGNUM)
> 
> Similarly for 'if', 'while', and 'for' conditions.  I just searched in
> Emacs for the regexp '[a-z]('.
> 
I have fixed hundreds of such things when I was cleaning up the source
code. I missed some. Now fixed.

> static int
> bfin_sim_regno (int regno)
> {
>   switch (regno)
>     {
>       case SIM_BFIN_ASTAT_REGNUM :
>       case SIM_BFIN_CYCLES_REGNUM :
>       case SIM_BFIN_CYCLES2_REGNUM :
>       case SIM_BFIN_USP_REGNUM :
> 
> The GNU coding standards have 'case' labels un-indented two spaces
> relative to the code they label, like this.
> 
> static int
> bfin_sim_regno (int regno)
> {
>   switch (regno)
>     {
>     case SIM_BFIN_ASTAT_REGNUM :
>     case SIM_BFIN_CYCLES_REGNUM :
>     case SIM_BFIN_CYCLES2_REGNUM :
>     case SIM_BFIN_USP_REGNUM :
>     case SIM_BFIN_SEQSTAT_REGNUM :
> 
Done.

> Looking at:
> 
> static void
> bfin_linux_sigtramp_frame_prev_register (struct frame_info *next_frame,
> 					 void **this_cache,
> 					 int regnum, int *optimizedp,
> 					 enum lval_type *lvalp,
> 					 CORE_ADDR *addrp,
> 					 int *realnump, gdb_byte *valuep)
> 
> When we have a long argument list like this, I think it's more legible
> to have each argument on its own line, or at least have line groupings
> reflect some sort of semantic grouping (an array and its length, for
> example).  The bfin_frame_prev_register case is similar.  This is up
> to you, though.
> 
Done.

>   /* Integral values greater than one word are stored in consecutive
>      registers starting with R0.  This will always be a multiple of
>      the regiser size.  */
> 
> Typo.
> 
"regiser" has been replaced with "register".

> Overall, though, it looks pretty clean to me.
> 
> 
Thanks! :-)

Best regards,
Jie