Re: [Patch, microblaze]: Fix for remote G Packet message too long error for baremetal.

Pedro Alves
2014-06-24T09:13:56Z

Hi,

I read this again and found things I should have mentioned before
or things I mentioned before but weren't addressed. See below.

On 06/23/2014 08:36 PM, Ajit Kumar Agarwal wrote:

> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
> index 14c1b52..c57437c 100644
> --- a/gdb/microblaze-tdep.c
> +++ b/gdb/microblaze-tdep.c
> @@ -33,13 +33,15 @@
> #include "frame-unwind.h"
> #include "dwarf2-frame.h"
> #include "osabi.h"
> -
> +#include "features/microblaze-with-stack-protect.c"
> +#include "features/microblaze.c"
> #include "gdb_assert.h"

A little odd to see the .c files included in the middle of the .h
files. All other files seem to include the ".c" files
after all the .h files. I wonder which existing file you modelled
from?

> #include <string.h>
> #include "target-descriptions.h"
> #include "opcodes/microblaze-opcm.h"
> #include "opcodes/microblaze-dis.h"
> #include "microblaze-tdep.h"
> +#include "remote.h"
>
>
> /* Instruction macros used for analyzing the prologue. */
> /* This set of instruction macros need to be changed whenever the
> @@ -73,7 +75,8 @@ static const char *microblaze_register_names[] =
> "rpc", "rmsr", "rear", "resr", "rfsr", "rbtr",
> "rpvr0", "rpvr1", "rpvr2", "rpvr3", "rpvr4", "rpvr5", "rpvr6",
> "rpvr7", "rpvr8", "rpvr9", "rpvr10", "rpvr11",
> - "redr", "rpid", "rzpr", "rtlbx", "rtlbsx", "rtlblo", "rtlbhi"
> + "redr", "rpid", "rzpr", "rtlbx", "rtlbsx", "rtlblo", "rtlbhi",
> + "rslr", "rshr"
> };
>
> #define MICROBLAZE_NUM_REGS ARRAY_SIZE (microblaze_register_names)
> @@ -663,17 +666,66 @@ microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
> gdb_assert (reg < sizeof (dwarf2_to_reg_map));
> return dwarf2_to_reg_map[reg];
> }
> +static void

Please make you sure there's an empty line between functions. I
believe I commented on this before.

> +microblaze_register_g_packet_guesses (struct gdbarch *gdbarch)
> +{
> + register_remote_g_packet_guess (gdbarch,
> + MICROBLAZE_NUM_CORE_REGS,
> + tdesc_microblaze);
>
> + register_remote_g_packet_guess (gdbarch,
> + MICROBLAZE_NUM_REGS,
> + tdesc_microblaze_with_stack_protect);
> +}
> static struct gdbarch *

Here too.

> microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> {
> struct gdbarch_tdep *tdep;
> struct gdbarch *gdbarch;
> + struct tdesc_arch_data *tdesc_data = NULL;
> + const struct target_desc *tdesc = info.target_desc;
>
> /* If there is already a candidate, use it. */
> arches = gdbarch_list_lookup_by_info (arches, &info);
> if (arches != NULL)
> return arches->gdbarch;
> + if (tdesc == NULL)
> + tdesc = tdesc_microblaze_with_stack_protect;

Shouldn't the default be to _not_ assume stack protect ?

> + /* Check any target description for validity. */
> + if (tdesc_has_registers (tdesc))
> + {
> + const struct tdesc_feature *feature;
> + int valid_p;
> + int i;
> +
> + feature = tdesc_find_feature (tdesc,
> + "org.gnu.gdb.microblaze.core");
> + if (feature == NULL)
> + return NULL;
> + tdesc_data = tdesc_data_alloc ();
> +
> + valid_p = 1;
> + for (i = 0; i < MICROBLAZE_NUM_CORE_REGS; i++)
> + valid_p &= tdesc_numbered_register (feature, tdesc_data, i,
> + microblaze_register_names[i]);
> + feature = tdesc_find_feature (tdesc,
> + "org.gnu.gdb.microblaze.stack-protect");
> +
> + if (feature != NULL)
> + {
> + valid_p = 1;
> + valid_p &= tdesc_numbered_register (feature, tdesc_data,
> + MICROBLAZE_SLR_REGNUM,
> + "rslr");
> + valid_p &= tdesc_numbered_register (feature, tdesc_data,
> + MICROBLAZE_SHR_REGNUM,
> + "rshr");
> + }
> + }
> + tdep = xcalloc (1, sizeof (struct gdbarch_tdep));
> + gdbarch = gdbarch_alloc (&info, tdep);
> +
> + microblaze_register_g_packet_guesses (gdbarch);
>
> /* Allocate space for the new architecture. */
> tdep = XNEW (struct gdbarch_tdep);
> @@ -725,7 +777,11 @@ microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> dwarf2_append_unwinders (gdbarch);
> frame_unwind_append_unwinder (gdbarch, &microblaze_frame_unwind);
> frame_base_append_sniffer (gdbarch, dwarf2_frame_base_sniffer);
> -
> + if (tdesc_data != NULL)
> + {
> + tdesc_use_registers (gdbarch, tdesc, tdesc_data);
> + set_gdbarch_register_type (gdbarch, microblaze_register_type);

Hmm, why is this set_gdbarch_register_type call necessary?

> + }
> return gdbarch;
> }
>

> + /* Offsets to saved registers. */
> + int register_offsets[MICROBLAZE_NUM_REGS]; /* Must match MICROBLAZE_NUM_REGS. */

The "Must match MICROBLAZE_NUM_REGS" comment now looks unnecessary to me.

As I mentioned before, please don't forget to document the new target
features in the manual.

--
Pedro Alves