Hi Alex,
On 21-12-12 20:06, Alex Aycinena wrote:
A couple of years ago, I used valgrind to find and correct memory
leaks in some code I had previously committed. In that process I
discovered that some of my leaks were caused by
'scm_to_locale_string'.
I investigated on the internet and found that to prevent memory leaks
in scm_to_locale_string() per the guile manual (see
'http://www.gnu.org/software/guile/manual/html_node/Dynamic-Wind.html#Dynamic-Wind),
you needed to surround scm_to_locale_string() with calls to
scm_dynwind_begin (0) and scm_dynwind_free (str) followed by
scm_dynwind_end ().
So I added the code that you have removed with this commit. I also
added it in many more places, including in code that I hadn't
committed, to clear up more memory leaks. Later, I realized that I
should refactor my code to replace all the instances that I had put it
in with a call to gnc_scm_to_locale_string. I haven't got around to
that last part yet but it is on my to do list.
So my questions to you are:
1. were you aware of the memory leak issue with gnc_scm_to_locale_string?
I remember those commits very well. I was excited about your valgrind
work (which even today still feels like dark magic to me). I didn't
really understand the details of the dynwind constructs, but trusted
they fixed the memory leaks, which I still do.
2. has something changed between then and now that make this no longer
an issue and therefore the code no longer necessary?
Nothing has changed, except that I now believe the dynwind code has
never really be necessary. My work to make GnuCash guile 2 compatible
forced me in many ways to get a deeper understanding of how guile and c
interact. As part of this, I also had to revisit the dynwind construct,
what it does and when/why we should use it. This lead me to a slightly
different understanding. From the manual:
"For Scheme code, the fundamental procedure to react to non-local entry
and exits of dynamic contexts is |dynamic-wind."
|The key part in this sentence is "non-local entry and exits". dynwind
is meant to wrap function calls that may not return to the place where
they are called. Guile comes internally with an error handler that can
make this happen for example. In the particular case of
scm_to_locale_string, this function allocates memory to a variable. If a
subsequent guile function is called that triggers guile's internal error
handler, the call to g_free that follows is never reached. Hence the
memory leak.
Does that mean that every call to scm_to_locale_string must be wrapped
with scn_dynwind_begin and scm_dynwind_end ? In my opinion: no. Just
look at the example in the manual you refer to: the first call to
scm_to_locale_string isn't wrapped. It's the subsequent call and the
call to scm_memory_error that are wrapped, because either of these
function can trigger a non-local exit preventing the normal code flow
from freeing the first assigned variable.
The same goes for our own gnc_scm_to_locale_string function :
gchar *gnc_scm_to_locale_string(SCM scm_string)
{
gchar* s;
char * str;
scm_dynwind_begin (0);
str = scm_to_locale_string(scm_string);
s = g_strdup(str);
scm_dynwind_free (str);
scm_dynwind_end ();
return s;
}
scm_to_locale_string doesn't have to be wrapped itself, because if it
fails, str isn't assigned yet and can't be a memory leak. So there's
only one function left that is wrapped: g_strdup(str). This function
won't ever cause a non-local exit for guile: either it succeeds or it
brings the whole application down because of memory issues. In the first
case, the code proceeds normally and str can be freed normally. In the
second case, well, a memory leak is not an issue anymore.
To conclude: as I understand it, the wrapping is not wrong, but adds
unneeded overhead for our use case.
BUT... While writing all this, I noticed I glossed over a subtle memory
issue nonetheless that I have to fix again: scm_to_locale_string uses
malloc to allocate memory for the return value. The memory should be
freed using free. However gnucash is based on glib and hence mostly
deopends g_malloc to allocate memory. This memory should be freed using
g_free. That was probably the main reason scm_to_locale_string was
wrapped in the first place, which I didn't realize.
I'll readd the function gnc_scm_to_locale_string (in a simplified form)
shortly to correct this. So once again: thanks for point this out.
||
3. does moving from guile 1.2 to guile 2 affect this in some way?
No
Geert
_______________________________________________
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
