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

[Pedro Alves <pedro@codesourcery.com>]

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