1

This is a follow up on one of my previous questions. The issue that I am dealing with is explained in detail in the formulation of the aforementioned question. Unfortunately, I was not able to provide a minimal example that showcases the problem.

In this question, I am making an attempt to redefine the problem and provide a minimal example. The code presented in the example below executes and does what it is supposed to do. However, in a slightly more complex case presented in the previous question, sometimes, it results in the runtime error

dynamic_links(3941,0x7fff749a2310) malloc: *** error for object 0x61636f6c65720054: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug

Unfortunately, I was only able to produce the error when the optimisation is set to -O3 (possibly -O2 as well). This creates a problem when using debugging tools. Unfortunately, I was not able to reproduce the problem with no/minimal code optimisation. For a reference, I am using gcc 4.9.1.

In the present question, I would like to understand if the structural design of the class inheritance mechanism that I am using could, potentially, be dangerous from the perspective of the dynamic memory allocation. Please find the code below:

namespace ublas = boost::numeric::ublas;

template<typename TScalarType = double>
using ublasRn = ublas::vector<TScalarType>;

class Base
{
public:
    virtual ~Base(void) = 0;
};
Base::~Base(void){}

template<typename T1, typename T2>
class Composite : public Base
{
protected:
    T1 T1Instance;
    std::unique_ptr<T2> u_T2Instance;
public:
    Composite(){}
    virtual ~Composite(void){}
    const std::type_info& returnT1TypeID(void) const
        {return typeid(T1);}
    const std::type_info& returnT2TypeID(void) const
        {return typeid(T2);}
};

template<typename T2>
class CompositeCT: virtual public Composite<double, T2>
{
public:
    using Composite<double, T2>::Composite;
    virtual ~CompositeCT(void)
        {}
};

template<typename T1>
class CompositeRn: virtual public Composite<T1, ublasRn<double>>
{
public:
    using Composite<T1, ublasRn<double>>::Composite;
    virtual ~CompositeRn(void){}
};

class CompositeCTRn :
    public CompositeCT<ublasRn<double>>,
    public CompositeRn<double>
{
public:
    CompositeCTRn(void):
        Composite<double, ublasRn<double>>(),
        CompositeCT<ublasRn<double>>(),
        CompositeRn<double>()
        {};
};

template<typename T1, typename T2, class TComposite>
class Trajectory: public Base
{
protected:
    std::vector<std::unique_ptr<TComposite>> m_Trajectory;
public:
    Trajectory(void)
        {checkType();}
    void checkType(void) const
        {
            TComposite CI;
            if (
                !(
                    CI.returnT1TypeID() == typeid(T1) &&
                    CI.returnT2TypeID() == typeid(T2)
                    )
                )
            throw std::runtime_error("Error");
        }
    virtual ~Trajectory(void){}
};

int main()
{

    Trajectory<
        double,
        ublasRn<>,
        CompositeCTRn
        > T;

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

Note. I am using the external library boost::ublas. I believe that it is not unlikely that the problem is related to the dynamic memory allocation mechanism for the ublas objects.

Community
  • 1
  • 1
  • Did you try gdb, to see where the error occurs? You can also try valgrind. – Brahim May 27 '15 at 12:48
  • By the way I don't see the point of using virtual when inheriting from Composite – Brahim May 27 '15 at 12:54
  • @Brahim Thank you for the comments. Could you please explain further why should I not be using `virtual` when inheriting from `Composite`. I believe the classes `CompositeCT` and `CompositeRn` are not derived using the `virtual` keyword, then I would have a diamond problem in the `CompositieCTRn`. –  May 27 '15 at 13:22
  • My bad I missed that. Have you solved your problem? If you have some debug information from gdb or valgrind please share them – Brahim May 27 '15 at 14:14
  • I've looked at your code. Other than a nice bouquet of design smells I don't see anything wrong with it, really. If you need more help, you'll have to start being specific about versions, flags, compilers, architecture... etc. – sehe May 27 '15 at 20:43

1 Answers1

0

Well. I'm thinking you're really pushing the type system. I cannot begin to fathom why you would want this egregious type hierarchy:

enter image description here

That's a lot of damage done in ~60 lines of code. I cannot think of a reasonable situation where Liskov Substitution Principle holds for this hierarchy.

I also note that this kind of emphasis on runtime polymorphism does seem to run counter to the design goals of C++ and uBlas in particular.

Especially this line (declaring the center of the diagram, essentially):

class CompositeCTRn : public CompositeCT<ublasRn<double> >, public CompositeRn<double> {

This effectively declares a class with a single virtual base, which is "aliased" via three bases:

using VBase  = Composite<double, ublasRn<double> >;
using CTBase = CompositeCT<ublasRn<double> >;
using RnBase = CompositeRn<double>;

This means there should be only one _i1 and one _i2 member in the type. That means that on all conforming compilers, the following should compile and run without triggering any asserts or causing memory leaks etc.:

class CompositeCTRn : public CompositeCT<ublasRn<double> >, public CompositeRn<double> {
    using VBase  = Composite<double, ublasRn<double> >;
    using CTBase = CompositeCT<ublasRn<double> >;
    using RnBase = CompositeRn<double>;
public:
    CompositeCTRn() : VBase(), CTBase(), RnBase() {
        VBase::_i1 = 1;
        VBase::_i2.reset(new ublasRn<double>(3));

        CTBase::_i1 = 2;
        CTBase::_i2.reset(new ublasRn<double>(3));

        RnBase::_i1 = 3;
        RnBase::_i2.reset(new ublasRn<double>(3));

        assert(3 == VBase::_i1);
        assert(3 == CTBase::_i1);
        assert(3 == RnBase::_i1);

        assert(VBase::_i2.get() == CTBase::_i2.get());
        assert(VBase::_i2.get() == RnBase::_i2.get());

        RnBase::_i2.reset();
        assert(!(VBase::_i2 || CTBase::_i2 || RnBase::_i2));
    };
};

In fact, that's exactly what happens on gcc 4.8, 4.9, 5.0 and clang++ 3.5.

Live On Coliru

#include <boost/numeric/ublas/vector.hpp>

namespace ublas = boost::numeric::ublas;

template <typename T = double> using ublasRn = ublas::vector<T>;

struct Base {
    virtual ~Base() {}
};

template <typename T1, typename T2> class Composite : public Base {
  protected:
      template <typename, typename, typename> friend class Trajectory; // or make this public
      using Type1 = T1;
      using Type2 = T2;

      Type1 _i1;
      std::unique_ptr<Type2> _i2;

  public:
      Composite() = default;
};

template <typename T2> class CompositeCT : virtual public Composite<double, T2> {
  public:
    using Composite<double, T2>::Composite;
};

template <typename T1> class CompositeRn : virtual public Composite<T1, ublasRn<double> > {
  public:
    using Composite<T1, ublasRn<double> >::Composite;
};

class CompositeCTRn : public CompositeCT<ublasRn<double> >, public CompositeRn<double> {
    using VBase  = Composite<double, ublasRn<double> >;
    using CTBase = CompositeCT<ublasRn<double> >;
    using RnBase = CompositeRn<double>;
  public:
    CompositeCTRn() : VBase(), CTBase(), RnBase() {
        VBase::_i1 = 1;
        VBase::_i2.reset(new ublasRn<double>(3));

        CTBase::_i1 = 2;
        CTBase::_i2.reset(new ublasRn<double>(3));

        RnBase::_i1 = 3;
        RnBase::_i2.reset(new ublasRn<double>(3));

        assert(3 == VBase::_i1);
        assert(3 == CTBase::_i1);
        assert(3 == RnBase::_i1);

        assert(VBase::_i2.get() == CTBase::_i2.get());
        assert(VBase::_i2.get() == RnBase::_i2.get());

        RnBase::_i2.reset();
        assert(!(VBase::_i2 || CTBase::_i2 || RnBase::_i2));
    };
};

template <typename T1, typename T2, class TComposite> class Trajectory : public Base {
    static_assert(std::is_same<T1, typename TComposite::Type1>::value, "mismatched composite");
    static_assert(std::is_same<T2, typename TComposite::Type2>::value, "mismatched composite");
  protected:
    std::vector<std::unique_ptr<TComposite> > m_Trajectory;

  public:
    Trajectory() { checkType(); }
    void checkType() const {
        std::vector<TComposite> CI(1000);
    }
    virtual ~Trajectory() {}
};

#include <iostream>

int main() {
    Trajectory<double, ublasRn<>, CompositeCTRn> T;
    std::cout << 123 << "\n";
}

Under valgrind:

==15664== Memcheck, a memory error detector
==15664== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==15664== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright info
==15664== Command: ./test
==15664== 
123
==15664== 
==15664== HEAP SUMMARY:
==15664==     in use at exit: 0 bytes in 0 blocks
==15664==   total heap usage: 6,001 allocs, 6,001 frees, 184,000 bytes allocated
==15664== 
==15664== All heap blocks were freed -- no leaks are possible
==15664== 
==15664== For counts of detected and suppressed errors, rerun with: -v
==15664== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

No problem at all.

Notes

  • I replaced the type check with a static assert (because, why not?!)

    template <typename T1, typename T2, class TComposite> class Trajectory : public Base {
        static_assert(std::is_same<T1, typename TComposite::Type1>::value, "mismatched composite");
        static_assert(std::is_same<T2, typename TComposite::Type2>::value, "mismatched composite");
    

Summary / TL;DR

I've looked at your code. Other than a nice bouquet of design smells I don't see anything wrong with it, really. If you need more help, you'll have to start being specific about versions, flags, compilers, architecture...

sehe
  • 374,641
  • 47
  • 450
  • 633
  • Thank you for the detailed answer. However, I would also appreciate any further advice on the subject of the design of the class hierarchy structure. The "diamond" that I created seems to be intuitive for the application. I would like to provide a generic base class (i.e. `Composite`) that stores time and state, both of which can be any types. I would also like to provide some specific functionality for particular types of the time and space (e.g. `CompositeCT` and `CompositeRn`) and provide an ability to combine this functionality in an arbitrary fashion (e.g. `CompositeCTRn`). –  May 27 '15 at 21:48
  • Static polymorphism and a modicum of meta programming are likely more clear, and a **lot** more efficient. I can't tell from the flimsy description here, though. – sehe May 27 '15 at 21:50
  • I am familiar with static polymorphism and would gladly accept any advice. A more detailed example of what I am trying to achieve is available here: [link](http://stackoverflow.com/questions/30397068/c-pointer-being-freed-was-not-allocated-possibly-an-issue-with-unique-ptr-or). I would gladly ask an explicit question and provide more details if you will answer it (from my experience, the questions of this nature are often left without an answer). Please let me know. –  May 27 '15 at 22:16
  • @user1391279 That seems genuine. I'll look at the link later. You're right that you should have a concrete question if you expect people to invest. [codereview.SE] is nice sometimes – sehe May 27 '15 at 22:17