This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA 4/8] Make strip_bg_char return a unique_xmalloc_ptr
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>, Tom Tromey <tom at tromey dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 18 Oct 2017 10:36:57 +0100
- Subject: Re: [RFA 4/8] Make strip_bg_char return a unique_xmalloc_ptr
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6735E13A53
- References: <20171013205950.22943-1-tom@tromey.com> <20171013205950.22943-5-tom@tromey.com> <86shejwi6t.fsf@gmail.com>
On 10/16/2017 10:33 AM, Yao Qi wrote:
> Tom Tromey <tom@tromey.com> writes:
>
>> - args = strip_bg_char (args, &async_exec);
>> - args_chain = make_cleanup (xfree, args);
>> + gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
>> + args = stripped.get ();
>>
>> /* Do validation and preparation before possibly changing anything
>> in the inferior. */
>> @@ -663,9 +663,6 @@ run_command_1 (char *args, int from_tty, enum run_how run_how)
>> uiout->flush ();
>> }
>>
>> - /* Done with ARGS. */
>> - do_cleanups (args_chain);
>> -
>
> My concern is that we may leak something if some cleanups are registered
> to the cleanup chain in the callees between make_cleanup and do_cleanups
> here. However, I am not sure how to detect that.
Personally, I'm not as much worried about cleanup leaks (because there's
a TRY/CATCH at the top of the event loop, and TRY detects dangling
cleanups via restore_my_cleanups), as about some cleanup installed in
that chain, whose do_cleanups call must be run at the particular point
it's being run.
I've touched and moved around these cleanups in the past, and what
thought might be worth a second look was whether prepare_execute_command
or something around it was installing a cleanup. It isn't, and when I went
looking a bit deeper I found 8bc2fe488957, where I had written:
~~~
attach_command installs a cleanup to re-enable stdin, but that's not
necessary, as per the comment in prepare_execution_command. In any
case, if someday it turns out necessary, we have a single place to
install it now.
~~~
I think we're good. Found one nit:
> @@ -1074,16 +1066,14 @@ step_1 (int skip_subroutines, int single_inst, char *count_string)
...
> - count_string = strip_bg_char (count_string, &async_exec);
> - args_chain = make_cleanup (xfree, count_string);
> + gdb::unique_xmalloc_ptr<char> stripped =
> + strip_bg_char (count_string, &async_exec);
= on next line.
Thanks,
Pedro Alves