gnupic: Thread: Re: [gnupic] gpasm memory leaks

A status update on the memory leaks for those interested: I've fixed a few 
and rearranged a few of them.  Two of them were just careless mistakes, but 
several will take some serious rearranging, and for those I looked at today, 
I just moved the responsibility for allocation up a stack frame or two 
(closer to the frame that should be responsible for deallocation) and added 
comments to point out those memory leaks.  I know that sounds 
confusing...basically I made the caller pass a pointer to allocated memory 
down instead of the callee returning the allocated block.  I had a real 
headache when I finished with all the changes and learned that C doesn't 
directly support pass-by-reference =(.  To actually fix those leaks, I'll 
need to do some more involved rearranging, but I'm trying to focus on one 
change at a time so I don't forget something major.  BTW, for those 
experienced with fixing memory leaks in C, is the technique of passing a 
reference/pointer down instead of returning one up a common fix?  How does 
that work for string and array functions where the callee determines the 
length dynamically?

Also, it's been a while since I've used CVS, so I have a stupid question: Is 
there an equivalent to "SVN diff -u" that will diff all files under version 
control that changed in a directory or its subdirectories?  I can only find 
very roundabout ways of doing so, and it makes it very hard for me to keep 
up with the changes I've made.  I don't know how I used to manage without 
SVN...

David Barnett

----- Original Message ----- 
From: "David Barnett" ####@####.####
To: ####@####.####
Sent: Tuesday, May 29, 2007 3:32 PM
Subject: [gnupic] gpasm memory leaks


I noticed some suspicious allocation code in gpasm, so I checked it with 
valgrind.  It seems to report several memory leaks in various places.  I'll 
look into them further as I have time.  Did anyone already notice these?  I 
didn't see any bugs about memory leaks in the bug tracker...should I post 
some?

David Barnett 

Hi David,

> BTW, for those
> experienced with fixing memory leaks in C, is the technique of passing a
> reference/pointer down instead of returning one up a common fix?  How does
> that work for string and array functions where the callee determines the
> length dynamically?

C is a bastard at handling heap correctly, basically the concept is that the
programmer know what is he doing. The three common mistake about allocating
and freeing memory using C is that 1. a pointer holds a previously allocated
memory address, and the programmer doesn't check it before allocate another
one so basically loosing the reference for the first one. 2. the programmer
does not take care about freeing allocated spaces and it's because of a
badly structured program flow. 3. keeping more than one reference to the
same allocated space, then deallocating the space or reallocating so that
one or more references to that memory space are not up-to-date, then
something allocates a memory which goes to the same address, and then
referencing or even deallocating that memory block using those out-dated
references. Again, badly structured code.

Usually the best approach is to always use initialised pointers (with NULL
of course) and have an own version of memory allocation function set. By
using this own malloc/realloc/free set you can put some error checking as
well, like for example in myMalloc(void *ptr, int size) if the ptr is not
NULL then it calls realloc instead of malloc or give a nice error message or
whatever best fits on your needs. You can use double references so that a
reference to the pointer itself so that you cannot make any mistakes in the
features like myMalloc(void **ptr, int size). With myFree(void **ptr) you
always clear (put NULL to) the pointer - again, double reference, so no
mistake can be done by not to clearing the pointer.

With the dynamic memory allocation, well, as long as you have the original
pointer you are fine, you do not have to worry about if you were using
malloc/calloc/realloc or whatever just call free and it will know the size
of the allocated memory block. The only exception is when you have a serious
buffer overflow or other memory corruption in your code, but a tool like
valgrind will show this one up hopefully as well.

Tamas



On 5/31/07, David Barnett ####@####.#### wrote:
>
> A status update on the memory leaks for those interested: I've fixed a few
> and rearranged a few of them.  Two of them were just careless mistakes,
> but
> several will take some serious rearranging, and for those I looked at
> today,
> I just moved the responsibility for allocation up a stack frame or two
> (closer to the frame that should be responsible for deallocation) and
> added
> comments to point out those memory leaks.  I know that sounds
> confusing...basically I made the caller pass a pointer to allocated memory
> down instead of the callee returning the allocated block.  I had a real
> headache when I finished with all the changes and learned that C doesn't
> directly support pass-by-reference =(.  To actually fix those leaks, I'll
> need to do some more involved rearranging, but I'm trying to focus on one
> change at a time so I don't forget something major.  BTW, for those
> experienced with fixing memory leaks in C, is the technique of passing a
> reference/pointer down instead of returning one up a common fix?  How does
> that work for string and array functions where the callee determines the
> length dynamically?
>
> Also, it's been a while since I've used CVS, so I have a stupid question:
> Is
> there an equivalent to "SVN diff -u" that will diff all files under
> version
> control that changed in a directory or its subdirectories?  I can only
> find
> very roundabout ways of doing so, and it makes it very hard for me to keep
> up with the changes I've made.  I don't know how I used to manage without
> SVN...
>
> David Barnett
>
> ----- Original Message -----
> From: "David Barnett" ####@####.####
> To: ####@####.####
> Sent: Tuesday, May 29, 2007 3:32 PM
> Subject: [gnupic] gpasm memory leaks
>
>
> I noticed some suspicious allocation code in gpasm, so I checked it with
> valgrind.  It seems to report several memory leaks in various
> places.  I'll
> look into them further as I have time.  Did anyone already notice
> these?  I
> didn't see any bugs about memory leaks in the bug tracker...should I post
> some?
>
> David Barnett
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: ####@####.####
> For additional commands, e-mail: ####@####.####
>
>

On Thu May 31 2007 15:32, David Barnett wrote:
>  I had a real
> headache when I finished with all the changes and learned that C doesn't
> directly support pass-by-reference =(. 
>
    calledFunction(&referencedData);

Did I misunderstand your statement?

Jeff

On Thu, 31 May 2007 18:24:48 -0700
Jeff ####@####.#### wrote:

> On Thu May 31 2007 15:32, David Barnett wrote:
> >  I had a real
> > headache when I finished with all the changes and learned that C
> > doesn't directly support pass-by-reference =(. 
> >
>     calledFunction(&referencedData);
> 
> Did I misunderstand your statement?
Yep. I mean declare the parameter type (in the function prototype) to be
a reference instead of a pointer. You can get almost the same effect
the way you have above, but if you switch from one to the other, you
have to go through and add or remove a '*' or '&' on each access of
that parameter (or worse, hunt down '.' and '->'). It's not really
worth all this discussion, it was just a gripe that it sucked and also
I was surprised I didn't already know.

David Barnett

Sorry, then I misunderstood the question completely too, please ignore that
mail.

reference is a C++ stuff, the original C does not know anything like that.
You can use the fine preprocessor for avoiding using the -> markings like
this:

void foo(sth_t *something)
{
#define something (*something)
    printf("see: %s, %u\n", something.text, something.num);
#undef something
}

however, I would not recommend it.

Tamas


On 6/1/07, David ####@####.#### wrote:
>
> On Thu, 31 May 2007 18:24:48 -0700
> Jeff ####@####.#### wrote:
>
> > On Thu May 31 2007 15:32, David Barnett wrote:
> > >  I had a real
> > > headache when I finished with all the changes and learned that C
> > > doesn't directly support pass-by-reference =(.
> > >
> >     calledFunction(&referencedData);
> >
> > Did I misunderstand your statement?
> Yep. I mean declare the parameter type (in the function prototype) to be
> a reference instead of a pointer. You can get almost the same effect
> the way you have above, but if you switch from one to the other, you
> have to go through and add or remove a '*' or '&' on each access of
> that parameter (or worse, hunt down '.' and '->'). It's not really
> worth all this discussion, it was just a gripe that it sucked and also
> I was surprised I didn't already know.
>
> David Barnett
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: ####@####.####
> For additional commands, e-mail: ####@####.####
>
>

On Thu, 31 May 2007 17:32:02 -0500 David wrote:
> BTW, for those 
> experienced with fixing memory leaks in C, is the technique of passing a 
> reference/pointer down instead of returning one up a common fix? 

It's not really a fix, it just allows a little more flexibility for the caller. Actually, a lot of the standard C library functions do it the other way - for example, strdup calls malloc internally and expects the programmer to call free on the returned string. So it would probably be fair to answer "no" to your question.

As to your CVS question, my version of SVN rejects "svn diff -u". I believe the CVS equivalent of "svn diff -x -u" is "cvs diff -u".

Cheers,
Rob

----- Original Message ----- 
From: "Robert Pearce" ####@####.####
To: ####@####.####
Sent: Friday, June 01, 2007 2:17 AM
Subject: Re: [gnupic] gpasm memory leaks


> On Thu, 31 May 2007 17:32:02 -0500 David wrote:
>> BTW, for those
>> experienced with fixing memory leaks in C, is the technique of passing a
>> reference/pointer down instead of returning one up a common fix?
>
> It's not really a fix, it just allows a little more flexibility for the 
> caller. Actually, a lot of the standard C library functions do it the 
> other way - for example, strdup calls malloc internally and expects the 
> programmer to call free on the returned string. So it would probably be 
> fair to answer "no" to your question.
Sorry, I didn't mean that as the whole fix.  What I meant was in most 
languages you try to allocate and deallocate on the same stack level, 
because it can be much harder to keep them paired otherwise.  That also 
allows you to statically allocate some variables you couldn't otherwise.  I 
guess it's most important in C++ where the logic can be spread around much 
more.  From that angle I consider strdup and other obvious allocation 
functions in the same group as malloc.

Anyway, I was really asking whether doing so would open up new problems, and 
whether that reorganization would be considered more trouble than it's 
worth.  I thought it worth asking because I haven't spent enough time on C 
projects that I know all the tricks and conventions, and I didn't want to 
set to work on changes that would end up being useless or harmful.  So, 
would you usually recommend leaving the allocation where it is and working 
around it when there are leaks?  If you want to see what I'm looking at and 
you have the source, it's all the calls to the push_symbol_table function in 
libgputils/gpsymbol.c.  I don't think the code even attempts to free them.

> As to your CVS question, my version of SVN rejects "svn diff -u".
So it does...  I didn't really have an SVN checkout handy to try it on.
> I believe the CVS equivalent of "svn diff -x -u" is "cvs diff -u".
Yeah, looks like it is.  I thought I remembered some complication to it, but 
now I think about it I think it's reverting changes that's a pain.  I always 
have to delete the file and update it again...can't find any revert, and 
'unedit' doesn't seem to do exactly the same thing.  Well, sorry for the 
bother, anyway.  I try to get my facts straight, but sometimes there really 
are stupid questions =).  I know I've had a low signal-to-noise here so far.

David Barnett 

On the subject of where to call malloc,
On Fri, 1 Jun 2007 09:21:52 -0500 David wrote:
> Anyway, I was really asking whether doing so would open up new problems, and 
> whether that reorganization would be considered more trouble than it's 
> worth.

I'd probably consider it more trouble than I could be bothered with ;)
But I doubt it would open any particular problems unless there are
hidden assumptions in the structure of the code. That, however, is an
application design issue, not a language one.

>  So, 
> would you usually recommend leaving the allocation where it is and working 
> around it when there are leaks?  If you want to see what I'm looking at and 
> you have the source, it's all the calls to the push_symbol_table function in 
> libgputils/gpsymbol.c.  I don't think the code even attempts to free them.

I'd normally recommend leaving the existing design decisions unless
they're clearly broken. I'm not sure the function name
"push_symbol_table" strikes me as suggestive of being in the class of
functions like malloc, but what it does strikes me as clearly belonging
in that class. It looks like it really ought to be called
"symbol_table_new", and its companion "pop_symbol_table" would then be
"symbol_table_delete". If that ties up with their usage (which I
haven't confirmed) then the "pop" function ought to free the deleted
object. If the usage breaks catastrophically with that change, then the
design would seem to be broken.

Does that help at all?


Rob

Another status update: I have several changes to submit.  I separated them 
into 3 patches to make it less confusing to review and accept/reject 
changes.  There were 3 groups of changes so far.

A few comments on each, in order of complexity:
 * leaks1.patch -- the 2 small oversights.  1 call to free() added for each
 * leaks3.patch -- free some strings when the parser finishes with them. 
I'm not actually sure why they need to be allocated in the first place, 
since they're only used as a selector in one function call and the parser 
should be able to pass string literals instead (maybe it's in case the lexer 
rules change?).
 * leaks2a.patch -- the reorganization/push_symbol_table()-related changes. 
There are 3 pointers that I changed to be statically allocated and removed 
the memory leak (archive_tbl, definition_tbl, symbol_index), and one subtle 
leak fixed in the 3rd chunk (no malloc before the call).  The other changes 
are preparation for those changes and several to come.  For the preparation 
changes, the only real change to logic is that the call to calloc has been 
split into a malloc or static allocation outside push_s_t() and a memset 
inside.  Next I plan to change the symbol table stacks (state.stBuiltin, 
etc.) to be mostly statically allocated (as members of the global "state" 
struct).  There are some complications in that some of the stacks will still 
be pointers to others, but still I think static allocation should be enough.

Notice that all of the extra malloc's in the last patch are just moved out 
of push_symbol_table() and commented.  It looks like valgrind treats them 
differently now even though they're still leaks.  Only a few memory leaks 
were fixed, but there shouldn't be any new leaks.

Also, these changes affect several gputils tools, but for now I'm focusing 
on gpasm.

The changes passed regression tests as far as I understood, but it's still 
quite possible I'm wrecking things =), so I'd encourage someone to try to 
verify the changes before applying them to CVS.

David Barnett

----- Original Message ----- 
From: "David Barnett" ####@####.####
To: ####@####.####
Sent: Thursday, May 31, 2007 5:32 PM
Subject: Re: [gnupic] gpasm memory leaks


>A status update on the memory leaks for those interested: I've fixed a few 
>and rearranged a few of them.  Two of them were just careless mistakes, but 
>several will take some serious rearranging...
> <snip>
>
> David Barnett
>
> ----- Original Message ----- 
> From: "David Barnett" ####@####.####
> To: ####@####.####
> Sent: Tuesday, May 29, 2007 3:32 PM
> Subject: [gnupic] gpasm memory leaks
>
>
> I noticed some suspicious allocation code in gpasm, so I checked it with 
> valgrind.  It seems to report several memory leaks in various places. 
> I'll look into them further as I have time.  Did anyone already notice 
> these?  I didn't see any bugs about memory leaks in the bug 
> tracker...should I post some?
>
> David Barnett 

----- Original Message ----- 
From: "Robert Pearce" ####@####.####
To: ####@####.####
Sent: Friday, June 01, 2007 12:40 PM
Subject: Re: [gnupic] gpasm memory leaks


> On the subject of where to call malloc,
> On Fri, 1 Jun 2007 09:21:52 -0500 David wrote:
<snip>
> If that ties up with their usage (which I
> haven't confirmed) then the "pop" function ought to free the deleted
> object. If the usage breaks catastrophically with that change, then the
> design would seem to be broken.
The way the rest of the code uses pop_symbol_table, it seems like it's 
supposed to somehow free the block of memory, but it doesn't.  I say it 
seems like it's "supposed to" because of lines like this:
  /* destory the table */
  file_table = pop_symbol_table(file_table);
Moving the free() into pop_symbol_table() would be the alternative to what I 
was working on.  As it is, just adding free() into the right place in 
pop_symbol_table() seems to make the problem worse somehow.

Speaking of the naming scheme, I thought of an oddity in the behavior I 
changed to.  Whereas before it created a new empty symbol table and pushed 
it onto the stack, now it takes an existing symbol table, empties it, and 
pushes it onto the stack.  It might be better to have create and/or 
initialize functions and have push just push.

David Barnett