On Sat, 01 Nov 2008 19:54:10 +0100, Joel Brobecker wrote:
> Can we maybe expand the comment explaining why we can't save the whole
> bundle in the shadow buffer?

Done, mostly copied the mail of mine there.


> >  const unsigned char *
> >  ia64_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr)
> >  {
> > -  static unsigned char breakpoint[] =
> > -    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
...
> I think there is a piece missing in this change. Looks like breakpoint
> is no longer initialized.

According to ISO C90 6.5.7 such a static array is (was) initialized to zeroes.


> I think that what you need to do is read the instruction bundle from memory,
> and insert the breakpoint instruction, and then copy you
> BUNDLE_LEN - 2 bytes into the breakpoint buffer.

I just kept it broken with no regressions.  But OK, fixed.


> The reason why we didn't trip over this, I believe, is because this routine
> is only used when using the default insert/remove bp routines, or when using
> the remote protocol. It's also used from monitor.c.

The return value is used for the ia64 case only in bp_loc_is_permanent,
nowhere else.  And for ia64 the arch interface is inappropriate but we can
keep it this way as it is probably the right interface for anything non-ia64.


> Also, the check for BUNDLE_LEN against BREAKPOINT_MAX is slightly
> over-pessimistic. Can you move it somewhere at the beginning of
> the file (this assumption is required in other parts of the code),
> and compare BREAKPOINT_MAX against the actual size of data saved
> (BUNDLE_LEN -2)?

Right, thanks, done.


Regards,
Jan