SFML community forums
General => General discussions => Topic started by: graphitemaster on October 12, 2010, 05:58:14 am
-
It's come to my concern that SFML causes tons of problems when running the simplest of things through valgrind. It seems as if SFML is not freeing X11 properly period, now I could be wrong about this I never looked at the source as I don't have it available, I do however have the headers, so I decided to take a peek it seems as if
priv::WindowImpl* myWindow; ///< Platform-specific implementation of window
which is a pointer, is never freed, now this could be handled somewhere else, and freed I'm not exactly sure. Valgrind reports tons of errors, regarding libX11 which can be seen is handled by libSFML. I went out of my way and made a mini test using libX11 and freeing everything myself and it passed valgrind -100%- however the same usage in SFML reported tons of errors, this is how I know SFML is not freeing X11 properly.
I have included a link below of the valgrind output on pastebin.com
I would like to see this fixed because I'm very retentive about memory
leaks and corruption, and when I'm testing my own code in valgrind I hate to
see my console, or a file that I have piped the valgrind output to be filled
with issues regarding SFML and X11, but rather the issue involved in my
code, so I can fix it.
VALGRIND: http://pastebin.com/hZ198eL4
-
All the "leaks" seem to be related to the global OpenGL context, which is indeed never freed on purpose (destroying it would cause crashes, because it happens at global exit and doesn't mix well with some OpenGL drivers).
However this is done on purpose, and the memory is freed by the OS anyway. It's not like a constant leak making the memory usage grow over time.
The only real issue is that it pollutes your valgrind log.
-
I'm not sure I follow, creating the exact same application without SFML, but instead using X11 directly and opening my own OpenGL context, and freeing it manually does not produce any errors with valgrind period.
How could freeing the context after you're finished using it cause a crash, and even if it did you only ever free the context to your window when you're finished with the program and want it to close, so a crash would not be an issue to anyone unless it was causing the program to close prematurely.
I'm sure for the most part, there should be a manual freeing function in SFML for the developers who do want to free the context regardless of what harm it could do, understanding the risks from things is something every developer knows.
Yes it does severely cause some undesired talk back in valgrind, and the longer the program is run the more back talk it generates, THIS SHOULD NOT HAPPEN.
It makes it incredibly hard to do proper debugging - which I understand most people using SFML don't do because SFML is such a high level API-
I'm having second thoughts on using SDL, or writing my own implementation for each operating system and platform for window context handling, Unlike most people here I'm not creating 2D side scrolling plat-formers, and my project that utilizes SFML is not a hobby project, rather a serious project that I have devoted most of my time to.
Please find a way to fix the issue, I beg you, valgrind begs you. Or else I'm going to half to find an alternative solution for my 3D engine.
-
Well, this crash at global exit was so hard to solve that I finally gave up and changed it to a memory leak as a workaround. If you want to see by yourself, you just have to change line 82 in src/SFML/Window/Context.cpp to this:
static Context GlobalContext;
No more memory leak, but a crash. If I remember well, it happens when you use dynamic libraries and the default font of the graphics module.
You can also modify SFML to add your manual cleaning function.
I have a similar bug in SFML 2 that I'm trying to solve, but it's even more tricky, it happens only with ATI and Intel drivers.
-
The only real issue is that it pollutes your valgrind log.
Unless the behavior is different on Windows, this sentence is not true. FAQ says the library works fine on Win98. This OS doesn't always free resources at exit, possibly resulting in memory (and probably not only memory) leak. If you want to be portable, you shouldn't rely on OS doing anything for you automatically, because some simply won't.
EDIT: rephrased to make it clearer.
-
Unless the behavior is different on Windows, this sentence is not true. FAQ says the library works on Win98, which don't always free resources at exit, possibly resulting in memory (and probably not only memory) leak.
Really? Can you show me this FAQ?
If you want to be portable, you shouldn't rely on OS doing anything for you automatically.
I'm not relying on anything, just trying to make this crappy bug less noticeable for users. If you have a clean fix, I'd be glad to apply it.
-
Really? Can you show me this FAQ?
Here you are (http://sfml-dev.org/wiki/en/faq#on_which_platforms_is_sfml_currently_available). I think you misunderstood me (my bad, that wasn't as clear as it should be), that "FAQ says..." part lasts only to the comma ;)
-
Here you are. I think you misunderstood me (my bad, that wasn't as clear as it should be), that "FAQ says..." part lasts only to the comma
Ah, ok ;)
My question was about the second part
Win98, which don't always free resources at exit
Do you have a source for that?
-
don't know if it's the problem but I think I have found a bug
http://sfml.svn.sourceforge.net/viewvc/sfml/branches/sfml2/src/SFML/Window/Linux/GlxContext.cpp?revision=1570&view=markup
line 104-105
the display should be flushed before it's destroyed?
also
http://sfml.svn.sourceforge.net/viewvc/sfml/branches/sfml2/src/SFML/Window/Linux/GlxContext.cpp?revision=1570&view=markup
line 234
GLXFBConfig* configs = glXChooseFBConfig(myDisplay, DefaultScreen(myDisplay), NULL, &nbConfigs);
thats in a while loop, and never freed, thats just a mem leak unless I'm not seing something.
-
Do you have a source for that?
Alas, I don't. I just vaguely remember that Win9x stores resources in some kind of memory pool (one for all programs), and if it ran out of space, the system crashed due to an alleged lack of free memory. Because some programs didn't free resources, and operating system didn't do that automatically, either, the OS as a whole was utterly unstable.
-
line 104-105
the display should be flushed before it's destroyed?
I don't remember if this line is really necessary. But it can't do harm, anyway.
line 234
GLXFBConfig* configs = glXChooseFBConfig(myDisplay, DefaultScreen(myDisplay), NULL, &nbConfigs);
Yup, this one was leaking, I fixed it. Thanks.
Alas, I don't. I just vaguely remember that Win9x stores resources in some kind of memory pool (one for all programs), and if it ran out of space, the system crashed due to an alleged lack of free memory. Because some programs didn't free resources, and operating system didn't do that automatically, either, the OS as a whole was utterly unstable.
Okay. I'll remember that.
-
XVisualInfo* bestVisual = NULL; line 184
http://sfml.svn.sourceforge.net/viewvc/sfml/branches/sfml2/src/SFML/Window/Linux/GlxContext.cpp?revision=1579&view=markup
not freed!?
-
No, this is just a pointer to an element of the visuals array, and this one is freed at the end of the function.
I guess there's no more memory leak, otherwise it would appear in the valgrind log. The GLXFBConfig stuff was not detected because the corresponding code was not executed (OpenGL 3 specific).
-
in that same while loop, you need a
delete [] name; that's also not freeded
const GLubyte* name = reinterpret_cast<const GLubyte*>("glXCreateContextAttribsARB");
line 229,
http://sfml.svn.sourceforge.net/viewvc/sfml/branches/sfml2/src/SFML/Window/Linux/GlxContext.cpp?revision=1579&view=markup
-
yes in that same while loop, you need a
delete [] name; that's also not freeded
const GLubyte* name = reinterpret_cast<const GLubyte*>("glXCreateContextAttribsARB");
line 229,
http://sfml.svn.sourceforge.net/viewvc/sfml/branches/sfml2/src/SFML/Window/Linux/GlxContext.cpp?revision=1579&view=markup
String literals are not allocated dynamically, they don't need to be freed.
Maybe you should investigate a little bit more about the "leaks" that you report ;)
-
yes in that same while loop, you need a
delete [] name; that's also not freeded
const GLubyte* name = reinterpret_cast<const GLubyte*>("glXCreateContextAttribsARB");
line 229,
http://sfml.svn.sourceforge.net/viewvc/sfml/branches/sfml2/src/SFML/Window/Linux/GlxContext.cpp?revision=1579&view=markup
String literals are not allocated dynamically, they don't need to be freed.
Maybe you should investigate a little bit more about the "leaks" that you report ;)
thats a byte which is a uchar* which does need to be freed, in this case it's being used to store memory, thus it has to be freed.
-
thats a byte which is a uchar* which does need to be freed
No, it points to a variable that has automatic storage. It would have to be freed if it was allocated dynamically, the type of the variable has nothing to do with that.
-
my bad, I thought we were working with memory, Tank just reminded me that
glXCreateContextAttribsARB is a const literal, sorry Laurent
-
graphitemaster: As I told Trina last night, the only time you really need to use free is when you have an associated new as well. Just because something has a pointer doesn't mean you always need to free it, which is what it appears that you are assuming.
Perhaps reading up on pointers and memory allocation/dynamic memory a little more might make things clearer.
I am, however, glad to see that your picking through the source code has uncovered a few bugs. :)
-
graphitemaster: As I told Trina last night, the only time you really need to use free is when you have an associated new as well. Just because something has a pointer doesn't mean you always need to free it, which is what it appears that you are assuming.
Perhaps reading up on pointers and memory allocation/dynamic memory a little more might make things clearer.
I am, however, glad to see that your picking through the source code has uncovered a few bugs. :)
You'll have to forgive me, I try to avoid pointers very much, only because they're the number one cause for memory leaks, and issues. Passing by reference is usually the smart thing to do in C++. However there is times when pointers are needed, and when then are, I usually code it to free it regardless of whether or not it should be. If she seg's then I give it a whirl in gdb where it usually tells me something like a double free, or free where there should not be, and I remove it.
the above may not always be a smart idea, but at least in the end I know everything I used has been freed properly, and we can all get on with our lifes, without worrying about memory leaks, which come and go. Please note some have come to me in the past and told me that the above was the dumbest idea that they have ever herd, I'm sorry it it is, but it was a trick that I was taught by from my teacher, and it has never failed me yet.