Re: r22651 - gnucash/trunk/src - Guile 2: replace deprecated SCM_SYMBOL_CHARS function
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
Re: r22651 - gnucash/trunk/src - Guile 2: replace deprecated SCM_SYMBOL_CHARS function
And just to complete my explanation, you are in fact using the scm_dynwind_* functions slightly differently from their intended use. In the example of gnc_scm_to_locale_string the net result is the same, but in locations where it matters you won't get the desired memory leak protection effect. A minimal code snippet: str = scm_to_locale_string(scm_string); scm_dynwind_begin (0); call_other_scm_function; /* potentially non-local exiting function */ scm_dynwind_free (str); /* wrong: too late */ scm_dynwind_end (); What happens here is that call_other_scm_function may exit non-locally (for example because it triggers the error catch code internally). If that happens, scm_dynwind_free is never reached and str won't be freed. scm_dynwind_free is not actually freeing memory itself, but it tells guile to free the memory whenever the dynwind context is left (locally by reaching scm_dynwind_end or non-locally). So this function should be called *before* calling any function that might exit non-locally. The proper invocation would be: str = scm_to_locale_string(scm_string); scm_dynwind_begin (0); scm_dynwind_free (str); /* ok */ call_other_scm_function; /* potentially non-local exiting function */ scm_dynwind_end (); In this case, str is first marked for freeing (but not freed yet !) whenever the context ends and only then the potentially non-local exiting function is called. In this case str is guaranteed to be freed: if other_scm_function succeeds, str will be freed once scm_dynwind_end is reached. If it fails the non-local exit is detected internally and str is freed then. As said before, for gnc_scm_to_locale_string this didn't matter really, because g_strdup is never exiting non-locally from guile's point of view. I hope this helps clearing it up. Geert ___ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: r22651 - gnucash/trunk/src - Guile 2: replace deprecated SCM_SYMBOL_CHARS function
On 22-12-12 10:38, Geert Janssens wrote: 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. I have readded a gnc_scm_to_locale_string functino and evaluated each use of scm_to_locale_string for possible memory leaks. I think they should all be properly handled now. In most cases gnc_scm_to_locale_string was the proper replacement. In the very few cases where it made sense, I kept the original scm_to_locale_string accompanied by the proper scm_dynwind_* calls. I also took the opportunity to improve some other guile convenience functions. But this work is incomplete. I may add to it as I encounter other cases where the wrapper functions make sense. Geert ___ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: r22651 - gnucash/trunk/src - Guile 2: replace deprecated SCM_SYMBOL_CHARS function
Geert, On Sat, Dec 15, 2012 at 9:58 AM, Geert Janssens gjanss...@code.gnucash.org wrote: Author: gjanssens Date: 2012-12-15 12:58:40 -0500 (Sat, 15 Dec 2012) New Revision: 22651 Trac: http://svn.gnucash.org/trac/changeset/22651 Added: gnucash/trunk/src/core-utils/gnc-guile-utils.c gnucash/trunk/src/core-utils/gnc-guile-utils.h Modified: gnucash/trunk/src/app-utils/guile-util.c gnucash/trunk/src/app-utils/guile-util.h gnucash/trunk/src/app-utils/option-util.c gnucash/trunk/src/core-utils/Makefile.am gnucash/trunk/src/engine/engine-helpers.c gnucash/trunk/src/gnome/dialog-tax-info.c gnucash/trunk/src/import-export/qif-import/assistant-qif-import.c Log: Guile 2: replace deprecated SCM_SYMBOL_CHARS function The replacements require guile 1.8 or above ___ gnucash-patches mailing list gnucash-patc...@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-patches 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? 2. has something changed between then and now that make this no longer an issue and therefore the code no longer necessary? 3. does moving from guile 1.2 to guile 2 affect this in some way? Regards, Alex ___ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
