RE: [Patch] Microblaze: Port of Linux gdbserver

[Ajit Kumar Agarwal <ajit dot kumar dot agarwal at xilinx dot com>]

-----Original Message-----
From: Michael Eager [mailto:eager@eagerm.com] 
Sent: Friday, October 10, 2014 5:12 AM
To: Ajit Kumar Agarwal; Michael Eager; 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/09/14 11:54, Ajit Kumar Agarwal wrote:
>
> To send the patches after incorporating the comments, Is there any other way of sending the patches without top post?

After you address comments, include the changelog at the end and attach the patch (unless it is just a few lines).  That way we can tell how you responded to each comment.

> +#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.

>Let's follow the GNU coding standard, even if some other targets haven't.

https://gcc.gnu.org/ml/gcc-patches/2000-09/msg00901.html
https://gcc.gnu.org/ml/gcc-patches/2000-09/msg00912.html

Here is the mailing list archive that mentions when not to use ALL_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)?
>
> 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.

>Let's follow good coding practice, even if there have been lapses in the past.
Unless there is some particular relevance to instruction length on MIPS or ARM/Thumb, let's stick to what is relevant to MicroBlaze.

Ok.

> 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.

>I haven't seen this patch.  Please let me know when you posted it, or send me a link to it in the mailing list archive.


>If you submit a patch which depends on previously submitted patches which have not been accepted, the new patch will not be accepted.
Please don't submit dependent patches until all prior prerequisite patches are accepted.

http://sourceware-org.1504.n7.nabble.com/PATCH-Microblaze-Reject-invalid-target-descriptions-td285201.html


> 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.

OK.  As mentioned above, please do not resubmit this patch until that patch is submitted and accepted.

http://sourceware-org.1504.n7.nabble.com/PATCH-Microblaze-Replace-microblaze-expedite-from-pc-to-rpc-td285649.html

Thanks & Regards
Ajit

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