Re: [Patch] Microblaze: Port of Linux gdbserver

[Michael Eager <eager@eagercon.com>]

On 10/08/14 06:51, Ajit Kumar Agarwal wrote:

> Please find the updated patch with feedback comments incorporated.

Please don't top post and please trim email.

Please respond to questions/comments inline.  This makes it easier
to tell whether all issues have been addressed.

>      Microblaze: Port of Linux gdbserver
>
>      This patch is the port of Linux gdbserver.
>      Tested with gdb regression testsuite with this patch of
>      gdbserver.
>
>      gdb/:
>      2014-10-08  Ajit Agarwal  <ajitkum@xilinx.com>
>
>          * configure.tgt (build_gdbserver): New Definition.
>
>      gdb/gdbserver/:
>
>          * gdbserver/Makefile.in (microblaze-linux.c): New target.
>          * gdbserver/configure.srv (microblaze*-*-linux*): New target.
>          * gdbserver/linux-microblaze-low.c: New file.

+#define microblaze_breakpoint_len 4

Use CAPS for macros.

+  (*the_target->read_memory) (where, (unsigned char *) &insn, 4);
+
+  if (insn == microblaze_breakpoint)

Why use the explicit length rather than the macro you just defined?
Why not use sizeof (insn)?

Pedro:
> I'd much prefer if we had that patch in the tree before accepting further patches that tweak things around register names, etc.  Could you send that (as an independent patch, in a new thread).

Please address issues with previous patches before moving
on to submit dependent patches.

Pedro:
> Did this kernel port make it upstream without PTRACE_GETREGSET?
> If there's support for that, can you please switch to using it?

Please answer all questions.

Pedro:
> PTRACE_GETREGS is supposed to an old way of doing things...

And address all comments.

Pedro:
>diff --git a/gdb/regformats/microblaze-with-stack-protect.dat
...
> Please send a preparatory, independent, patch that updates features/Makefile instead and generates this file, in a new thread, with self-contained description, following the
> checklist:
>   https://sourceware.org/gdb/wiki/ContributionChecklist

Preparatory means that the patch should be submitted
before the current patch.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077