9

I am having a bit of a struggle with Microsoft Visual C++ 2015 and was able to replicate the issue with a small program. Given the following classes:

class BaseClass {
public:
    BaseClass()
        : mValue( 0 )
        , mDirty( true )
    {}
    virtual ~BaseClass() {}
    virtual int getValue() const { if( mDirty ) updateValue(); return mValue; }

protected:
    virtual void updateValue() const = 0;

    mutable bool mDirty;
    mutable int  mValue;
};

class DerivedClass : public BaseClass {
public:
    DerivedClass() {}

protected:
    void updateValue() const override
    {
        mValue++;
        mDirty = false;
    }
};

class Impersonator {
public:
    Impersonator() {}

    // conversion operator
    operator DerivedClass() const
    {
        return DerivedClass();
    }

    // conversion method
    DerivedClass toDerived() const
    {
        return DerivedClass();
    }
};

I get a "pure virtual function call" error when I do the following:

void use( const BaseClass &inst )
{
    // calls `getValue` which in turns calls the virtual function 'updateValue'
    int value = inst.getValue();
}

int main()
{
    // creates a temporary, then passes it by reference:
    use( DerivedClass() ); // this works

    // calls conversion operator to create object on stack, then passes it by reference:
    DerivedClass i = Impersonator();
    use( i ); // this works

    // calls conversion method to create a temporary, then passes it by reference:
    use( Impersonator().toDerived() ); // this works

    // calls conversion operator to create a temporary, then passes it by reference:
    Impersonator j = Impersonator();
    use( j ); // causes a pure virtual function call error!

    return 0;
}

Given that I can't change the void use(const BaseClass&) function, can I change anything in the Impersonator class to allow using the last call without generating a debug error?

Paul Houx
  • 1,984
  • 1
  • 14
  • 16
  • Are your run-time components in Visual Studio is OK? Maybe some missing components cause that problem. – stuck Aug 01 '16 at 17:24
  • 1
    If you breakpoint inside the last call to `getValue` and inspect the vtable pointer, MSVC thinks you have a `BaseClass` object, which looks incorrect. – Praetorian Aug 01 '16 at 17:26
  • http://stackoverflow.com/questions/99552/where-do-pure-virtual-function-call-crashes-come-from – Sergio Lopes Aug 01 '16 at 17:27
  • 2
    [Cannot reproduce](http://coliru.stacked-crooked.com/a/f408c11a34d128f4). MVC considered bad. – Shoe Aug 01 '16 at 17:29
  • GCC 4.8.3 gives error: *error: cannot allocate an object of abstract type ‘BaseClass’*. – ach Aug 01 '16 at 17:43
  • Sorry about bad dupe closing, I was sure the virtual function was called from the destructor. – n. m. could be an AI Aug 01 '16 at 17:52
  • 3
    Inspecting the resulting assembly shows that for some reason or other MSVC decides to call `BaseClass::BaseClass` to copy the temporary returned from `operator DerivedClass` despite `BaseClass` being abstract. Explicit declaration of the copy constructor as non-public makes MSVC complain: *error C2248: 'BaseClass::BaseClass' : cannot access protected member declared in class 'BaseClass'*. – ach Aug 01 '16 at 17:58
  • Looks as if MSVC decided to apply the rules related to binding temporaries to named constant references, only without precaution. The following fragment causes a proper error about `BaseClass` being abstact: `Impersonator j = Impersonator(); BaseClass const& k = j; use( k );` – ach Aug 01 '16 at 18:02
  • And yet again, using another function that returns a `DerivedClass` object produces the expected code; copy constructor is omitted entirely. – ach Aug 01 '16 at 18:09
  • @AndreyChernyakhovskiy If you make `BaseClass::updateValue` non-pure and feed the program to the old gcc, `BaseClass::updateValue` [gets called](http://ideone.com/0JheaG). It looks like the old gcc and the new MSVC somehow share a bug. – n. m. could be an AI Aug 01 '16 at 18:46
  • @n.m., I am not sure about it being a bug. My attempted investigation makes me incline to the conclusion that it is, but C++ being so [fine](http://www.gpwa.org/forum/images/imported/2014/09/singaporefinecity450x600-1.jpg) language, it is quite hard to be sure. – ach Aug 01 '16 at 18:51
  • 2
    @AndreyChernyakhovskiy I don't know. I have constructed a small [test case](http://ideone.com/rkjccT). Old gcc and msvc print "gotcha", new gcc and clang do not. Why do old gcc and msvc want to copy-construct Base out of Derived? – n. m. could be an AI Aug 01 '16 at 19:43
  • 3
    @AndreyChernyakhovskiy MSVC is most definitely in error since BaseClass is abstract and objects of this class should never be created. If copy construction is somehow the correct action (which seems totally incredible to me) MSVC should complain that BaseClass is abstract, not silently proceed to copy-construct it. – n. m. could be an AI Aug 01 '16 at 20:12
  • @n.m. Ah, I was just trying to joke on the complexity of C++. ☺ – ach Aug 01 '16 at 20:15
  • I am happy to see I am not the only one puzzled by MSVC's behavior. Should I report this to the Microsoft team? – Paul Houx Aug 01 '16 at 21:04
  • Partial solution: if I use a method on `Impersonator` to create a `DerivedClass`, instead of a conversion operator, it works: `DerivedClass toDerived() const { return DerivedClass(); }`. I'll add this to the sample code. – Paul Houx Aug 01 '16 at 21:13
  • @Paul One way or another it's a bug and, yes, it would be worth reporting on [VS Connect](https://connect.microsoft.com/visualstudio). – dxiv Aug 01 '16 at 21:30
  • If you add `protected: BaseClass(const BaseClass& r) { throw 1; }` compilation fails on that line with `error C2248: 'BaseClass::BaseClass': cannot access protected member ` – Ben Aug 01 '16 at 21:35
  • Apologies to @AndreyChernyakhovskiy I've duplicated your comments exactly :-) – Ben Aug 01 '16 at 21:42

2 Answers2

2

The only way to mitigate the problem that I see is to add an operator const BaseClass&() to Impersonator and have it return a reference to DerivedClass.

This will create a better conversion than the problematic/erroneous one the compiler is trying to use.

Naturally Impersonator won't be able to return by value and create a temporary, so it will have to own a DerivedClass object, or many objects, and dispose them somehow at an appropriate time. The simplest way that works for this demo program is to have it return a reference to its data member, but a real program may have to do something else.

class Impersonator {
public:
    Impersonator() {}

    // conversion operator
    operator DerivedClass()
    {
        return d;
    }
    operator const BaseClass&()
    {
        return d;
    }

private:
    DerivedClass d;
};
n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243
  • Yes, that works. Unfortunately the "impersonator" in the actual application is a LightSource class trying to impersonate a Camera and its data members should strictly follow the std140 layout rules, so I can't add a DerivedClass data member. I could create a static one in the conversion operator, but then only one such "temporary" can be active and it's also not thread safe. Thanks for your input, though. – Paul Houx Aug 01 '16 at 21:08
  • Partial solution: if I use a method on `Impersonator` to create a `DerivedClass`, instead of a conversion operator, it works: `DerivedClass toDerived() const { return DerivedClass(); }`. I'll add this to the sample code. – Paul Houx Aug 01 '16 at 21:25
  • You can allocate DerivedClass objects dynamically, and place them in a thread-local container. Clean it up when you know no DerivedClass objects can possibly be needed. Or, if you can wrap `use`, delete the dynamic object in the wrapper. – n. m. could be an AI Aug 01 '16 at 21:33
1

This is a workaround. Create a wrapper for use which accepts a const DerivedClass&.

//I get a "pure virtual function call" error when I do the following :
void use(const BaseClass &inst)
{
    // calls `getValue` which in turns calls the virtual function 'updateValue'
    int value = inst.getValue();
}

void use(const DerivedClass &inst) {
    use(static_cast<const BaseClass&>(inst));
}

The better match means the workaround wrapper will be selected, so a temporary of the correct type will be created, and a reference to that passed to the real use implementation.

Ben
  • 34,935
  • 6
  • 74
  • 113