This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [Patch] Microblaze: Port of Linux gdbserver



-----Original Message-----
From: Michael Eager [mailto:eager@eagercon.com] 
Sent: Thursday, October 09, 2014 9:59 PM
To: Ajit Kumar Agarwal; Pedro Alves; Joel Brobecker
Cc: gdb-patches@sourceware.org; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch] Microblaze: Port of Linux gdbserver

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.
To send the patches after incorporating the comments, Is there any other way of sending the patches without top post?
I will make sure that I will trim the mail.

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

Sure. Please find my comments inlined below.

>      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 MIPS and the ARM gdbserver code does not use the CAPS for the above macro defined.

+  (*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)?

To match up with the MIPS target and ARM target they have not used the macro defined. In the Mips  4 is used  and in the ARM target for the THUMB_ARM 2 is used  and for the ARM Mode code 4 is used.  

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.

I have already send  the patch related to the above Pedro's comment. I have also send the patch after incorporating the Pedro feedback comments.

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.

Sure.  The Kernel code(ptrace.h) for Microblaze doesn't have upstream code without PTRACE_GETREGSET.

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

>>And address all comments.

The Microblaze Kernel code PTRACE_GETREGS is always defined and there is no conditional compilation which is without the PTRACE_GETREGS. So I agree with Pedro comment of not using #ifdef PTRACE_GETREGS and in the patch submitted I have removed if #ifdef PTRACE_GETREGS which is not required.

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.

I will be sending this patch soon.

Thanks & Regards
Ajit

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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]