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
const gchar* vs gchar *
And now a question to show that I never had a formal c/c++ education.
Are the below functions equivalent ?
void funcA ()
{
gchar *varA = g_strdup (Test);
do something with a
g_free (varA);
}
and
void funcA ()
{
const gchar *varA = g_strdup (Test);
do something with a
}
I'm mostly wondering if the second function would have a memory leak or
not. If varA is defined as a const gchar *, will the program
automatically free the memory allocated with g_strdup ?
I don't expect so, but I'm seeing mixed uses in GnuCash and want to
determine for once and for all what is the proper way to handle this.
Geert
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: const gchar* vs gchar *
They're definitely not equivalent. Specifically:
If varA is defined as a const gchar *, will the program automatically free the
memory allocated with g_strdup ?
No. If you use strdup (or malloc, etc.), you need to free the resulting pointer
afterwards.
const on a pointer just means that you shouldn't modify the memory that the
pointer points to. This is more useful when you use string constants. In C, this
isn't strictly enforced, but in C++ it is. See also
http://publications.gbdirect.co.uk/c_book/chapter8/const_and_volatile.html .
Jethro
On 22-12-12 12:51, Geert Janssens wrote:
And now a question to show that I never had a formal c/c++ education.
Are the below functions equivalent ?
void funcA ()
{
gchar *varA = g_strdup (Test);
do something with a
g_free (varA);
}
and
void funcA ()
{
const gchar *varA = g_strdup (Test);
do something with a
}
I'm mostly wondering if the second function would have a memory leak or not.
If
varA is defined as a const gchar *, will the program automatically free the
memory allocated with g_strdup ?
I don't expect so, but I'm seeing mixed uses in GnuCash and want to determine
for once and for all what is the proper way to handle this.
Geert
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: const gchar* vs gchar *
Hi,
No, they are not equivalent.
The 'const' basically tells the compiler that the object is immutable.
It's used in an argument to promise that the function will not modify the
object. It's used in a return value to say that the caller may not modify
or free the object because the callee will free it later.
So no, your second function will have a memory leak, because g_strdup is
expecting the caller to free the object.
-derek
On Sat, December 22, 2012 6:51 am, Geert Janssens wrote:
And now a question to show that I never had a formal c/c++ education.
Are the below functions equivalent ?
void funcA ()
{
gchar *varA = g_strdup (Test);
do something with a
g_free (varA);
}
and
void funcA ()
{
const gchar *varA = g_strdup (Test);
do something with a
}
I'm mostly wondering if the second function would have a memory leak or
not. If varA is defined as a const gchar *, will the program
automatically free the memory allocated with g_strdup ?
I don't expect so, but I'm seeing mixed uses in GnuCash and want to
determine for once and for all what is the proper way to handle this.
Geert
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
--
Derek Atkins 617-623-3745
de...@ihtfp.com www.ihtfp.com
Computer and Internet Security Consultant
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: const gchar* vs gchar *
In C it is not enforced, but if you have a const pointer the compiler will
complain if you pass it as an argument to a function expecting a non-const
pointer.
-derek
On Sat, December 22, 2012 7:14 am, Jethro Beekman wrote:
They're definitely not equivalent. Specifically:
If varA is defined as a const gchar *, will the program automatically
free the
memory allocated with g_strdup ?
No. If you use strdup (or malloc, etc.), you need to free the resulting
pointer
afterwards.
const on a pointer just means that you shouldn't modify the memory that
the
pointer points to. This is more useful when you use string constants. In
C, this
isn't strictly enforced, but in C++ it is. See also
http://publications.gbdirect.co.uk/c_book/chapter8/const_and_volatile.html
.
Jethro
On 22-12-12 12:51, Geert Janssens wrote:
And now a question to show that I never had a formal c/c++ education.
Are the below functions equivalent ?
void funcA ()
{
gchar *varA = g_strdup (Test);
do something with a
g_free (varA);
}
and
void funcA ()
{
const gchar *varA = g_strdup (Test);
do something with a
}
I'm mostly wondering if the second function would have a memory leak or
not. If
varA is defined as a const gchar *, will the program automatically free
the
memory allocated with g_strdup ?
I don't expect so, but I'm seeing mixed uses in GnuCash and want to
determine
for once and for all what is the proper way to handle this.
Geert
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
--
Derek Atkins 617-623-3745
de...@ihtfp.com www.ihtfp.com
Computer and Internet Security Consultant
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Re: const gchar* vs gchar *
Thanks Jethro and Derek, that confirms my understanding of the matter.
I'll go fix some memory leaks now...
Geert
On 22-12-12 13:19, Derek Atkins wrote:
Hi,
No, they are not equivalent.
The 'const' basically tells the compiler that the object is immutable.
It's used in an argument to promise that the function will not modify the
object. It's used in a return value to say that the caller may not modify
or free the object because the callee will free it later.
So no, your second function will have a memory leak, because g_strdup is
expecting the caller to free the object.
-derek
On Sat, December 22, 2012 6:51 am, Geert Janssens wrote:
And now a question to show that I never had a formal c/c++ education.
Are the below functions equivalent ?
void funcA ()
{
gchar *varA = g_strdup (Test);
do something with a
g_free (varA);
}
and
void funcA ()
{
const gchar *varA = g_strdup (Test);
do something with a
}
I'm mostly wondering if the second function would have a memory leak or
not. If varA is defined as a const gchar *, will the program
automatically free the memory allocated with g_strdup ?
I don't expect so, but I'm seeing mixed uses in GnuCash and want to
determine for once and for all what is the proper way to handle this.
Geert
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
___
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: const gchar* vs gchar *
I think the main source of misunderstanding is that in C, the compiler
functionally doesn't make any difference between a const and a non-const
pointer (except for the warning messages).
This is fundamentally different in C++, where due to argument overloading
different functions might be called for const vs. non-const pointer arguments.
But in C, basically a pointer is a pointer and that's it. There is no special
behaviour for a non-const pointer vs. a const pointer in the C language.
The main advantage of using const pointers in general and writing const-
correct code is in its communication for the other programmers: The
programmers should understand a const pointer as being immutable (i.e. the
pointed-to object shouldn't have its state changed), and continue to use it
this way.
Regards,
Christian
Am Samstag, 22. Dezember 2012, 07:19:55 schrieb Derek Atkins:
Hi,
No, they are not equivalent.
The 'const' basically tells the compiler that the object is immutable.
It's used in an argument to promise that the function will not modify the
object. It's used in a return value to say that the caller may not modify
or free the object because the callee will free it later.
So no, your second function will have a memory leak, because g_strdup is
expecting the caller to free the object.
-derek
On Sat, December 22, 2012 6:51 am, Geert Janssens wrote:
And now a question to show that I never had a formal c/c++ education.
Are the below functions equivalent ?
void funcA ()
{
gchar *varA = g_strdup (Test);
do something with a
g_free (varA);
}
and
void funcA ()
{
const gchar *varA = g_strdup (Test);
do something with a
}
I'm mostly wondering if the second function would have a memory leak or
not. If varA is defined as a const gchar *, will the program
automatically free the memory allocated with g_strdup ?
I don't expect so, but I'm seeing mixed uses in GnuCash and want to
determine for once and for all what is the proper way to handle this.
Geert
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
