C++ Dynamic Member Arrays Deleted Right Before Destructor Called

Question

I'm working on an AI project and have started to implement a NeuralNetwork class. I just want to get something basic down so I used some malloc statements with some placement news and finally delete[]s.

However, once the NeuralNetwork object (created on the stack in the main function) is about to be deleted (I set a breakpoint at the start of the destructor), my arrays seem to have been prematurely deleted (value 0xcccccccc) and the delete[] statements therefore throw access violations.

Through further investigation I found out that this deleting happens right between the last Vector object being destructed and the start of the destructor of my NeuralNetwork object being called.

I made sure to implement both copy constructors and assignment operators to make sure no copying was taking place without me noticing.

I'm really baffled with this problem and hope that someone can catch my mistake. Source code will follow:

NeuralNetwork::NeuralNetwork(const std::initializer_list<size_t>& l):
    m_size(l.size() - 1),
    m_weights(static_cast<Matrix<double>*>(malloc(sizeof(Matrix<double>) * m_size))),
    m_biases(static_cast<Vector<double>*>(malloc(sizeof(Vector<double>) * m_size)))
{
    size_t index = 0;
    auto itr = l.begin();

    for (auto next = itr + 1; next != l.end(); ++next, ++itr, ++index)
    {
        new (m_weights + index) Matrix<double>(*next, *itr);
        new (m_biases + index) Vector<double>(*next);
    }
}

NeuralNetwork::NeuralNetwork(const NeuralNetwork& nn) :
    m_size(nn.m_size),
    m_weights(static_cast<Matrix<double>*>(malloc(sizeof(Matrix<double>)* m_size))),
    m_biases(static_cast<Vector<double>*>(malloc(sizeof(Vector<double>)* m_size)))
{
    for (size_t index = 0; index < m_size; ++index)
    {
        new (m_weights + index) Matrix<double>(nn.m_weights[index]);
        new (m_biases + index) Vector<double>(nn.m_biases[index]);
    }
}

NeuralNetwork::NeuralNetwork(NeuralNetwork&& nn) noexcept :
    m_size(nn.m_size),
    m_weights(nn.m_weights),
    m_biases(nn.m_biases)
{
    nn.m_size = 0;
    nn.m_weights = nullptr;
    nn.m_biases = nullptr;
}

NeuralNetwork::~NeuralNetwork()
{
    delete[] m_weights; // exception thrown here, value is 0xcccccccc, nullptr
    delete[] m_biases;
}

Main code:

int main()
{
    NeuralNetwork nn{ 2, 1 };

    Vector<double> input(2);
    input.Get(0) = 0.5;
    input.Get(1) = 0.25;

    Vector<double> output = nn.Forward(input); // just does the math, nothing special

    for (size_t i = 0; i < output.GetSize(); ++i)
        std::cout << output.Get(i) << " ";

    std::cout << std::endl;
}

In case any important source code is missing please let me know, thanks!

Why would you do that `static_cast`+`malloc` magic? Also, `delete[]` is the pair of `new[]`. Btw. `new`, people usually want to store their result, you just call them in various loops. — tevemadar, Dec 07 '19 at 22:36
I'm storing them in the weights and biases arrays. m_weights + index is a pointer to the address at which to store them — J. Lengel, Dec 07 '19 at 22:39
I use malloc + cast because I don't want to default construct the arrays — J. Lengel, Dec 07 '19 at 22:41
But you should manually call the destructor on each element and free the memory with free not delete[] — Victor Padureanu, Dec 07 '19 at 22:42
That is true and I will change it but like I stated the issue happens before delete[] is even called — J. Lengel, Dec 07 '19 at 22:43
Add a data breakpoint on the memory after is was initialized and you can find where it was modified — Victor Padureanu, Dec 07 '19 at 22:44
Even if it would matter, you could use a `std::vector` of pointers. But I assume `Vector` and `Matrix` are just storing numbers, so there is nothing wrong with having them allocated automatically and then setting the values. — tevemadar, Dec 07 '19 at 22:46
First of all: How do I explicitly call a destructor on a non pointer type and secondly: how do I open the memory tab (I've looked for it quite often, I'm using VS2019 Community) — J. Lengel, Dec 07 '19 at 22:46
https://devblogs.microsoft.com/cppblog/data-breakpoints-15-8-update/ — Victor Padureanu, Dec 07 '19 at 22:47
I do have to call the destructors on my Vector and Matrix objects as they contain dynamic arrays as well — J. Lengel, Dec 07 '19 at 22:48
https://stackoverflow.com/questions/14187006/is-calling-destructor-manually-always-a-sign-of-bad-design — Victor Padureanu, Dec 07 '19 at 22:48
I also just realized that I can call it like a regular method but thanks anyway :D — J. Lengel, Dec 07 '19 at 22:50
Deleting my arrays with free solved the problem! I still don't know exactly why, but it works now thanks! Could you please post that comment in the form of an answer so I can accept it? — J. Lengel, Dec 07 '19 at 22:52

score 3 · Accepted Answer · answered Dec 07 '19 at 22:55

There is a big difference between malloc/free and new/delete and new[] / delete[]

malloc will allocate an unformated chunk of memory and free will free it

new will allocate and initialize that region and delete will call the destructor

sometimes it might work to use malloc and delete but it's a bad idea

new[] will also keep a few extra info before the the returned memory to know how many objects need to be deleted

Usefull links: https://www.geeksforgeeks.org/placement-new-operator-cpp/

score 1 · Answer 2 · answered Dec 07 '19 at 23:54

You should never write such code:

You should never have to use malloc/free in a C++ program.
Allocation and desallocation should match.
Dynamic memory allocation has surely more overhead that default initialization you try to avoid.
Your code would miserably failed if initializer list is empty.
Code has memory leaks.
If you define a copy constructor, then you should also define assignment operator (same for move constructor).

Standard library already do most relavant optimization. For example,, for a std::vector the constructor of an item will be only called when you call emplace_back.

You should really write standard code. It does not worth the trouble to write bugged code for marginal performance improvement.

Your class declaration should really look something like:

class NeuralNetwork
{
public:
    NeuralNetwork(const std::initializer_list<size_t>& l);
    // Other constructors as appropriate here…

private:
    std::vector<Matrix<double>> m_weights;
    std::vector<Vector<double>> m_biases;
};

And constructor should look similar to that (code not tested):

NeuralNetwork::NeuralNetwork(const std::initializer_list<size_t>& l):
{
    if (l.empty()
    {
        // might assert in debug or throw an exception...
        return;
    }

    m_weights.reserve(m_size);
    m_biases.reserve(m_size);

    auto itr = l.begin();
    for (auto next = itr + 1; next != l.end(); ++next, ++itr, ++index)
    {
        m_weights.emplace(*next, *itr);
        m_biases.emplace(*next);
    }
}

Other constructors, assignment operators and destructors should be easier to implement, more robust and performance very similar.

score 0 · Answer 3 · answered Dec 08 '19 at 11:51

As you are using C++11 features already, you can also use std::vector::emplace_back(), which will deal with placement new internally.

Example:

#include <iostream>
#include <initializer_list>
#include <vector>

template<class T> class Matrix {
public:
    Matrix() {std::cout << "Matrix() " << this << std::endl;}
    Matrix(int width,int height):w(width),h(height) {std::cout << "Matrix(" << w << "x" << h << ") " << this << std::endl;}
    ~Matrix() {std::cout << "Matrix(" << w << "x" << h << ") " << this << " goodbye" << std::endl;}
private:
    int w,h;
};

class NN {
public:
    NN()=default;
    NN(const std::initializer_list<size_t> &l);
private:
    std::vector<Matrix<double>> m_weights;
};

NN::NN(const std::initializer_list<size_t> &l) {
    m_weights.reserve(l.size()-1); // or deal with move constructors
    auto itr = l.begin();
    for (auto next = itr + 1; next != l.end(); ++next, ++itr)
    {
        m_weights.emplace_back(*next, *itr);
    }
}

int main() {
    NN test{2,3,3,2};
    return 0;
}

Output (from https://ideone.com/yHGAMc):

Matrix(3x2) 0x5638f59aae70
Matrix(3x3) 0x5638f59aae78
Matrix(2x3) 0x5638f59aae80
Matrix(3x2) 0x5638f59aae70 goodbye
Matrix(3x3) 0x5638f59aae78 goodbye
Matrix(2x3) 0x5638f59aae80 goodbye

So the default constructor was not involved and objects were destructed properly.

C++ Dynamic Member Arrays Deleted Right Before Destructor Called

3 Answers3