This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] remote.c, clean-up mix-up
- From: Pedro Alves <pedro at codesourcery dot com>
- To: Michael Snyder <msnyder at vmware dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Thu, 6 May 2010 00:08:20 +0100
- Subject: Re: [RFA] remote.c, clean-up mix-up
- References: <4BE1F306.8060006@vmware.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