This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA 4/8] Make strip_bg_char return a unique_xmalloc_ptr


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]