[OB] Add cleanup, source.c

[OB] Add cleanup, source.c

[msnyder]

A missed opportunity for cleanup, flagged by Coverity.

I'm cut'and'pasting the patch to avoid my browser turning it into a
binary stream.  This may eliminate tabs, but I don't expect anybody
to actually apply this with 'patch'...

2007-06-28  Michael Snyder  <msnyder@access-company.com>

        * source.c (unset_substitute_path_command): Plug leak (Coverity).

Index: source.c
===================================================================
RCS file: /cvs/src/src/gdb/source.c,v
retrieving revision 1.79
diff -p -r1.79 source.c
*** source.c    24 Jan 2007 00:03:15 -0000      1.79
--- source.c    28 Jun 2007 22:12:36 -0000
*************** unset_substitute_path_command (char *arg
*** 1852,1857 ****
--- 1852,1858 ----

    /* This function takes either 0 or 1 argument.  */

+   make_cleanup_freeargv (argv);
    if (argv != NULL && argv[0] != NULL && argv[1] != NULL)
      error (_("Incorrect usage, too many arguments in command"));

Re: [OB] Add cleanup, source.c

[Daniel Jacobowitz]

On Thu, Jun 28, 2007 at 03:27:43PM -0700, msnyder@sonic.net wrote:
> A missed opportunity for cleanup, flagged by Coverity.
> 
> I'm cut'and'pasting the patch to avoid my browser turning it into a
> binary stream.  This may eliminate tabs, but I don't expect anybody
> to actually apply this with 'patch'...
> 
> 2007-06-28  Michael Snyder  <msnyder@access-company.com>
> 
>         * source.c (unset_substitute_path_command): Plug leak (Coverity).

I think you need to do these a little slower...

>     /* This function takes either 0 or 1 argument.  */
> 
> +   make_cleanup_freeargv (argv);
>     if (argv != NULL && argv[0] != NULL && argv[1] != NULL)
>       error (_("Incorrect usage, too many arguments in command"));

There's no call to do_cleanups in this function, so it's quite hard to
see if this cleanup will be run or discarded if error is not called.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [OB] Add cleanup, source.c

[msnyder]

> There's no call to do_cleanups in this function, so it's quite hard to
> see if this cleanup will be run or discarded if error is not called.

When are they ever discarded?

My last understanding of the mechanism was that the cleanups
always get called, eventually.  Presumably when we return to
the command / event loop.

The only reason to call do_cleanups, in my understanding,
is if you want to force them to be done right now instead
of later.

Re: [OB] Add cleanup, source.c

[Daniel Jacobowitz]

On Thu, Jun 28, 2007 at 03:59:23PM -0700, Michael Snyder wrote:
> 
> 
> > There's no call to do_cleanups in this function, so it's quite hard to
> > see if this cleanup will be run or discarded if error is not called.
> 
> When are they ever discarded?
> 
> My last understanding of the mechanism was that the cleanups
> always get called, eventually.  Presumably when we return to 
> the command / event loop.

No, that's not right.  Cleanups are often discarded after a successful
operation, in order to not free something that would have been cleaned
up on error.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [OB] Add cleanup, source.c

[msnyder]

> On Thu, Jun 28, 2007 at 03:59:23PM -0700, Michael Snyder wrote:
>>
>>
>> > There's no call to do_cleanups in this function, so it's quite hard to
>> > see if this cleanup will be run or discarded if error is not called.
>>
>> When are they ever discarded?
>>
>> My last understanding of the mechanism was that the cleanups
>> always get called, eventually.  Presumably when we return to
>> the command / event loop.
>
> No, that's not right.  Cleanups are often discarded after a successful
> operation, in order to not free something that would have been cleaned
> up on error.

Well, I'm pretty sure you're mistaken.

Or put it another way -- one of us is mistaken, and naturally,
I think it's you.   ;-)

1) From logic: I know that frequently when we allocate something,
we call a make_cleanup function to register it to be freed -- and
then we never actually free it.  We depend on it being freed at
cleanup time.  Which is exactly what we're doing here.

2) Empirically: I run gdb with this change, and set a breakpoint
at do_freeargv.  It gets called, from command_handler (event-top.c).
So the change accomplishes what it is meant to do.

3) By example: in the same module (source.c), three other functions
call buildargv and make_cleanup_freeargv -- and none of them calls
do_cleanups.

So you're holding me to a higher standard than the preexisting code.
;-)

Re: [OB] Add cleanup, source.c

[Daniel Jacobowitz]

On Thu, Jun 28, 2007 at 06:36:38PM -0700, msnyder@sonic.net wrote:
> > No, that's not right.  Cleanups are often discarded after a successful
> > operation, in order to not free something that would have been cleaned
> > up on error.
> 
> Well, I'm pretty sure you're mistaken.

We're both right.  Cleanups do get discarded, and cleanups that aren't
discarded are called at the top level.  Every cleanup I've written
since I started working on GDB at 2001 has been freed locally rather
than at the top level, though.  I think it's very confusing if the
cleanups are not locally paired.

From gdbint.texinfo:

   Your function should explicitly do or discard the cleanups it
creates.  Failing to do this leads to non-deterministic behavior since
the caller will arbitrarily do or discard your functions cleanups.
This need leads to two common cleanup styles.

> 3) By example: in the same module (source.c), three other functions
> call buildargv and make_cleanup_freeargv -- and none of them calls
> do_cleanups.
> 
> So you're holding me to a higher standard than the preexisting code.
> ;-)

True.  There are several functions that do though.  Well, I'll stop
objecting now :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [OB] Add cleanup, source.c

[Joel Brobecker]

> >From gdbint.texinfo:
> 
>    Your function should explicitly do or discard the cleanups it
> creates.  Failing to do this leads to non-deterministic behavior since
> the caller will arbitrarily do or discard your functions cleanups.
> This need leads to two common cleanup styles.

Humpf! I think I have contributed loads of patches that do not
follow this advice. I'll be careful in the future, but sometimes
it's not easy. For instance, when you have a function that has
multiple possible exit points...

-- 
Joel

Re: [OB] Add cleanup, source.c

[Michael Snyder]

> > >From gdbint.texinfo:
> >
> >    Your function should explicitly do or discard the cleanups it
> > creates.  Failing to do this leads to non-deterministic behavior since
> > the caller will arbitrarily do or discard your functions cleanups.
> > This need leads to two common cleanup styles.
>
> Humpf! I think I have contributed loads of patches that do not
> follow this advice. I'll be careful in the future, but sometimes
> it's not easy. For instance, when you have a function that has
> multiple possible exit points...

Yes, I don't feel right about this at all.  When was this paragraph
written?  It took me a long time to figure out how make_cleanup
worked, but it was also a long time ago, and I have been writing
cleanups without explicitly invoking do_cleanup for many many
years now.

This is how I understand the subsystem:

1) Every invocation of make_cleanup (or variants) throws a callback
onto a queue, that is guaranteed to be executed no later than the end
of the command loop cycle, whether due to error or normal return.

2) When you save an "old_cleanups" pointer, eg.
    struct cleanups *old_cleanups = make_cleanup (stuff);
it represents a watermark or bookmark in that queue.  Now there are
two things you can do with that watermark.

  a) discard_cleanups (old_cleanup) discards all the cleanups made
  AFTER that point (including "stuff").
  b) do_cleanups invokes all the cleanups made after that point.

3) But both of those actions have always been optional, and in my
understanding, only needed in special circumstances.  The normal
flow is to do nothing -- and the cleanups will be done by the
command loop, before the next command.

The whole purpose of this was to PREVENT having to look out
for multiple or abnormal exits from a function, so that you didn't
have to make sure you freed stuff before calling error or something.

Adding a requirement to explicitly call do_cleanups would go
against the original design purpose of the subsystem.

Re: [OB] Add cleanup, source.c

[Michael Snyder]

> We're both right.  Cleanups do get discarded, and cleanups that aren't
> discarded are called at the top level.  Every cleanup I've written
> since I started working on GDB at 2001 has been freed locally rather
> than at the top level, though.  I think it's very confusing if the
> cleanups are not locally paired.
>
> From gdbint.texinfo:
>
>    Your function should explicitly do or discard the cleanups it
> creates.  Failing to do this leads to non-deterministic behavior since
> the caller will arbitrarily do or discard your functions cleanups.
> This need leads to two common cleanup styles.

Yeah, that text was written by Andrew in 2003.  Before that we
had some text written by Eli in about 2001, also saying that you
should call do_cleanups.

<ahem>
Don't mean to be a curmudgeon, but I go back way before that.  ;-)

Here's how version 1.1 of the document read:

@section Cleanups

Cleanups are a structured way to deal with things that need to be done
later.  When your code does something (like @code{malloc} some memory,
or open a file) that needs to be undone later (e.g. free the memory or
close the file), it can make a cleanup.  The cleanup will be done at
some future point: when the command is finished, when an error occurs,
or when your code decides it's time to do cleanups.

You can also discard cleanups, that is, throw them away without doing
what they say.  This is only done if you ask that it be done.

Syntax:

@table @code

@item struct cleanup *@var{old_chain};
Declare a variable which will hold a cleanup chain handle.

@item @var{old_chain} = make_cleanup (@var{function}, @var{arg});
Make a cleanup which will cause @var{function} to be called with
@var{arg} (a @code{char *}) later.  The result, @var{old_chain}, is a
handle that can be passed to @code{do_cleanups} or
@code{discard_cleanups} later.  Unless you are going to call
@code{do_cleanups} or @code{discard_cleanups} yourself, you can ignore
the result from @code{make_cleanup}.

Re: [OB] Add cleanup, source.c

[Eli Zaretskii]

> From: "Michael Snyder" <msnyder@sonic.net>
> Cc: "Michael Snyder" <Michael.Snyder@access-company.com>,         <gdb-patches@sourceware.org>
> Date: Fri, 29 Jun 2007 13:34:05 -0700
> 
> > We're both right.  Cleanups do get discarded, and cleanups that aren't
> > discarded are called at the top level.  Every cleanup I've written
> > since I started working on GDB at 2001 has been freed locally rather
> > than at the top level, though.  I think it's very confusing if the
> > cleanups are not locally paired.
> >
> > From gdbint.texinfo:
> >
> >    Your function should explicitly do or discard the cleanups it
> > creates.  Failing to do this leads to non-deterministic behavior since
> > the caller will arbitrarily do or discard your functions cleanups.
> > This need leads to two common cleanup styles.
> 
> Yeah, that text was written by Andrew in 2003.  Before that we
> had some text written by Eli in about 2001, also saying that you
> should call do_cleanups.
> 
> <ahem>
> Don't mean to be a curmudgeon, but I go back way before that.  ;-)
> 
> Here's how version 1.1 of the document read:

Sorry, I'm confused: where exactly is the current gdbint.texinfo text
wrong?

Are you saying that the non-deterministic behavior mentioned in the
manual is a red herring?  But the explanation it provides (the fact
that some other function further on could discard_cleanups) seems to
provide valid basis for that, doesn't it?

Re: [OB] Add cleanup, source.c

[Michael Snyder]

> > > From gdbint.texinfo:
> > >
> > >    Your function should explicitly do or discard the cleanups it
> > > creates.  Failing to do this leads to non-deterministic behavior since
> > > the caller will arbitrarily do or discard your functions cleanups.
> > > This need leads to two common cleanup styles.
> >
> > Yeah, that text was written by Andrew in 2003.  Before that we
> > had some text written by Eli in about 2001, also saying that you
> > should call do_cleanups.
> >
> > <ahem>
> > Don't mean to be a curmudgeon, but I go back way before that.  ;-)
> >
> > Here's how version 1.1 of the document read:
>
> Sorry, I'm confused: where exactly is the current gdbint.texinfo text
> wrong?
>
> Are you saying that the non-deterministic behavior mentioned in the
> manual is a red herring?  But the explanation it provides (the fact
> that some other function further on could discard_cleanups) seems to
> provide valid basis for that, doesn't it?

Depends on what the "cleanups" are, I suppose.  Those I'm familiar
with are mostly xfrees, fcloses, etc.  Doesn't really matter what
order they are performed in, so long as they get done before the
next command loop cycle.

Can you give me an example of such non-deterministic behavior?

Re: [OB] Add cleanup, source.c

[Daniel Jacobowitz]

On Sat, Jun 30, 2007 at 09:00:56AM -0700, Michael Snyder wrote:
> Depends on what the "cleanups" are, I suppose.  Those I'm familiar
> with are mostly xfrees, fcloses, etc.  Doesn't really matter what
> order they are performed in, so long as they get done before the
> next command loop cycle.
> 
> Can you give me an example of such non-deterministic behavior?

If you're called from any function that discards cleanups, your
cleanups will be lost.  There's about 50...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [OB] Add cleanup, source.c

[Eli Zaretskii]

> From: "Michael Snyder" <msnyder@sonic.net>
> Cc: <drow@false.org>, <Michael.Snyder@access-company.com>,
>         <gdb-patches@sourceware.org>
> Date: Sat, 30 Jun 2007 09:00:56 -0700
> 
> Depends on what the "cleanups" are, I suppose.  Those I'm familiar
> with are mostly xfrees, fcloses, etc.  Doesn't really matter what
> order they are performed in, so long as they get done before the
> next command loop cycle.

Yes, but discard_cleanups throws them away without performing them.  I
don't believe you won't care about that.

> Can you give me an example of such non-deterministic behavior?

What for?  Isn't it obvious, that, given the user-driven workings of
GDB, you can never know when will the next discard_cleanups call
happen?

Re: [OB] Add cleanup, source.c

[Jim Blandy]

What's going on here is that GDB uses one construct, cleanups, to
implement two different useful behaviors, and does so in a perilous
way.

In Java, you can write:

    try {
        ...;
    } catch {
        ...;
    }

Or:

    try {
        ...;
    } finally {
        ...;
    }

The 'catch' block gets executed if the 'try' block throws an
exception.  The 'finally' block gets executed no matter whether the
try block terminates normally or throws an exception.

In GDB, when we push cleanups that we intend to discard on success,
those are playing the role of a 'catch' block.  When we push cleanups
that we're counting on being called even if things go wrong, then
they're playing the role of a 'finally' block.  They're both useful
features, and they're definitely not the same feature, because
'finally' blocks get called in more cases.

The problem with GDB is that we decide how to treat the blocks when we
clean up, not when we register.  So here's the case we've been talking
about:

    foo ()
    {
      make_cleanup (a 'finally' action)
      do stuff which may throw an error
      don't call do_cleanups, on at least one path out of the function
    }

    bar ()
    {
      make_cleanup (a 'catch' action)
      foo ()
      discard_cleanups, since we've succeeded and we don't want our catch
      action to run
    }

Here, if no errors occur, foo's finally action won't get run because
bar's discard_cleanups call applies more broadly than the author of
'bar' intended.

What GDB should do instead is this:

    foo ()
    {
      finally_handler (a 'finally' action)
      do stuff which may throw an error
      don't call do_cleanups, at least one one path out
    }

    bar ()
    {
      failure_handler (a 'failure' action)
      foo ()
      cleanup_on_success ()
    }

where 'cleanup_success' discards failure handlers and runs finally
handlers.  The 'error' function would call a companion to that named
'cleanup_on_failure', which runs both finally and failure handlers.

So.  Only 270 calls to 'do_cleanups' and 47 calls to
'discard_cleanups' to examine.

Re: [OB] Add cleanup, source.c

[Joel Brobecker]

> What's going on here is that GDB uses one construct, cleanups, to
> implement two different useful behaviors, and does so in a perilous
> way.
> where 'cleanup_success' discards failure handlers and runs finally
> handlers.  The 'error' function would call a companion to that named
> 'cleanup_on_failure', which runs both finally and failure handlers.

A very interesting analysis, Jim. I confess that up to today, I have
been using the two concepts interchangeably, so I'm pretty sure that
I have introduced the potential for memory leaks :-(.

> So.  Only 270 calls to 'do_cleanups' and 47 calls to
> 'discard_cleanups' to examine.

Good luck. Let me know if you'd like some help, I feel partly responsible
for the mess. We can split the number of files that need to be inspected.

-- 
Joel