[RFA] remote.c, clean-up mix-up

[RFA] remote.c, clean-up mix-up

[Michael Snyder]

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

I'm not sure if this clean-up ever happens, because the declaration
merely shadows an outer declaration of the same name, and the 
do_cleanups call happens outside the scope of this declaration.

This is my **GUESS** as to what might be intended here.
If anybody has an alternate hypothesis, it's probably
better than mine.


[-- Attachment #2: remote2.txt --]
[-- Type: text/plain, Size: 868 bytes --]

2010-05-05  Michael Snyder  <msnyder@vmware.com>

	* remote.c (remote_threads_info): Remove shadowing declaration.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.407
diff -u -p -5 -r1.407 remote.c
--- remote.c	5 May 2010 22:27:15 -0000	1.407
+++ remote.c	5 May 2010 22:31:28 -0000
@@ -2510,12 +2510,12 @@ remote_threads_info (struct target_ops *
       struct cleanup *back_to = make_cleanup (xfree, xml);
       if (xml && *xml)
 	{
 	  struct gdb_xml_parser *parser;
 	  struct threads_parsing_context context;
-	  struct cleanup back_to = make_cleanup (null_cleanup, NULL);
 
+	  back_to = make_cleanup (null_cleanup, NULL);
 	  context.items = 0;
 	  parser = gdb_xml_create_parser_and_cleanup (_("threads"),
 						      threads_elements,
 						      &context);
 

Re: [RFA] remote.c, clean-up mix-up

[Pedro Alves]

On Wednesday 05 May 2010 23:36:54, Michael Snyder wrote:
> I'm not sure if this clean-up ever happens, because the declaration
> merely shadows an outer declaration of the same name, and the 
> do_cleanups call happens outside the scope of this declaration.

It runs when the outer do_cleanups runs.  But that's not
the point.

> 
> This is my **GUESS** as to what might be intended here.
> If anybody has an alternate hypothesis, it's probably
> better than mine.

A null_cleanup's intention is _always_ to start a new
cleanup scope.  If the variable is unused, it either
means the new cleanup scope wasn't neecessary in the
first place, or, there's a do_cleanups call missing
for this inner cleanups scope.

As I said in the other email, there appears to be
a bug here that may hint at what was meant.  See the
other uses of gdb_xml_create_parser_and_cleanup in
the tree:

osdata.c:

  back_to = make_cleanup (null_cleanup, NULL);
  parser = gdb_xml_create_parser_and_cleanup (_("osdata"),
                                             osdata_elements, &data);
  gdb_xml_use_dtd (parser, "osdata.dtd");

  before_deleting_result = make_cleanup (clear_parsing_data, &data);

  if (gdb_xml_parse (parser, xml) == 0)
    /* Parsed successfully, don't need to delete the result.  */
    discard_cleanups (before_deleting_result);

  do_cleanups (back_to);

solib-target.c:

  back_to = make_cleanup (null_cleanup, NULL);
  parser = gdb_xml_create_parser_and_cleanup (_("target library list"),
                                              library_list_elements, &result);
  gdb_xml_use_dtd (parser, "library-list.dtd");

  before_deleting_result = make_cleanup (solib_target_free_library_list,
                                         &result);

  if (gdb_xml_parse (parser, library) == 0)
    /* Parsed successfully, don't need to delete the result.  */
    discard_cleanups (before_deleting_result);

  do_cleanups (back_to);

memory-map.c:

  back_to = make_cleanup (null_cleanup, NULL);
  parser = gdb_xml_create_parser_and_cleanup (_("target memory map"),
                                              memory_map_elements, &data);

  /* Note: 'clear_result' will zero 'result'.  */
  before_deleting_result = make_cleanup (clear_result, &result);
  data.memory_map = &result;

  if (gdb_xml_parse (parser, memory_map) == 0)
    /* Parsed successfully, don't need to delete the result.  */
    discard_cleanups (before_deleting_result);

  do_cleanups (back_to);


Note how all of them install a cleanup for deleting
the result of the parsing, in case parsing fails midway,
when pieces of the result had already been allocated.



> 
> remote2.txt
>   2010-05-05  Michael Snyder  <msnyder@vmware.com>
> 
>         * remote.c (remote_threads_info): Remove shadowing declaration.
> 
> Index: remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.407
> diff -u -p -5 -r1.407 remote.c
> --- remote.c    5 May 2010 22:27:15 -0000       1.407
> +++ remote.c    5 May 2010 22:31:28 -0000
> @@ -2510,12 +2510,12 @@ remote_threads_info (struct target_ops *
>        struct cleanup *back_to = make_cleanup (xfree, xml);
>        if (xml && *xml)
>         {
>           struct gdb_xml_parser *parser;
>           struct threads_parsing_context context;
> -         struct cleanup back_to = make_cleanup (null_cleanup, NULL);
>  
> +         back_to = make_cleanup (null_cleanup, NULL);
>           context.items = 0;
>           parser = gdb_xml_create_parser_and_cleanup (_("threads"),
>                                                       threads_elements,
>                                                       &context);
>  


-- 
Pedro Alves

Re: [RFA] remote.c, clean-up mix-up

[Pedro Alves]

On Thursday 06 May 2010 00:08:20, Pedro Alves wrote:
> A null_cleanup's intention is always to start a new
> cleanup scope.  If the variable is unused, it either
> means the new cleanup scope wasn't neecessary in the
> first place, or, there's a do_cleanups call missing
> for this inner cleanups scope.
> 
> As I said in the other email, there appears to be
> a bug here that may hint at what was meant.  See the
> other uses of gdb_xml_create_parser_and_cleanup in
> the tree:

...

> 
> Note how all of them install a cleanup for deleting
> the result of the parsing, in case parsing fails midway,
> when pieces of the result had already been allocated.

Here's what I'm testing.

-- 
Pedro Alves

2010-05-05  Pedro Alves  <pedro@codesourcery.com>

	* remote.c (clear_threads_parsing_context): New.
	(remote_threads_info): Delele unused null_cleanup.  Install a
	cleanup to clear the threads_parsing_context in case parsing
	throws.

---
 gdb/remote.c |   25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2010-05-06 00:35:10.000000000 +0100
+++ src/gdb/remote.c	2010-05-06 00:41:27.000000000 +0100
@@ -2482,6 +2482,21 @@ const struct gdb_xml_element threads_ele
   { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
 };
 
+/* Discard the contents of the constructed thread info context.  */
+
+static void
+clear_threads_parsing_context (void *p)
+{
+  struct threads_parsing_context *context = p;
+  int i;
+  struct thread_item *item;
+
+  for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i)
+    xfree (item->extra);
+
+  VEC_free (thread_item_t, context->items);
+}
+
 #endif
 
 /*
@@ -2512,7 +2527,7 @@ remote_threads_info (struct target_ops *
 	{
 	  struct gdb_xml_parser *parser;
 	  struct threads_parsing_context context;
-	  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
+	  struct cleanup *clear_parsing_context;
 
 	  context.items = 0;
 	  parser = gdb_xml_create_parser_and_cleanup (_("threads"),
@@ -2521,6 +2536,9 @@ remote_threads_info (struct target_ops *
 
 	  gdb_xml_use_dtd (parser, "threads.dtd");
 
+	  clear_parsing_context
+	    = make_cleanup (clear_threads_parsing_context, &context);
+
 	  if (gdb_xml_parse (parser, xml) == 0)
 	    {
 	      int i;
@@ -2542,13 +2560,12 @@ remote_threads_info (struct target_ops *
 		      info = demand_private_info (item->ptid);
 		      info->core = item->core;
 		      info->extra = item->extra;
-		      item->extra = 0;
+		      item->extra = NULL;
 		    }
-		  xfree (item->extra);
 		}
 	    }
 
-	  VEC_free (thread_item_t, context.items);
+	  do_cleanups (clear_parsing_context);
 	}
 
       do_cleanups (back_to);

Re: [RFA] remote.c, clean-up mix-up

[Pedro Alves]

On Thursday 06 May 2010 00:44:35, Pedro Alves wrote:
> Here's what I'm testing.

No regressions against x86_64-linux gdbserver.

> 2010-05-05  Pedro Alves  <pedro@codesourcery.com>
> 
>         * remote.c (clear_threads_parsing_context): New.
>         (remote_threads_info): Delele unused null_cleanup.  Install a
>         cleanup to clear the threads_parsing_context in case parsing
>         throws.

Applied as below with a little comment added and typo in
changelog fixed.

-- 
Pedro Alves

2010-05-06  Pedro Alves  <pedro@codesourcery.com>

	* remote.c (clear_threads_parsing_context): New.
	(remote_threads_info): Delete unused null_cleanup.  Install a
	cleanup to clear the threads_parsing_context in case parsing
	throws.

---
 gdb/remote.c |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2010-05-06 01:01:56.000000000 +0100
+++ src/gdb/remote.c	2010-05-06 01:10:47.000000000 +0100
@@ -2482,6 +2482,21 @@ const struct gdb_xml_element threads_ele
   { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
 };
 
+/* Discard the contents of the constructed thread info context.  */
+
+static void
+clear_threads_parsing_context (void *p)
+{
+  struct threads_parsing_context *context = p;
+  int i;
+  struct thread_item *item;
+
+  for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i)
+    xfree (item->extra);
+
+  VEC_free (thread_item_t, context->items);
+}
+
 #endif
 
 /*
@@ -2512,15 +2527,20 @@ remote_threads_info (struct target_ops *
 	{
 	  struct gdb_xml_parser *parser;
 	  struct threads_parsing_context context;
-	  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
+	  struct cleanup *clear_parsing_context;
 
 	  context.items = 0;
+	  /* Note: this parser cleanup is already guarded by BACK_TO
+	     above.  */
 	  parser = gdb_xml_create_parser_and_cleanup (_("threads"),
 						      threads_elements,
 						      &context);
 
 	  gdb_xml_use_dtd (parser, "threads.dtd");
 
+	  clear_parsing_context
+	    = make_cleanup (clear_threads_parsing_context, &context);
+
 	  if (gdb_xml_parse (parser, xml) == 0)
 	    {
 	      int i;
@@ -2542,13 +2562,12 @@ remote_threads_info (struct target_ops *
 		      info = demand_private_info (item->ptid);
 		      info->core = item->core;
 		      info->extra = item->extra;
-		      item->extra = 0;
+		      item->extra = NULL;
 		    }
-		  xfree (item->extra);
 		}
 	    }
 
-	  VEC_free (thread_item_t, context.items);
+	  do_cleanups (clear_parsing_context);
 	}
 
       do_cleanups (back_to);

Re: [RFA] remote.c, clean-up mix-up

[Michael Snyder]

Pedro Alves wrote:
> On Thursday 06 May 2010 00:44:35, Pedro Alves wrote:
>> Here's what I'm testing.
> 
> No regressions against x86_64-linux gdbserver.
> 
>> 2010-05-05  Pedro Alves  <pedro@codesourcery.com>
>>
>>         * remote.c (clear_threads_parsing_context): New.
>>         (remote_threads_info): Delele unused null_cleanup.  Install a
>>         cleanup to clear the threads_parsing_context in case parsing
>>         throws.
> 
> Applied as below with a little comment added and typo in
> changelog fixed.
> 

Thanks a lot for following through!
Michael