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

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

[Ajit Kumar Agarwal]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

Please find the following patch based on Pedro Suggestion. 

   [Patch, microblaze]: Added cleanup data for invalid target description.
    
    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.
    
    Signed-off-by:Ajit Agarwal ajitkum@xilinx.com

---
 gdb/microblaze-tdep.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 6a9f11f..f257b96 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -722,7 +722,13 @@ microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
                                               MICROBLAZE_SHR_REGNUM,
                                               "rshr");
         }
-     }
+
+      if (!valid_p)
+        {
+          tdesc_data_cleanup (tdesc_data);
+          return NULL;
+        }
+    }

   /* Allocate space for the new architecture.  */
   tdep = XNEW (struct gdbarch_tdep);
--
1.7.1

Thanks & Regards
Ajit

[-- Attachment #2: 0001-Patch-microblaze-Added-cleanup-data-for-invalid-targ.patch --]
[-- Type: application/octet-stream, Size: 1196 bytes --]

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.

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.

Signed-off-by:Ajit Agarwal ajitkum@xilinx.com
---
 gdb/microblaze-tdep.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 6a9f11f..f257b96 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -722,7 +722,13 @@ microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
                                               MICROBLAZE_SHR_REGNUM,
                                               "rshr");
         }
-     }
+
+      if (!valid_p)
+        {
+          tdesc_data_cleanup (tdesc_data);
+          return NULL;
+        }
+    }
 
   /* Allocate space for the new architecture.  */
   tdep = XNEW (struct gdbarch_tdep);
-- 
1.7.1

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

[Pedro Alves]

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.

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

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

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

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

Or does this uncover other bugs?

Thanks,
Pedro Alves

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

[Ajit Kumar Agarwal]

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

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

[Pedro Alves]

On 10/07/2014 05:36 PM, Ajit Kumar Agarwal wrote:
> 
> 
> -----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.

Please remember to always state this in the commit/submit log.  I've
now added an explicit bullet item for that to the contribution
checklist.

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

Sure you do.  Just hack your target or gdbserver to send a bogus description.

Thanks,
Pedro Alves