[<<] [<] Page 1 of 2 [>] [>>] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: "David Barnett" ####@####.#### Date: 31 May 2007 23:36:45 +0100 Message-Id: <013f01c7a3d3$8365c3e0$0301a8c0@barnett2> 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: "Tamas Rudnai" ####@####.#### Date: 1 Jun 2007 00:14:34 +0100 Message-Id: <492f1420705311614n4526d911kfa7ade01aa6d71c8@mail.gmail.com> 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: ####@####.#### > > | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: Jeff ####@####.#### Date: 1 Jun 2007 02:38:10 +0100 Message-Id: <200705311824.48266.j_post@pacbell.net> 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: David ####@####.#### Date: 1 Jun 2007 03:18:21 +0100 Message-Id: <20070531211220.2a683ce9@DEEPTHOUGHT.BARNET.net> 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: "Tamas Rudnai" ####@####.#### Date: 1 Jun 2007 06:30:35 +0100 Message-Id: <492f1420705312230xfaaeb49pfe928acf5a2eaf7e@mail.gmail.com> 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: ####@####.#### > > | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: Robert Pearce ####@####.#### Date: 1 Jun 2007 08:17:29 +0100 Message-Id: <20070601081725.a340f22d.rob@bdt-home.demon.co.uk> 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: "David Barnett" ####@####.#### Date: 1 Jun 2007 15:26:34 +0100 Message-Id: <014601c7a458$33640a20$0301a8c0@barnett2> ----- 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: Robert Pearce ####@####.#### Date: 1 Jun 2007 18:40:35 +0100 Message-Id: <20070601184032.41d03449.rob@bdt-home.demon.co.uk> 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: "David Barnett" ####@####.#### Date: 1 Jun 2007 18:44:08 +0100 Message-Id: <017801c7a473$cf81e7e0$0301a8c0@barnett2> 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 [Content type application/octet-stream not shown. Download] [Content type application/octet-stream not shown. Download] [Content type application/octet-stream not shown. Download] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Subject:
Re: [gnupic] gpasm memory leaks
From: "David Barnett" ####@####.#### Date: 1 Jun 2007 19:29:57 +0100 Message-Id: <018f01c7a47a$31cef2c0$0301a8c0@barnett2> ----- 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[<<] [<] Page 1 of 2 [>] [>>] |