-4

I have researched this question for a while now and I think I have narrowed my problem down.

This is the error output

Critical error detected c0000374
Duke's Army.exe has triggered a breakpoint.

Exception thrown at 0x77E49841 (ntdll.dll) in Duke's Army.exe: 0xC0000374: A heap has been corrupted (parameters: 0x77E7C8D0).
Unhandled exception at 0x77E49841 (ntdll.dll) in Duke's Army.exe: 0xC0000374: A heap has been corrupted (parameters: 0x77E7C8D0).

The program '[14436] Duke's Army.exe' has exited with code 0 (0x0).

Call stack is as follows

ucrtbased.dll!0f8aa672()    Unknown
[Frames below may be incorrect and/or missing, no symbols loaded for ucrtbased.dll] 
[External Code] 
>   Duke's Army.exe!Tile::Tile() Line 19    C++
[External Code] 
Duke's Army.exe!Map::Map(int w, int h) Line 70  C++
Duke's Army.exe!MapGenerator::init(int w, int h) Line 37    C++
Duke's Army.exe!MapGenerator::MapGenerator(int w, int h) Line 13    C++
Duke's Army.exe!PlayGameState::PlayGameState(Game * g) Line 13  C++
Duke's Army.exe!main() Line 11  C++
[External Code] 

Other answers suggest removing a static member that wasn't declared properly or something akin to that. However, in the (supposed) affected class, there is a static vector that I cannot find a way to remove. Any suggestions?

[This is the class I think the errors occurs from] (Line 19 in the call stack is the beginning of the definition of the default constructor)

Tile.h

class Tile
{
public:
    static std::vector<Tile> tiles;

    // Constructors and methods...

    // Method used in constructors to add to static tiles       
    void Tile::init(const std::string& n, const sf::Color& c) {
        this->name = n;
        this->color = c;
        tiles.push_back(*this);
    }

    Tile(std::string n, sf::Color c) {
        init(n, c);
    };

    Tile() {
        init("bounds", sf::Color::Black);
    }

    const static Tile wall;
    const static Tile floor;
    const static Tile bounds;
    const static float TILE_SIZE;
};

Static members are declared in Tile.cpp

std::vector<Tile> Tile::tiles = std::vector<Tile>(3);
const Tile Tile::wall("wall", sf::Color::White);
const Tile Tile::floor("floor", sf::Color::Green);
const Tile Tile::bounds;
const float Tile::TILE_SIZE = 16.f;
underscore_d
  • 6,309
  • 3
  • 38
  • 64
  • I don't think the problem is going to be in any declaration code. Check your constructors and methods. – PC Luddite Aug 05 '16 at 20:35
  • `const Tile Tile::xxxx` should be `const Tile::xxxx` – Jean-François Fabre Aug 05 '16 at 20:37
  • 1
    @Jean-FrançoisFabre Um. I don't think so. Those members (three of them) are `Tile` objects, static to the class `Tile` (which is allowed). – WhozCraig Aug 05 '16 at 20:38
  • Does this happen in the debugger? What is the callstack? – kfsone Aug 05 '16 at 20:39
  • This happens in the debugger. I'll post the call stack with the answer. – user3131163 Aug 05 '16 at 20:42
  • That `tiles.push_back(*this)` seems awefully suspicious, and a recipe for an infinite loop of pushes if `init` is called from the constructor of Tile. – WhozCraig Aug 05 '16 at 20:43
  • Please show Tile::Tile() and annotate which is line 19 in your source. – kfsone Aug 05 '16 at 20:44
  • Edited OP to show both. Line 19 is beginning of definition of the default constructor for Tile – user3131163 Aug 05 '16 at 20:47
  • OK Now I see it. Thanks. Now think about this. It's impossible to construct a `Tile` without invoking `init()`, which itself constructs a tile when pushing a *copy* of the current tile into your static vector. That copy construction, invokes `init`, which creates a copy... Do you see the problem? – WhozCraig Aug 05 '16 at 20:52
  • I honestly never realized that a default constructor was used when using *this. I'm assuming that's what you mean. – user3131163 Aug 05 '16 at 20:52
  • yeah, I see them. read my prior comment. – WhozCraig Aug 05 '16 at 20:53
  • I'm thinking that `push_back` should invoke the copy constructor--which is either not defined or not shown by the way. Watch out for [the Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). Anyway, you're in for a bit of fun because what went into `tiles` (which is incompletely defined: `std::vector` of what?) will not be the object you are creating, but a copy of it. Here's where the Rule of Three may kick in and shoot you in the head if `Tile` is not copyable without a custom copy constructor. – user4581301 Aug 05 '16 at 21:04
  • I suppose a separate registrar would be advisable? Or maybe passing a tile vector where Tile::tiles is used? I see your point either way. Thank you for the help – user3131163 Aug 05 '16 at 21:05
  • @user4581301 so essentially overload the copy constructor is what you're saying? – user3131163 Aug 05 '16 at 21:12
  • The copy constructor problem may kick in later depending on the unknown members of `Tile`. If everything automatically copies, you have no problem here. If, say, you have a pointer... watch out. – user4581301 Aug 05 '16 at 21:19

1 Answers1

3

Your code default-initializes Tile::tiles like this:

std::vector<Tiles> Tile::tiles = std::vector<Tile>(3);

That construction of vector doesn't just set capacity, it creates a vector containing 3 elements, default constructored, which will result in 3 calls to init, and in init you

tiles.push_back(*this);

push_back attempts to grow the vector by one and then copy-construct the newly added element. The key part here is grow the vector.

Again: remember, this is happening during the construction of the vector.

You will either create a new element beyond the target size of the vector or over-write the element currently being populated.

The GNU implementation of std::vector doesn't set the vector size until the constructor has been completed. As a result, you get overwrites:

#include <iostream>
#include <string>
#include <vector>

struct S {
    std::string s_;
    static std::vector<S> tiles;

    S() { std::cout << "S()\n"; init("default"); }
    S(const std::string& s) {
        std::cout << "S(" << (void*) this << " with " << s << ")\n";
        init(s);
    }
    S(const S& rhs) {
        std::cout << (void*) this << " copying " << (void*)&rhs << " (" << rhs.s_ << ")\n";
        s_ = rhs.s_;
        s_ += " copy";
    }

    void init(const std::string& s) {
        s_ = s;
        std::cout << "init " << (void*)this << " " << s_ << "\n";
        tiles.push_back(*this);  // makes copy
    }
};


std::vector<S> S::tiles = std::vector<S>(3);

int main() {
    for (const auto& el : S::tiles) {
        std::cout << el.s_ << "\n";
    }
}

Outputs http://ideone.com/0dr7L2

S()
init 0x9e67a10 default
0x9e67a10 copying 0x9e67a10 ()
S()
init 0x9e67a14 default
0x9e67a14 copying 0x9e67a14 ()
S()
init 0x9e67a18 default
0x9e67a18 copying 0x9e67a18 ()
 copy
 copy
 copy

So you are introducing UB during startup of your application.

In the above example, the copy constructor default-initializes its target before performing the copy, and since it's copying itself this results in rhs.s_ being cleared. This is why we get "copy" and not "default copy".

--- Edit ---

(invalid, as pointed out by @underscore_d)

--- Edit 2 ---

The MSVC vector implementation does this:

explicit vector(size_type _Count)
    : _Mybase()
    {   // construct from _Count * value_type()
    if (_Buy(_Count))
        {   // nonzero, fill it
        _TRY_BEGIN
        _Uninitialized_default_fill_n(this->_Myfirst(), _Count,
            this->_Getal());
        this->_Mylast() += _Count;
        _CATCH_ALL
        _Tidy();
        _RERAISE;
        _CATCH_END
        }
    }

The key part being:

        _Uninitialized_default_fill_n(this->_Myfirst(), _Count,
            this->_Getal());
        this->_Mylast() += _Count;

During the fill, your push_back's will increment _MyLast by 3 positions, and then the next line of the ctor will increment _MyLast by 3 more.

Here's the same code from above running under Visual Studio: http://rextester.com/WNQ21225

kfsone
  • 23,617
  • 2
  • 42
  • 74
  • @user4581301 Yes - see also my edit about wall, floor and ceil. – kfsone Aug 05 '16 at 21:26
  • "order of initialization is undefined" only between different translation units. Those 2 variables are defined in a single TU. In that case, as with any other `static` objects, the order of initialisation is guaranteed to follow the order of definition. Your last section seems to be irrelevant. – underscore_d Aug 05 '16 at 21:37
  • @user4581301 See edit 2, along with a link to an online visual studio version of the test code. – kfsone Aug 05 '16 at 21:45
  • @underscore_d doh! duly corrected thanks – kfsone Aug 05 '16 at 21:47