RE: [Patch, microblaze]: Added cleanup data for invalid target description

[Ajit Kumar Agarwal <ajit.kumar.agarwal@xilinx.com>]

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Tuesday, October 07, 2014 9:37 PM
To: Ajit Kumar Agarwal; gdb-patches@sourceware.org
Cc: Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Added cleanup data for invalid target description

On 10/07/2014 11:16 AM, Ajit Kumar Agarwal wrote:
> 
> From 00f2692d10e0254366471095516d657693aeff42 Mon Sep 17 00:00:00 2001
> From: Ajit Kumar Agarwal <ajitkum@xhdspdgnu.(none)>
> Date: Tue, 7 Oct 2014 15:06:08 +0530
> Subject: [PATCH] [Patch, microblaze]: Added cleanup data for invalid target description.

>>s/Added/Add/.  But even better would be saying what this actually intends to do, which is "reject".  Note the [PATCH] tag usually end ups stripped when >>the commit is imported into git, but the redundant [Patch, ...] seems like something you added manually, and is unnecessary.

Thanks for the suggestion.
> 
> Cleanup the tdesc data if the target description check is invalid.
> 
> 2014-10-07  Ajit Agarwal  <ajitkum@xilinx.com>
> 
> 	* microblaze-tdep.c (microblaze_gdbarch_init): Use of
> 	tdesc_data_cleanup.

So, I'd write:

~~~
>>[PATCH] Microblaze: Reject invalid target descriptions

>>We currently validate the target description, but then forget to reject it if found invalid.

>>gdb/
>>2014-10-07  Ajit Agarwal  <ajitkum@xilinx.com>

>>	* microblaze-tdep.c (microblaze_gdbarch_init): If the description
>>	isn't valid, release the tdesc arch data and return NULL.
~~~

I will make this Change. Thanks for suggestion.

>>But, you didn't state how you tested this, which should be part of the commit log too.

I have tested the Microblaze Design with and without stack -protect registers. The gdb command "info registers" displayed the register correctly. If stack protect designs is not selected only core registers are displayed. When the stack-protect register is selected in the design, the core registers along with stack-protect registers are displayed.

>>Did you make sure incorrect descriptions are rejected and GDB warns about them?

We don't have the invalid Microblaze design with which I can test gdb warning and cleaning up the tdesc data.

>>Did you make sure valid descriptions do end up correctly used?

Yes.

>>Or does this uncover other bugs?

It doesn't cover other bugs but  I would like to state  the following

"The gdbserver patch which I have sent to FSF would be using  expedite "rpc" and the patch that got committed in FSF for stack-protect registers has the expedite set as "pc". For the gdbserver patch it should be "rpc". I will send a separate patch for this by changing the Makefiles as you have suggested."

Thanks & Regards
Ajit

Thanks,
Pedro Alves