Re: [patch] ia64: Fix breakpoints memory shadow

[Joel Brobecker <brobecker@adacore.com>]

> I agree your proposal would be better but we have to comply with the current
> `struct bp_target_info' layout which is being intepreted outside of
> ia64-tdep.c - in breakpoint_restore_shadows.
> 
> If we would like to store the whole bundle to SHADOW_CONTENTS we would have to
> store already the base address (`address & ~0x0f') into PLACED_ADDRESS.  In
> such case there is no other place where to store SLOTNUM (`adress & 0x0f',
> value in the range <0..2>).  We need to know SLOTNUM in
> ia64_memory_remove_breakpoint.

Aie aie aie, that's true. I was also looking at the idea of stuffing
the slot number in the first 5 bits of the bundle, but that wouldn't
work either, as breakpoint_restore_shadows can restore those bits
as well. There is one alternative which is to define a gdbarch
restore_shadow, but your approach does work after all, so let's not
complexify the rest of GDB just for ia64.

Can we maybe expand the comment explaining why we can't save the whole
bundle in the shadow buffer?

>  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 };
> -  *lenptr = sizeof (breakpoint);
> -#if 0
> -  *pcptr &= ~0x0f;
> +  static unsigned char breakpoint[BUNDLE_LEN];
> +  int slotnum = (int) (*pcptr & 0x0f) / SLOT_MULTIPLIER;
> +
> +  if (slotnum > 2)
> +    error (_("Can't insert breakpoint for slot numbers greater than 2."));
> +
> +#if BREAKPOINT_MAX < BUNDLE_LEN
> +# error "BREAKPOINT_MAX < BUNDLE_LEN"
>  #endif
> +
> +  *lenptr = BUNDLE_LEN - 2;
> +
>    return breakpoint;
>  }

I think there is a piece missing in this change. Looks like breakpoint
is no longer initialized. 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.

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.

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)?

The rest looks OK to me.

-- 
Joel