[Solved] Resource Manager Stack Corruption

[Lignum]

I am currently attempting to write a resource manager for SFML to make my life easier. Ironically, it made my life harder. After having had two different managers, for fonts and textures, I decided to switch over to making a general manager that uses templates. This, however did not work out as expected.

I have submitted the entire class on pastebin: http://pastebin.com/ggynEjXP

Using this class causes the stack to corrupt around either the variable "line" or "tempResource" (seems to be random?) in the "LoadCollection" function. What weirds me out is that Visual Studio breaks on the closing bracket of the function and the call stack says the function "GetResource" leads to the corruption of both of these variables, when it doesn't even contain them.

This code snippet, that uses the manager, seems to be triggering the issue:

...
game->client->fontManager.LoadCollection("Error");
errorTextLn1.setFont(*game->client->fontManager.GetResource("arial.ttf")); // <-- Call stack refers to this.
...
 

Any help would be vastly appreciated since I'm sitting in an error storm, which is not too much of a nice experience. Thanks in advance.

[Lee R]

Long version: From the defines dotted around, I assume you have explicit template instantiations inside a DLL. When passing STL objects across DLL boundaries (e.g. std::sting as function parameters), both the DLL and .exe must link against the multithreaded DLL version of the CRT to ensure they use the same heap allocator. In addition, the release/debug version must match because STL objects may have a different size/layout in each.

Short version: Forget the DLL. Go header only; it is a template after all.

[Lignum]

Thanks for your reply, however, even though I have fiddled with the Runtime Library setting in the past they do match for both, the .exe and .dll, as do the Release and Debug configurations. So that's most likely not the problem.

Since the Resource Manager is not the only thing in my .dll (it's an entire game engine), I cannot go header only. Unfortunately making it an unacceptable solution.

--------------------------------------------------------------------------

I don't know whether this helps but when building with the release configuration I get an Acess Violation in new.cpp on line 62:

void *__CRTDECL operator new(size_t size) _THROW1(_STD bad_alloc)
        {       // try to allocate size bytes
        void *p;
        while ((p = malloc(size)) == 0)
                if (_callnewh(size) == 0)
                {       // report no memory
                        _THROW_NCEE(_XSTD bad_alloc, ); // <- Access violation is thrown here.
                }

        return (p);
        }

The debugger says that 'size' and 'p' were optimized away, is this normal?

The code above makes me think that I've reached the stack's maximum capacity, which is unlikely because 'new' doesn't have anything to do with the stack.

EDIT: The issue doesn't seem to occur when there's no resources to load.

[Lignum]

After having the resource map store std::unique_ptrs rather than actual objects, everything works as expected.
However when making a manager for sf::Fonts the program will trigger a breakpoint on

Code: [Select]

msvcr120d.dll!_CrtIsValidHeapPointer(const void * pUserData) Line 2034
when my program exits.

I do believe that this is a problem on SFML's side, because this code reproduces the error:

sf::Font* font = new sf::Font;
font->loadFromFile("Fonts/arial.ttf");
delete font;

EDIT: http://en.sfml-dev.org/forums/index.php?topic=10191.5 this guy is in the exact same situation.

[amir ramezani]

remove throw from your code, it should work
and, p must not optimized, it isn't normal

[Lignum]

remove throw from your code, it should work
and, p must not optimized, it isn't normal

Of course it will work, but it will merely hide the issue rather than actually fix it. Besides, that's Microsoft's code, not mine.

I've fixed that issue already, if you've seen my last post.

[Lee R]

I do believe that this is a problem on SFML's side, because this code reproduces the error:

sf::Font* font = new sf::Font;
font->loadFromFile("Fonts/arial.ttf");
delete font;

EDIT: http://en.sfml-dev.org/forums/index.php?topic=10191.5 this guy is in the exact same situation.

I'm unable to reproduce this behaviour. It's hard to imagine how this code could cause a problem if you're linking against the correct runtime libraries in all of your application's modules. Perhaps you could upload the project files of a minimal repro for someone to take a look at. I don't know about everyone else, but my crystal ball is currently in for cleaning

[Lignum]

I do believe that this is a problem on SFML's side, because this code reproduces the error:

sf::Font* font = new sf::Font;
font->loadFromFile("Fonts/arial.ttf");
delete font;

EDIT: http://en.sfml-dev.org/forums/index.php?topic=10191.5 this guy is in the exact same situation.

I'm unable to reproduce this behaviour. It's hard to imagine how this could cause a problem if you're linking to the correct runtime libraries in all of your application's modules. Perhaps you could upload the project files of a minimal repro for someone to take a look at. I don't know about everyone else, by my crystal ball is currently in for cleaning ;P

Which compiler are you using? I literally downgraded to VS2012 as you posted this, and everything's working fine. I was using a self-built version of SFML for VS2013 before.

[Lee R]

VS2012

[Lignum]

VS2012

Ah, that makes sense. Well I'm glad it works now. Thanks for the help.

[eXpl0it3r]

Ah, that makes sense.

It doesn't really make sense. I'd still say you're linked something wrong.

Also you should really abandon the use of manual memory management (new/delete). With modern C++ where we have RAII and smart pointers the new/delete pairs shouldn't be used anymore. Here's just an example why.

And I really don't advise to down grade to VS 2012, since VS 2013 comes with a lot of nice C++11 features and has a few bug fixes that might hunt you a long time when using VS 2012, since it's not fixed there.

[Lignum]

Ah, that makes sense.

It doesn't really make sense. I'd still say you're linked something wrong.

Also you should really abandon the use of manual memory management (new/delete). With modern C++ where we have RAII and smart pointers the new/delete pairs shouldn't be used anymore. Here's just an example why.

And I really don't advise to down grade to VS 2012, since VS 2013 comes with a lot of nice C++11 features and has a few bug fixes that might hunt you a long time when using VS 2012, since it's not fixed there.

Well perhaps I did link something wrong, but maybe it has something to do with the fact that I built it myself? Since for VS 2012 I used the pre-built binaries and that works.

I normally use smart pointers when it's too much of a pain to use new/delete because I care too much about performance/memory when I really shouldn't. But thanks for your advice, I'll consider (or rather force myself) to use smart pointers from now on.

[eXpl0it3r]

I normally use smart pointers when it's too much of a pain to use new/delete because I care too much about performance/memory when I really shouldn't.

Performance is not really an argument when it comes to unique_ptr, because compilers can esentially translate it the same way as new/delete, with the exception to cover all destruction cases.

And generally as with driving a car, I put safety first. The micro gain in performace is nothing, if the application leaks or crashes constantly.

[Lee R]

Performance is not really an argument when it comes to unique_ptr, because compilers can esentially translate it the same way as new/delete, with the exception to cover all destruction cases. ;)st. The micro gain in performace is nothing, if the application leaks or crashes constantly.

I'm no computer scientist, but it seems to me like they may even be more performant in some cases. Consider for example std::map<int, T*> versus std::map<int, std::unique_ptr<T>>. Assume the destruction of the map object takes O(N) time (i.e. there are N nodes to delete). The std::map<int, T*> version imposes and additional, cache miss filled loop to first manually delete the pointed to objects. What was an O(N) time operation has now effectively becomes O(2n) with a bunch of cache misses thrown in for good measure.

[eXpl0it3r]

Well computer scientists would now tell you that O(n) and O(2n) are "equal", since we'd only care about the asymptotic growth, thus a constant factor wouldn't matter, but I see what you mean. It for sure makes things a lot easier in most cases.