[RFA,4/8] Make strip_bg_char return a unique_xmalloc_ptr
Commit Message
This changes strip_bg_char to return a unique_xmalloc_ptr and removes
several cleanups.
gdb/ChangeLog
2017-10-13 Tom Tromey <tom@tromey.com>
* infcmd.c (strip_bg_char): Return gdb::unique_xmalloc_ptr.
(run_command_1, continue_command, step_1, jump_command)
(signal_command, until_command, advance_command, finish_command)
(attach_command): Update.
---
gdb/ChangeLog | 7 +++++
gdb/infcmd.c | 84 ++++++++++++++++++-----------------------------------------
2 files changed, 32 insertions(+), 59 deletions(-)
Comments
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.
On 2017-10-16 05:33, 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.
When reviewing previous cleanup-removal patches, I tried to look for
something that hinted like it would install and return a cleanup, but
it's obviously not a 100% reliable method. What you could do is
temporarily add
struct cleanup *before = cleanup_chain;
at the beginning of the function, and
gdb_assert (cleanup_chain == before);
at the end.
Simon
Simon Marchi <simon.marchi@polymtl.ca> writes:
> When reviewing previous cleanup-removal patches, I tried to look for
> something that hinted like it would install and return a cleanup, but
> it's obviously not a 100% reliable method. What you could do is
> temporarily add
>
> struct cleanup *before = cleanup_chain;
>
> at the beginning of the function, and
>
> gdb_assert (cleanup_chain == before);
>
> at the end.
Yes, that is one approach I though of, but it needs changing code. I am
looking at running gdb testsuite with valgrind, but there are too many
warnings on leak, so it is not that useful to find new leaks.
The patch is the good direction to go, so I don't want to block it.
Meanwhile, I am still looking at memory leaks reported by valgrind.
Tom, if you don't hear me by Friday this week, feel free to commit it.
>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes:
Yao> Yes, that is one approach I though of, but it needs changing code. I am
Yao> looking at running gdb testsuite with valgrind, but there are too many
Yao> warnings on leak, so it is not that useful to find new leaks.
That sounds pretty bad.
A valgrind or ASAN buildbot would be good...
Yao> The patch is the good direction to go, so I don't want to block it.
Yao> Meanwhile, I am still looking at memory leaks reported by valgrind.
I don't know if the cleanup checker still works, but that would be one
route. I don't believe there are all that many dangling cleanups any
more, but I do agree that it's hard to know.
It's maybe worth pointing out that, at least in the past (I didn't look
recently), commands were always run with a cleanup installed, and this
cleanup was run after the command returned. So, before the cleanup
checker work, it was normal to see dangling cleanups in command
implementations. If this still exists then I think it would reduce the
risk of changing strip_bg_char.
Tom
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
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
[...]
Pedro> I think we're good. Found one nit:
Thanks.
>> + gdb::unique_xmalloc_ptr<char> stripped =
>> + strip_bg_char (count_string, &async_exec);
Pedro> = on next line.
I finally got around to fixing this locally.
Tom
@@ -421,7 +421,7 @@ construct_inferior_arguments (int argc, char **argv)
NULL is returned. *BG_CHAR_P is an output boolean that indicates
whether the '&' character was found. */
-static char *
+static gdb::unique_xmalloc_ptr<char>
strip_bg_char (const char *args, int *bg_char_p)
{
const char *p;
@@ -441,13 +441,14 @@ strip_bg_char (const char *args, int *bg_char_p)
*bg_char_p = 1;
if (p != args)
- return savestring (args, p - args);
+ return gdb::unique_xmalloc_ptr<char>
+ (savestring (args, p - args));
else
- return NULL;
+ return gdb::unique_xmalloc_ptr<char> (nullptr);
}
*bg_char_p = 0;
- return xstrdup (args);
+ return gdb::unique_xmalloc_ptr<char> (xstrdup (args));
}
/* Common actions to take after creating any sort of inferior, by any
@@ -592,7 +593,6 @@ run_command_1 (char *args, int from_tty, enum run_how run_how)
struct ui_out *uiout = current_uiout;
struct target_ops *run_target;
int async_exec;
- struct cleanup *args_chain;
CORE_ADDR pc;
dont_repeat ();
@@ -616,8 +616,8 @@ run_command_1 (char *args, int from_tty, enum run_how run_how)
reopen_exec_file ();
reread_symbols ();
- 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);
-
/* We call get_inferior_args() because we might need to compute
the value now. */
run_target->to_create_inferior (run_target, exec_file,
@@ -850,13 +847,12 @@ continue_command (char *args, int from_tty)
{
int async_exec;
int all_threads = 0;
- struct cleanup *args_chain;
ERROR_NO_INFERIOR;
/* Find out whether we must run in the background. */
- 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 ();
if (args != NULL)
{
@@ -918,9 +914,6 @@ continue_command (char *args, int from_tty)
}
}
- /* Done with ARGS. */
- do_cleanups (args_chain);
-
ERROR_NO_INFERIOR;
ensure_not_tfind_mode ();
@@ -1065,7 +1058,6 @@ step_1 (int skip_subroutines, int single_inst, char *count_string)
{
int count;
int async_exec;
- struct cleanup *args_chain;
struct thread_info *thr;
struct step_command_fsm *step_sm;
@@ -1074,16 +1066,14 @@ step_1 (int skip_subroutines, int single_inst, char *count_string)
ensure_valid_thread ();
ensure_not_running ();
- 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);
+ count_string = stripped.get ();
prepare_execution_command (¤t_target, async_exec);
count = count_string ? parse_and_eval_long (count_string) : 1;
- /* Done with ARGS. */
- do_cleanups (args_chain);
-
clear_proceed_status (1);
/* Setup the execution command state machine to handle all the COUNT
@@ -1259,7 +1249,6 @@ jump_command (char *arg, int from_tty)
struct symbol *fn;
struct symbol *sfn;
int async_exec;
- struct cleanup *args_chain;
ERROR_NO_INFERIOR;
ensure_not_tfind_mode ();
@@ -1267,8 +1256,8 @@ jump_command (char *arg, int from_tty)
ensure_not_running ();
/* Find out whether we must run in the background. */
- arg = strip_bg_char (arg, &async_exec);
- args_chain = make_cleanup (xfree, arg);
+ gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (arg, &async_exec);
+ arg = stripped.get ();
prepare_execution_command (¤t_target, async_exec);
@@ -1280,9 +1269,6 @@ jump_command (char *arg, int from_tty)
if (sals.size () != 1)
error (_("Unreasonable jump request"));
- /* Done with ARGS. */
- do_cleanups (args_chain);
-
symtab_and_line &sal = sals[0];
if (sal.symtab == 0 && sal.pc == 0)
@@ -1341,7 +1327,6 @@ signal_command (char *signum_exp, int from_tty)
{
enum gdb_signal oursig;
int async_exec;
- struct cleanup *args_chain;
dont_repeat (); /* Too dangerous. */
ERROR_NO_INFERIOR;
@@ -1350,8 +1335,9 @@ signal_command (char *signum_exp, int from_tty)
ensure_not_running ();
/* Find out whether we must run in the background. */
- signum_exp = strip_bg_char (signum_exp, &async_exec);
- args_chain = make_cleanup (xfree, signum_exp);
+ gdb::unique_xmalloc_ptr<char> stripped
+ = strip_bg_char (signum_exp, &async_exec);
+ signum_exp = stripped.get ();
prepare_execution_command (¤t_target, async_exec);
@@ -1374,8 +1360,6 @@ signal_command (char *signum_exp, int from_tty)
oursig = gdb_signal_from_command (num);
}
- do_cleanups (args_chain);
-
/* Look for threads other than the current that this command ends up
resuming too (due to schedlock off), and warn if they'll get a
signal delivered. "signal 0" is used to suppress a previous
@@ -1620,7 +1604,6 @@ static void
until_command (char *arg, int from_tty)
{
int async_exec;
- struct cleanup *args_chain;
ERROR_NO_INFERIOR;
ensure_not_tfind_mode ();
@@ -1628,8 +1611,8 @@ until_command (char *arg, int from_tty)
ensure_not_running ();
/* Find out whether we must run in the background. */
- arg = strip_bg_char (arg, &async_exec);
- args_chain = make_cleanup (xfree, arg);
+ gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (arg, &async_exec);
+ arg = stripped.get ();
prepare_execution_command (¤t_target, async_exec);
@@ -1637,16 +1620,12 @@ until_command (char *arg, int from_tty)
until_break_command (arg, from_tty, 0);
else
until_next_command (from_tty);
-
- /* Done with ARGS. */
- do_cleanups (args_chain);
}
static void
advance_command (char *arg, int from_tty)
{
int async_exec;
- struct cleanup *args_chain;
ERROR_NO_INFERIOR;
ensure_not_tfind_mode ();
@@ -1657,15 +1636,12 @@ advance_command (char *arg, int from_tty)
error_no_arg (_("a location"));
/* Find out whether we must run in the background. */
- arg = strip_bg_char (arg, &async_exec);
- args_chain = make_cleanup (xfree, arg);
+ gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (arg, &async_exec);
+ arg = stripped.get ();
prepare_execution_command (¤t_target, async_exec);
until_break_command (arg, from_tty, 1);
-
- /* Done with ARGS. */
- do_cleanups (args_chain);
}
/* Return the value of the result of a function at the end of a 'finish'
@@ -2031,7 +2007,6 @@ finish_command (char *arg, int from_tty)
{
struct frame_info *frame;
int async_exec;
- struct cleanup *args_chain;
struct finish_command_fsm *sm;
struct thread_info *tp;
@@ -2041,17 +2016,14 @@ finish_command (char *arg, int from_tty)
ensure_not_running ();
/* Find out whether we must run in the background. */
- arg = strip_bg_char (arg, &async_exec);
- args_chain = make_cleanup (xfree, arg);
+ gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (arg, &async_exec);
+ arg = stripped.get ();
prepare_execution_command (¤t_target, async_exec);
if (arg)
error (_("The \"finish\" command does not take any arguments."));
- /* Done with ARGS. */
- do_cleanups (args_chain);
-
frame = get_prev_frame (get_selected_frame (_("No selected frame.")));
if (frame == 0)
error (_("\"finish\" not meaningful in the outermost frame."));
@@ -2846,7 +2818,6 @@ void
attach_command (char *args, int from_tty)
{
int async_exec;
- struct cleanup *args_chain;
struct target_ops *attach_target;
struct inferior *inferior = current_inferior ();
enum attach_post_wait_mode mode;
@@ -2869,8 +2840,8 @@ attach_command (char *args, int from_tty)
this function should probably be moved into target_pre_inferior. */
target_pre_inferior (from_tty);
- 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 ();
attach_target = find_attach_target ();
@@ -2948,17 +2919,12 @@ attach_command (char *args, int from_tty)
a->mode = mode;
add_inferior_continuation (attach_command_continuation, a,
attach_command_continuation_free_args);
- /* Done with ARGS. */
- do_cleanups (args_chain);
if (!target_is_async_p ())
mark_infrun_async_event_handler ();
return;
}
- /* Done with ARGS. */
- do_cleanups (args_chain);
-
attach_post_wait (args, from_tty, mode);
}