On 05/10/2011 09:58 PM, Ulrich Weigand wrote:
>> > +static int
>> > +thumb_copy_pop_pc_16bit (struct gdbarch *gdbarch, unsigned short insn1,
>> > +			 struct regcache *regs,
>> > +			 struct displaced_step_closure *dsc)
>> > +{
>> > +  dsc->u.block.regmask = insn1 & 0x00ff;
>> > +
>> > +  /* Rewrite instruction: POP {rX, rY, ...,rZ, PC}
>> > +     to :
>> > +
>> > +     (1) register list is not empty,
>> > +     Prepare: tmp[0] <- r8,
>> > +
>> > +     POP {rX};   PC is stored in rX
>> > +     MOV r8, rX; finally, PC is stored in r8
>> > +     POP {rX, rY, ...., rZ}
>> > +
>> > +     Cleanup: PC <-r8, r8 <- tmp[0]
> It seems this approach is actually incorrect.  If you do a
>   POP {rX, rY, ..., rZ, PC}
> the value at the SP gets restored into rX, the value at rX+4
> into rY and so on, and the value at the highest address gets
> restored into PC.
> 
> With your replacement sequence, it would appear that you
> instead get the value at the *lowest* address (just SP)
> restored into the PC ...
> 

Hmmm, you are right.  In test case, I put the same address on the first
two slots on stack, and pop them to r1 and pc respectively.  That is the
reason why this bug is not found by test case.

In my new patch, there are three different cases to handle POP instruction,
1.  register list is full, no free register.  The code sequence I am
using is like

     POP {r0, r1, ...., r6};
     POP {r7};
     MOV r8, r7;
     POP {r7};

after execution of this sequence, PC's value is stored in r7, and r7's
value is stored in r8.  In cleanup, we can set PC, r7, and r8 accordingly.

2.  register list is not full, and not empty.  In this case, we scan the
code to find a free register, rN.  Run the follow code sequence,

     POP {rX, rY, ...., rZ};
     POP {rN};

After execution of this sequence, PC's value is stored in rN.  In
cleanup, we can set PC from rN.

3.  register list is empty.  This case is relative simple.

     POP {r0}

In cleanup, we store r0's value to PC.

The testcase is update according to these three cases, and value of PC
after each POP instruction is checked.  I'll resend test case patch later.

> 
> Apart from this, just a few minor issues:
> 
>> > +/* Common copy routine for svc instruciton.  */
>> >  
>> > +static int
>> > +copy_svc (struct gdbarch *gdbarch, struct regcache *regs,
>> > +	  struct displaced_step_closure *dsc)
>> > +{
> I guess to keep in sync with terminology in the rest of the file,
> this really should be called install_svc ...
> 

Well, the function `copy_svc' is a little bit different from other
install_* routines.  Anyway, I am OK to name it to install_svc.  Fixed.

>> > +      err = thumb_copy_unmodified_16bit (gdbarch, insn1,"shift/add/sub/mov/cmp",
> Formatting: space after the comma (here and at a couple other places).
>

Fixed.

>> > +      thumb_process_displaced_32bit_insn(gdbarch, insn1, insn2, regs, dsc);
>> > +    }
>> > +  else
>> > +    thumb_process_displaced_16bit_insn(gdbarch, insn1, regs, dsc);
> Formatting: space before (

Fixed.

-- 
Yao (齐尧)