Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Pekka Paalanen]

On Mon, 18 Sep 2017 03:19:13 -0700
Yuri  wrote:

> On 09/18/17 03:07, Emil Velikov wrote:
> > Yes there's libraries that do all sorts of things to help lazy
> > users... I think pixman is not one of them.  
> 
> pixman allocates and maintains the object, so it should ensure that it 
> is also properly released. It doesn't appear reasonable to expect that 
> many users will be always releasing it. It's easier to do this in one place.

Hi,

I totally agree with Emil. Pixman is not a library that holds the hands
of developers who do not bother following the API. This is C, not some
high-level language that automatically cleans up.


Thanks,
pq


pgp1Bhx8BqK7R.pgp
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Yuri]

On 09/18/17 03:07, Emil Velikov wrote:

Failure to call a destructor in C code could be interpreted as a used error,
or as a library error. I prefer to call it a library error, because it's
easier to just destroy it in the library, and not depend on users.

One can call it spaceship if they want to. That doesn't mean it's right;-)


I see, you hold a monopoly on rightness.



Yes there's libraries that do all sorts of things to help lazy
users... I think pixman is not one of them.


pixman allocates and maintains the object, so it should ensure that it 
is also properly released. It doesn't appear reasonable to expect that 
many users will be always releasing it. It's easier to do this in one place.



Yuri

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Emil Velikov]

On 17 September 2017 at 22:25, Yuri  wrote:

>> What you reported seems like an user error. Although without a proper
>> log nobody can tell you for sure.
>> The leak I've spotted is a genuine leak in pixman.
>
>
> Failure to call a destructor in C code could be interpreted as a used error,
> or as a library error. I prefer to call it a library error, because it's
> easier to just destroy it in the library, and not depend on users.
One can call it spaceship if they want to. That doesn't mean it's right ;-)
Yes there's libraries that do all sorts of things to help lazy
users... I think pixman is not one of them.

> In my case, the user is the gtk app created with new Gtk::Main. It appears
> that gtk doesn't destroy the pixman object?
>
Perhaps your app is fine and gtk is doing something silly? Without a
valgrind log nobody can tell you for sure.
If you have one handy, do ensure that you have all the debug symbols present.

I'll repeat my earlier request - please don't use HTML emails.
See http://kb.mozillazine.org/Plain_text_e-mail_(Thunderbird)

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Yuri]

On 09/17/17 14:08, Emil Velikov wrote:

I don't think it's pixman's job to hold the user's hand. If the user
does not clear what it creates, then the user should be fixed. As
mentioned - pixman emits lovely BUG notations when that happens.


No, I don't see any such BUG messages in my app.





All this is obviously orthogonal to the original issue reported;-)

I don't see how it is orthogonal.


What you reported seems like an user error. Although without a proper
log nobody can tell you for sure.
The leak I've spotted is a genuine leak in pixman.


Failure to call a destructor in C code could be interpreted as a used 
error, or as a library error. I prefer to call it a library error, 
because it's easier to just destroy it in the library, and not depend on 
users.
In my case, the user is the gtk app created with new Gtk::Main. It 
appears that gtk doesn't destroy the pixman object?




There is actually __attribute__((destructor))
https://phoxis.org/2011/04/27/c-language-constructors-and-destructors-with-gcc
It works with gcc and clang, and probably with most or all other compilers.


This is precisely what I recommended, haven't I?



Ok then, if you meant this.


Yuri


___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Emil Velikov]

Yuri, please don't use HTML emails. It completely messes up the quotation.

On 17 September 2017 at 21:17, Yuri  wrote:
> On 09/17/17 13:07, Emil Velikov wrote:
>
>> Having the opposite - a destructor [1] should provide symmetry and
>> consistency.
>> Furthermore using atexit is not as portable/reliable as one would think.
>>
>
> It should be the destructor that handles this. Either the user clears it, or
> destructor clears it.
>
I don't think it's pixman's job to hold the user's hand. If the user
does not clear what it creates, then the user should be fixed. As
mentioned - pixman emits lovely BUG notations when that happens.

>> All this is obviously orthogonal to the original issue reported ;-)
>
>
> I don't see how it is orthogonal.
>
What you reported seems like an user error. Although without a proper
log nobody can tell you for sure.
The leak I've spotted is a genuine leak in pixman.

>
> There is actually __attribute__((destructor))
> https://phoxis.org/2011/04/27/c-language-constructors-and-destructors-with-gcc
> It works with gcc and clang, and probably with most or all other compilers.
>
This is precisely what I recommended, haven't I?

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Yuri]

On 09/17/17 13:07, Emil Velikov wrote:

Having the opposite - a destructor [1] should provide symmetry and consistency.
Furthermore using atexit is not as portable/reliable as one would think.


It should be the destructor that handles this. Either the user clears 
it, or destructor clears it.




All this is obviously orthogonal to the original issue reported;-)



I don't see how it is orthogonal.


There is actually |__attribute__((destructor)) 
https://phoxis.org/2011/04/27/c-language-constructors-and-destructors-with-gcc 
It works with gcc and clang, and probably with most or all other compilers.

|


Yuri

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Emil Velikov]

On 17 September 2017 at 17:46, Yuri  wrote:

>> Maybe the right solution is to add explicit pixman_init() and
>> pixman_destroy() functions. But we need to support existing
>> applications too and we can't expect them to start using these
>> new functions without enforcing some changes. That's a kind of
>> historical baggage, and nobody feels like opening this can of
>> worms.
>
>
>
> You can't rely on the apps to always call the right functions. Instead, the
> correct behavior is to uninitialize on exit. What you need to do is to
> schedule uninitialization with atexit call.
> (https://linux.die.net/man/3/atexit)
>
> The behavior should be the same as when there is a static C++ object with
> destructor. Such destructor will always be called via atexit, either at the
> end of the application, or when the shared library is unloaded.
>
Having the opposite - a destructor [1] should provide symmetry and consistency.
Furthermore using atexit is not as portable/reliable as one would think.

All this is obviously orthogonal to the original issue reported ;-)

-Emil

[1] 
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-destructor-function-attribute
[2] https://bugs.freedesktop.org/show_bug.cgi?id=91869
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Yuri]

On 09/17/17 06:10, Siarhei Siamashka wrote:

Can Yuri tell us more about his dynamic load/reload use case? So far
I'm not aware of anyone doing this in practice.


I instantiate LV2 (audio) plugin UIs, which are mostly gtk in the 
non-gtk app. I dynamically load libgtkmm/libglib libraries in order to 
show the UI, then I unload them. This is done with dlopen/dlsym/dlclose.




Maybe the right solution is to add explicit pixman_init() and
pixman_destroy() functions. But we need to support existing
applications too and we can't expect them to start using these
new functions without enforcing some changes. That's a kind of
historical baggage, and nobody feels like opening this can of
worms.



You can't rely on the apps to always call the right functions. Instead, 
the correct behavior is to uninitialize on exit. What you need to do is 
to schedule uninitialization with atexit call. 
(https://linux.die.net/man/3/atexit)


The behavior should be the same as when there is a static C++ object 
with destructor. Such destructor will always be called via atexit, 
either at the end of the application, or when the shared library is 
unloaded.



Yuri

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Siarhei Siamashka]

On Sun, 17 Sep 2017 13:18:01 +0100
Emil Velikov  wrote:

> Hi Yuri,
> 
> On 15 September 2017 at 22:41, Yuri  wrote:
> > google-perftools memory profile on a gtkmm-based app shows that there is
> > 0.3MB of memory left by pixman_glyph_cache_create.
> >
> >
> > pixman should free all memory that it allocates. For example, if the app
> > will load the gtkmm UI dynamically, this memory will be permanently lost
> > every time, resulting in the memory leak.
> >
> >
> > pixman-0.34.0
> >  
> 
> Keep in mind that I'm not a pixman developer, so take the following
> with a grain of salt.
> 
> Skimming through the pixman code - this seems like an user error.
> Namely are you sure the application has not forgot to free the cache
> before destroying it?
> 
> Pay special attention to stderr, as pixman_glyph_cache_destroy() is
> invoked. You should see a lovely "*** BUG ***" with extra information.
> 
> As a sanity check - the following shows no leaks in the mentioned code path*.
> 
> $ cat pp.c
> // gcc -O0 -I/usr/include/pixman-1 -lpixman-1 -o pp-test && valgrind pp-test
> #include 
> 
> int
> main(void)
> {
>pixman_glyph_cache_destroy(pixman_glyph_cache_create());
>return 0;
> }
> 
> HTH
> Emil

Yes, most of the problems with pixman are user errors nowadays.

> * There are some 'leaks' due to the custom DSO constructor, but let's
> look at that separately.

Indeed, it's a rather tricky and fragile stuff, which is also system and
compiler dependent. We need to do some init/deinit stuff at the time of
library load/unload (the pixman implementation pointer) and also at the
time of thread creation/destruction too (the per-thread fast path
cache).

If we only had to support just Linux as the operating system and only
GCC as the compiler, then everything would be rather straightforward.
But pixman works in other environments too.

Can Yuri tell us more about his dynamic load/reload use case? So far
I'm not aware of anyone doing this in practice.

Maybe the right solution is to add explicit pixman_init() and
pixman_destroy() functions. But we need to support existing
applications too and we can't expect them to start using these
new functions without enforcing some changes. That's a kind of
historical baggage, and nobody feels like opening this can of
worms.

-- 
Best regards,
Siarhei Siamashka
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Emil Velikov]

Hi Yuri,

On 15 September 2017 at 22:41, Yuri  wrote:
> google-perftools memory profile on a gtkmm-based app shows that there is
> 0.3MB of memory left by pixman_glyph_cache_create.
>
>
> pixman should free all memory that it allocates. For example, if the app
> will load the gtkmm UI dynamically, this memory will be permanently lost
> every time, resulting in the memory leak.
>
>
> pixman-0.34.0
>

Keep in mind that I'm not a pixman developer, so take the following
with a grain of salt.

Skimming through the pixman code - this seems like an user error.
Namely are you sure the application has not forgot to free the cache
before destroying it?

Pay special attention to stderr, as pixman_glyph_cache_destroy() is
invoked. You should see a lovely "*** BUG ***" with extra information.

As a sanity check - the following shows no leaks in the mentioned code path*.

$ cat pp.c
// gcc -O0 -I/usr/include/pixman-1 -lpixman-1 -o pp-test && valgrind pp-test
#include 

int
main(void)
{
   pixman_glyph_cache_destroy(pixman_glyph_cache_create());
   return 0;
}

HTH
Emil
* There are some 'leaks' due to the custom DSO constructor, but let's
look at that separately.
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman