r22651 - gnucash/trunk/src - Guile 2: replace deprecated SCM_SYMBOL_CHARS function

[Geert Janssens]

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