* make execute_control_command conform to docs
@ 2004-02-24 16:47 David Allan
2004-02-24 20:36 ` Andrew Cagney
0 siblings, 1 reply; 6+ messages in thread
From: David Allan @ 2004-02-24 16:47 UTC (permalink / raw)
To: GDB patches
[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]
I've filed PR cli/1566 on this issue. Also, I'm in the process of doing
the copyright assignment, but haven't yet completed it. From lurking on
the list, it seems like two line patches aren't significant enough to be
a problem. (I'm hoping.)
The attached patch makes two one-line changes to execute_control_command
in cli/cli-script.c to make it conform to the GDB internals
documentation section 13.1 on safely using cleanups. Without the patch,
execute_control_command depends on the caller having done the right
thing by putting something on the cleanup_chain. If
execute_control_command is called when cleanup_chain is null, the
cleanups execute_control_command puts on the chain are not removed, and
undefined behavior results.
The patch makes execute_control_command safe by initializing old_chain
with a call to make_cleanup with a null_cleanup and always calling
do_cleanups before returning if anything other than the null_cleanup has
been put on the chain. If the null_cleanup is the only thing put on the
chain, it could get left on it at return, but, as I read the code,
that's harmless. The other possiblity would be to call do_cleanups at
every point we could return, but that seemed like more changes than were
necessary to fix the problem.
Dave
[-- Attachment #2: execute_control_command.diff --]
[-- Type: text/plain, Size: 679 bytes --]
Index: cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.21
diff -u -r1.21 cli-script.c
--- cli-script.c 22 Dec 2003 03:43:19 -0000 1.21
+++ cli-script.c 24 Feb 2004 15:55:06 -0000
@@ -294,7 +294,7 @@
{
struct expression *expr;
struct command_line *current;
- struct cleanup *old_chain = 0;
+ struct cleanup *old_chain = make_cleanup (null_cleanup, 0);
struct value *val;
struct value *val_mark;
int loop;
@@ -427,8 +427,7 @@
return invalid_control;
}
- if (old_chain)
- do_cleanups (old_chain);
+ do_cleanups (old_chain);
return ret;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: make execute_control_command conform to docs
2004-02-24 16:47 make execute_control_command conform to docs David Allan
@ 2004-02-24 20:36 ` Andrew Cagney
2004-02-24 21:51 ` Dave Allan
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cagney @ 2004-02-24 20:36 UTC (permalink / raw)
To: David Allan; +Cc: GDB patches
>
> Index: cli-script.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
> retrieving revision 1.21
> diff -u -r1.21 cli-script.c
> --- cli-script.c 22 Dec 2003 03:43:19 -0000 1.21
> +++ cli-script.c 24 Feb 2004 15:55:06 -0000
> @@ -294,7 +294,7 @@
> {
> struct expression *expr;
> struct command_line *current;
> - struct cleanup *old_chain = 0;
> + struct cleanup *old_chain = make_cleanup (null_cleanup, 0);
> struct value *val;
> struct value *val_mark;
> int loop;
> @@ -427,8 +427,7 @@
> return invalid_control;
> }
>
> - if (old_chain)
> - do_cleanups (old_chain);
> + do_cleanups (old_chain);
>
> return ret;
> }
Close,
Paths where the function explicitly does a "return" such as this:
default:
warning ("Invalid control type in command structure.");
return invalid_control;
should cleanup (or better? let things flow to the end), and this:
struct cleanup *old_chain = make_cleanup (null_cleanup, 0);
...
old_chain = make_cleanup (free_current_contents, &new_line);
leaves a dangling cleanup (it is eventually processed but I'm not sure
where).
I'll re-arange the relevant code.
Andrew
PS: Remember to include a ChangeLog entry in patches.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: make execute_control_command conform to docs
2004-02-24 20:36 ` Andrew Cagney
@ 2004-02-24 21:51 ` Dave Allan
2004-02-24 23:12 ` Andrew Cagney
0 siblings, 1 reply; 6+ messages in thread
From: Dave Allan @ 2004-02-24 21:51 UTC (permalink / raw)
To: Andrew Cagney; +Cc: GDB patches
I was trying to make the patch as small as possible, but I ought to have
done it as you say. Letting everything flow through to the end so that
there is only one return point is definitely the cleanest way to do it.
Dave
On Tue, 2004-02-24 at 15:35, Andrew Cagney wrote:
> >
> > Index: cli-script.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
> > retrieving revision 1.21
> > diff -u -r1.21 cli-script.c
> > --- cli-script.c 22 Dec 2003 03:43:19 -0000 1.21
> > +++ cli-script.c 24 Feb 2004 15:55:06 -0000
> > @@ -294,7 +294,7 @@
> > {
> > struct expression *expr;
> > struct command_line *current;
> > - struct cleanup *old_chain = 0;
> > + struct cleanup *old_chain = make_cleanup (null_cleanup, 0);
> > struct value *val;
> > struct value *val_mark;
> > int loop;
> > @@ -427,8 +427,7 @@
> > return invalid_control;
> > }
> >
> > - if (old_chain)
> > - do_cleanups (old_chain);
> > + do_cleanups (old_chain);
> >
> > return ret;
> > }
>
> Close,
>
> Paths where the function explicitly does a "return" such as this:
>
> default:
> warning ("Invalid control type in command structure.");
> return invalid_control;
>
> should cleanup (or better? let things flow to the end), and this:
>
> struct cleanup *old_chain = make_cleanup (null_cleanup, 0);
> ...
> old_chain = make_cleanup (free_current_contents, &new_line);
>
> leaves a dangling cleanup (it is eventually processed but I'm not sure
> where).
>
> I'll re-arange the relevant code.
>
> Andrew
>
> PS: Remember to include a ChangeLog entry in patches.
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: make execute_control_command conform to docs
2004-02-24 21:51 ` Dave Allan
@ 2004-02-24 23:12 ` Andrew Cagney
2004-02-25 14:02 ` Dave Allan
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cagney @ 2004-02-24 23:12 UTC (permalink / raw)
To: da_gdb; +Cc: GDB patches
[-- Attachment #1: Type: text/plain, Size: 317 bytes --]
> I was trying to make the patch as small as possible, but I ought to have
> done it as you say. Letting everything flow through to the end so that
> there is only one return point is definitely the cleanest way to do it.
Here, yes, it appears to be the case. Any way, does the attached appear
to work?
Andrew
[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 2473 bytes --]
2004-02-24 Andrew Cagney <cagney@redhat.com>
PR cli/1566. Problem found, and fix suggested by David Allan.
* cli/cli-script.c (execute_control_command): Unconditionally
install a cleanup. Default "ret" to "invalid_control". Use
"break" instead of "return" to escape from the switch.
Index: cli/cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.21
diff -u -r1.21 cli-script.c
--- cli/cli-script.c 22 Dec 2003 03:43:19 -0000 1.21
+++ cli/cli-script.c 24 Feb 2004 23:10:13 -0000
@@ -294,21 +294,25 @@
{
struct expression *expr;
struct command_line *current;
- struct cleanup *old_chain = 0;
+ struct cleanup *old_chain = make_cleanup (null_cleanup, 0);
struct value *val;
struct value *val_mark;
int loop;
enum command_control_type ret;
char *new_line;
+ /* Start by assuming failure, if a problem is detected, the code
+ below will simply "break" out of the switch. */
+ ret = invalid_control;
+
switch (cmd->control_type)
{
case simple_control:
/* A simple command, execute it and return. */
new_line = insert_args (cmd->line);
if (!new_line)
- return invalid_control;
- old_chain = make_cleanup (free_current_contents, &new_line);
+ break;
+ make_cleanup (free_current_contents, &new_line);
execute_command (new_line, 0);
ret = cmd->control_type;
break;
@@ -325,8 +329,8 @@
/* Parse the loop control expression for the while statement. */
new_line = insert_args (cmd->line);
if (!new_line)
- return invalid_control;
- old_chain = make_cleanup (free_current_contents, &new_line);
+ break;
+ make_cleanup (free_current_contents, &new_line);
expr = parse_expression (new_line);
make_cleanup (free_current_contents, &expr);
@@ -385,8 +389,8 @@
{
new_line = insert_args (cmd->line);
if (!new_line)
- return invalid_control;
- old_chain = make_cleanup (free_current_contents, &new_line);
+ break;
+ make_cleanup (free_current_contents, &new_line);
/* Parse the conditional for the if statement. */
expr = parse_expression (new_line);
make_cleanup (free_current_contents, &expr);
@@ -424,11 +428,10 @@
default:
warning ("Invalid control type in command structure.");
- return invalid_control;
+ break;
}
- if (old_chain)
- do_cleanups (old_chain);
+ do_cleanups (old_chain);
return ret;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: make execute_control_command conform to docs
2004-02-24 23:12 ` Andrew Cagney
@ 2004-02-25 14:02 ` Dave Allan
2004-02-25 15:56 ` Andrew Cagney
0 siblings, 1 reply; 6+ messages in thread
From: Dave Allan @ 2004-02-25 14:02 UTC (permalink / raw)
To: Andrew Cagney; +Cc: GDB patches
That patch looks good to me. I was also able to apply it to the gdb
tree in 'crash' where I originally discovered the problem, and the
segfault there is gone.
Thanks for all the help!
Dave
On Tue, 2004-02-24 at 18:12, Andrew Cagney wrote:
> > I was trying to make the patch as small as possible, but I ought to have
> > done it as you say. Letting everything flow through to the end so that
> > there is only one return point is definitely the cleanest way to do it.
>
> Here, yes, it appears to be the case. Any way, does the attached appear
> to work?
>
> Andrew
>
>
> ______________________________________________________________________
>
> 2004-02-24 Andrew Cagney <cagney@redhat.com>
>
> PR cli/1566. Problem found, and fix suggested by David Allan.
> * cli/cli-script.c (execute_control_command): Unconditionally
> install a cleanup. Default "ret" to "invalid_control". Use
> "break" instead of "return" to escape from the switch.
>
> Index: cli/cli-script.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
> retrieving revision 1.21
> diff -u -r1.21 cli-script.c
> --- cli/cli-script.c 22 Dec 2003 03:43:19 -0000 1.21
> +++ cli/cli-script.c 24 Feb 2004 23:10:13 -0000
> @@ -294,21 +294,25 @@
> {
> struct expression *expr;
> struct command_line *current;
> - struct cleanup *old_chain = 0;
> + struct cleanup *old_chain = make_cleanup (null_cleanup, 0);
> struct value *val;
> struct value *val_mark;
> int loop;
> enum command_control_type ret;
> char *new_line;
>
> + /* Start by assuming failure, if a problem is detected, the code
> + below will simply "break" out of the switch. */
> + ret = invalid_control;
> +
> switch (cmd->control_type)
> {
> case simple_control:
> /* A simple command, execute it and return. */
> new_line = insert_args (cmd->line);
> if (!new_line)
> - return invalid_control;
> - old_chain = make_cleanup (free_current_contents, &new_line);
> + break;
> + make_cleanup (free_current_contents, &new_line);
> execute_command (new_line, 0);
> ret = cmd->control_type;
> break;
> @@ -325,8 +329,8 @@
> /* Parse the loop control expression for the while statement. */
> new_line = insert_args (cmd->line);
> if (!new_line)
> - return invalid_control;
> - old_chain = make_cleanup (free_current_contents, &new_line);
> + break;
> + make_cleanup (free_current_contents, &new_line);
> expr = parse_expression (new_line);
> make_cleanup (free_current_contents, &expr);
>
> @@ -385,8 +389,8 @@
> {
> new_line = insert_args (cmd->line);
> if (!new_line)
> - return invalid_control;
> - old_chain = make_cleanup (free_current_contents, &new_line);
> + break;
> + make_cleanup (free_current_contents, &new_line);
> /* Parse the conditional for the if statement. */
> expr = parse_expression (new_line);
> make_cleanup (free_current_contents, &expr);
> @@ -424,11 +428,10 @@
>
> default:
> warning ("Invalid control type in command structure.");
> - return invalid_control;
> + break;
> }
>
> - if (old_chain)
> - do_cleanups (old_chain);
> + do_cleanups (old_chain);
>
> return ret;
> }
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: make execute_control_command conform to docs
2004-02-25 14:02 ` Dave Allan
@ 2004-02-25 15:56 ` Andrew Cagney
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cagney @ 2004-02-25 15:56 UTC (permalink / raw)
To: da_gdb; +Cc: GDB patches
> That patch looks good to me. I was also able to apply it to the gdb
> tree in 'crash' where I originally discovered the problem, and the
> segfault there is gone.
>
> Thanks for all the help!
Just committing, ...
Andrew
Tue, 2004-02-24 at 18:12, Andrew Cagney wrote:
>
>>>> > I was trying to make the patch as small as possible, but I ought to have
>>>> > done it as you say. Letting everything flow through to the end so that
>>>> > there is only one return point is definitely the cleanest way to do it.
>>
>>>
>>> Here, yes, it appears to be the case. Any way, does the attached appear
>>> to work?
>>>
>>> Andrew
>>>
>>>
>>> ______________________________________________________________________
>>>
>>> 2004-02-24 Andrew Cagney <cagney@redhat.com>
>>>
>>> PR cli/1566. Problem found, and fix suggested by David Allan.
>>> * cli/cli-script.c (execute_control_command): Unconditionally
>>> install a cleanup. Default "ret" to "invalid_control". Use
>>> "break" instead of "return" to escape from the switch.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-02-25 15:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-02-24 16:47 make execute_control_command conform to docs David Allan
2004-02-24 20:36 ` Andrew Cagney
2004-02-24 21:51 ` Dave Allan
2004-02-24 23:12 ` Andrew Cagney
2004-02-25 14:02 ` Dave Allan
2004-02-25 15:56 ` Andrew Cagney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox