Hi there, After some hard code revisions, I decided to "eat" Valgrind's manual, install it and BINGO!: <q> ==6797== ==6797== Invalid memory access of size 4 ==6797== at 0x80662B3: a_Selection_button_press (selection.c:142) ==6797== by 0x805EB8F: Dw_page_send_selection_event (dw_page.c:1486) ==6797== by 0x805EE47: Dw_page_button_press (dw_page.c:1499) ==6797== by 0x806474D: Dw_widget_mouse_event (dw_widget.c:740) ==6797== Address 0x40D714F0 is 0 bytes inside a block of size 52 free'd ==6797== at 0x40148050: free (in /usr/local/lib/valgrind/valgrind.so) ==6797== by 0x403C1FE8: g_free (in /usr/lib/libglib-1.2.so.0.0.10) ==6797== by 0x8057CA1: a_Dw_ext_iterator_new (dw_ext_iterator.c:171) ==6797== by 0x8066286: a_Selection_button_press (selection.c:136) </q> What does this mean? As a_Dw_ext_iterator_new(it) may _free_ 'it', 'it' should no longer be used after that call. That was the case with the page (right-click test case). I commited a short _code_change_ that avoids the problem. It is not a "patch" but just illustrative code. I believe the functions that may have side effects on their arguments should use an API that somehow reflects that. For instance: DwExtIterator* a_Dw_ext_iterator_new (DwIterator *it); and be used like: a_Dw_ext_iterator_new(&it); so 'it' can be NULL at return time. The same applies for a_Dw_iterator_free() and friends. BTW, the first approach I wrote was to force a_Dw_iterator_free to make NULL its argument). Well, that's it. As I don't know the iterators code as well as Sebastian, I'll let him make the decisions. Hope this helps Happy :) Jorge.-
On Fri, Apr 25, Jorge Arellano Cid wrote:
After some hard code revisions, I decided to "eat" Valgrind's manual, install it and BINGO!:
[...]
What does this mean?
As a_Dw_ext_iterator_new(it) may _free_ 'it', 'it' should no longer be used after that call. That was the case with the page (right-click test case).
I commited a short _code_change_ that avoids the problem. It is not a "patch" but just illustrative code.
I believe the functions that may have side effects on their arguments should use an API that somehow reflects that. For instance:
DwExtIterator* a_Dw_ext_iterator_new (DwIterator *it);
Should be DwExtIterator* a_Dw_ext_iterator_new (DwIterator **it); ^^
and be used like:
a_Dw_ext_iterator_new(&it);
so 'it' can be NULL at return time.
Another possibility would be to define wrapper macros: #define a_Dw_ext_iterator_new(it) _a_Dw_ext_iterator_new(&it) DwExtIterator* _a_Dw_ext_iterator_new (DwIterator **it); but that's a bit kludgy for my taste.
The same applies for a_Dw_iterator_free() and friends.
It should be quite obvious that functions ending on "_free", "_destroy" etc. have a side-effect of their argument, so I'd keep this and regard it as an exception. a_Dw_ext_iterator_new is another case, and very confusing, so this one should be fixed.
BTW, the first approach I wrote was to force a_Dw_iterator_free to make NULL its argument).
Very useful for debugging! :-)
Well, that's it. As I don't know the iterators code as well as Sebastian, I'll let him make the decisions.
Well, first I'd use this change for the release, and fix it for 0.7.3. A grep for "a_Dw_ext_iterator_new" shows that this is handled correctly elsewhere. Perhaps it is cleaner to remove the side-effect completely, by letting a_Dw_ext_iterator_new putting a clone on top of the stack, so that the callers must free the simple iterator. This would be a bit slower, but compared to that, a_Dw_ext_iterator_new creates anyway quite a couple of DwIterator's. Has to be tested. (Thorben: As far as I can say from a first gprof run, this is not the critical code, so the problem is not made worse.) Sebastian
Hi there,
[...] I believe the functions that may have side effects on their arguments should use an API that somehow reflects that. For instance:
DwExtIterator* a_Dw_ext_iterator_new (DwIterator *it);
Should be
DwExtIterator* a_Dw_ext_iterator_new (DwIterator **it); ^^
Yeah, just a typo.
Another possibility would be to define wrapper macros:
#define a_Dw_ext_iterator_new(it) _a_Dw_ext_iterator_new(&it)
DwExtIterator* _a_Dw_ext_iterator_new (DwIterator **it);
but that's a bit kludgy for my taste.
Yep.
The same applies for a_Dw_iterator_free() and friends.
It should be quite obvious that functions ending on "_free", "_destroy" etc. have a side-effect of their argument, so I'd keep this and regard it as an exception. a_Dw_ext_iterator_new is another case, and very confusing, so this one should be fixed.
Hem, that's what I meant (rush writing).
BTW, the first approach I wrote was to force a_Dw_iterator_free to make NULL its argument).
Very useful for debugging! :-)
That was the idea!
Well, that's it. As I don't know the iterators code as well as Sebastian, I'll let him make the decisions.
Well, first I'd use this change for the release, and fix it for 0.7.3. A grep for "a_Dw_ext_iterator_new" shows that this is handled correctly elsewhere.
OK guys, please give Copy&Paste some hard testing. If it shows no problems, I'll probably make the final touches this Monday and try to make a release on Tuesday.
Perhaps it is cleaner to remove the side-effect completely,
That sounds good and clean.
by letting a_Dw_ext_iterator_new putting a clone on top of the stack, so that the callers must free the simple iterator. This would be a bit slower, but compared to that, a_Dw_ext_iterator_new creates anyway quite a couple of DwIterator's. Has to be tested.
Yes, probably 0.7.3 will require some profiling and tunning.
(Thorben: As far as I can say from a first gprof run, this is not the critical code, so the problem is not made worse.)
This problem certainly is worth an investigation. Let it be on 0.7.3. Cheers Jorge.-
participants (2)
-
Jorge Arellano Cid -
Sebastian Geerken