X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f
X-Recipient: geda-user AT delorie DOT com
X-CMAE-Analysis: v=2.4 cv=IqjbzJzg c=1 sm=1 tr=0 ts=5fd3db32
 a=+cj0cO56Fp8x7EdhTra87A==:117 a=yMh6k+SuEjDQ+c2SXtzTbA==:17
 a=9+rZDBEiDlHhcck0kWbJtElFXBc=:19 a=IkcTkHD0fZMA:10 a=zTNgK-yGK50A:10
 a=a1KZgU7cAAAA:8 a=Mj1Xp5F7AAAA:8 a=B-2xtJEx7iuf_Y2qmxcA:9 a=QEXdDO2ut3YA:10
 a=ng0hpkU2jXKPaRTLMVYJ:22 a=OCttjWrK5_uSHO_3Hkg-:22
X-SECURESERVER-ACCT: glimrick AT epilitimus DOT com
Subject: Re: [geda-user] Memory leak patches (was: SAB processing patches)
To: geda-user AT delorie DOT com
References: <e4ff3c96-939b-a93e-a32f-5e938b6daa63 AT epilitimus DOT com>
 <alpine DOT DEB DOT 2 DOT 21 DOT 2011302045040 DOT 2894 AT nimbus>
 <20201130220505 DOT 0AE4282C54FD AT turkos DOT aspodata DOT se>
 <7c75ed03-456c-b408-8b50-0448f6b3a04f AT epilitimus DOT com>
 <alpine DOT DEB DOT 2 DOT 21 DOT 2012011606400 DOT 6785 AT nimbus>
 <1b2c64b3-6a36-c1f3-dd54-bb583c6bea17 AT epilitimus DOT com>
 <alpine DOT DEB DOT 2 DOT 21 DOT 2012021253440 DOT 1262 AT nimbus>
 <475f980e-fddd-60d1-9a02-a5bc5fb5805b AT epilitimus DOT com>
 <alpine DOT DEB DOT 2 DOT 21 DOT 2012071333290 DOT 3907 AT nimbus>
 <e6c5135f-b127-fd55-91b2-79aa357a5a07 AT epilitimus DOT com>
 <alpine DOT DEB DOT 2 DOT 21 DOT 2012081644480 DOT 3649 AT nimbus>
 <8706c896-e01a-9e6e-2bab-fe3607064093 AT epilitimus DOT com>
 <alpine DOT DEB DOT 2 DOT 21 DOT 2012111645180 DOT 13096 AT nimbus>
From: "Glenn (glimrick AT epilitimus DOT com) [via geda-user AT delorie DOT com]" <geda-user AT delorie DOT com>
Message-ID: <87a182b1-1ec8-47ae-e2e7-68e7306e4797@epilitimus.com>
Date: Fri, 11 Dec 2020 12:48:42 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Firefox/60.0 SeaMonkey/2.53.3
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.21.2012111645180.13096@nimbus>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-AntiAbuse: This header was added to track abuse, please include it with any abuse report
X-AntiAbuse: Primary Hostname - a2plcpnl0121.prod.iad2.secureserver.net
X-AntiAbuse: Original Domain - delorie.com
X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12]
X-AntiAbuse: Sender Address Domain - epilitimus.com
X-Get-Message-Sender-Via: a2plcpnl0121.prod.iad2.secureserver.net: authenticated_id: glimrick AT epilitimus DOT com
X-Authenticated-Sender: a2plcpnl0121.prod.iad2.secureserver.net: glimrick AT epilitimus DOT com
X-Source: 
X-Source-Args: 
X-Source-Dir: 
X-CMAE-Envelope: MS4xfNb9W66dwMifRP/5OO/RyuNISlj/FMUJbX3Oko7D4jb8dt17cczqVSFCb6i3byKYXWoQOwl30uxkIIKdoRKR4seoC0uPrQBCot1UBVPU2aZuXmZpkoMW
 c26InzFUsvzMXxpDJbjf6zB0ZRPIY+XRKc2/iQ2Wugj+ryzE7nYyklrK8buXcabm5F+CPPe7JDFFsjCM0ReSh7v96/tM40N+sPx0p3LjVk+M2cO4wdg/APif
Reply-To: geda-user AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: geda-user AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk

Roland Lutz wrote:
> Hi Glenn,
>
> On Tue, 8 Dec 2020, Glenn (glimrick AT epilitimus DOT com) [via
> geda-user AT delorie DOT com] wrote:
>> There is currently only one commit to the mem_leak branch, commit
>> 5ccfd8b8a2
>
> thank you for fixing these!
I don't know if you saw it or not but I added a second commit the other
night.

I guess since you found these you were able to access my repo without
trouble.
>
> A few of the variables you freed were actually still in use, so I had
> to change a few things.  In particular, g_object_set_data takes a
> generic pointer, so the string mustn't be freed while the object is
> still around. In eda_config_get_context_for_file, the path object must
> only be freed if it was created in the function or it would result in
> a double free; and in restore_current_page, you accidentally freed the
> configuration context instead of the retrieved string.  I also moved
> the convenience function you introduced to i_basic.c and made it
> static (no need to pollute the global namespace with something that is
> only used once), removed a comment which made only sense if you knew
> the previous version of the code (comments like this go better into
> the commit message), simplified the free handling in x_clipboard.c,
> and applied gEDA/gaf coding style.
>
I would definitely encourage you to look over any of the leak fixes
carefully as I am not familiar enough with the code, or gnome for that
matter, to be certain I am not introducing bugs. I have been doing
compiles on each fix to make sure there aren't any syntax errors but
runtime is more subtle. My plan was to do a run test after I had a bunch
in place and see if anything popped up. Generally once I have identified
the variable that needs to be freed (not always successfully it would
seem) I do a simple find to locate the last place the variable is used
and then free it after that. If it is returned I follow it up the stack
trace until I think I've found the right spot. Usually my reason for
putting in comments is to make it clear *why* I did what I did, so yes
those comments can be discarded as they really aren't intended to be
permanent, but putting them in the commit distances them from the code
in question, especially since there are multiple files in each commit
and I may not remember the why or the where by the time I do the commit.
I understand it makes a bit more work for you though so I will try to
keep them to a minimum.
> May I ask how you proceeded to find these leaks?  I always struggled
> to do memory analysis on gschem because of the large amount of
> warnings introduced by Guile and Gtk.
When I hooked SAB into s_slot.c I ran valgrind against gschem while I
exercised the SAB code to make sure I didn't introduce any memory errors
as a result of my lack of experience with glib. That run produced a list
of 108 leaks, but some portion (roughly 1/4 to 1/3) of those are
entirely in 3rd party libraries, e.g. gnome or guile, so can be ignored,
some are also duplicates. I am about half way through the list. It is a
tedious process (I only manage about a hour or so at a time, usually
while I am letting my back brain grind on some other problem) of looking
at each entry for a reference to geda code and then looking at that code
so see if there is anything to be done there. Mostly there is but a few
times it is something the 3rd party library is doing. I have found a few
that I can't see where to fix because I don't know the code well enough.
If I can't figure them out by the time I have worked my way through the
entire list I will forward them to you.

Because the list I generated was purely from exercising the SAB code it
is by no means exhaustive. I have also found a few places that generate
compile warnings. I was going to fix them in a separate commit at the
end just to keep it from getting too overwhelming.

After I am done my plan is to do another run with valgrind to see what
happens. Unfortunately valgrind can only tell you about leaks in code
you exercise so it is an ongoing process.
>
> It will take me some more time to review the SAB patches; this is
> something which requires my full attention, so I can't just fit it
> into any free time slot.
No problem, take all the time you need. I have it now for my use :)

And please feel free to ask any more questions. I will try to keep my
answers as concise as I can :)

Glenn