Is C++ compiler allowed to store function ptr in register before its arguments are evaluated?

user_7190562 0
Viewed 100

My collegues and I were fighting a rather weird bug in an app we're developing. Eventually we got it fixed, but we are still unsure if what the compiler was doing is legit or not.

Assuming we have code like this:

class B {
public:
    virtual int foo(int d) { return d - 10; }
};

class C : public B {
public:
    virtual int foo(int d) { return d - 11; }
};

class A {
public:
    A() : count(0) { member = new B;}
    int bar() {
        return member->foo(renew());
    }

    int renew() {
        count++;
        delete member;
        member = new C;
        return count;
    }
private:
    B *member;
    int count;
};

int square() {
    A a;
    cout << a.bar() << endl;
    return 0;
}

The Visual Studio x86 compiler, for function A::bar, generates something like this when compiled with /O1 (You can check the full code on godbolt):

        push    esi
        push    edi
        mov     edi, ecx
        mov     eax, DWORD PTR [edi]  ; eax = member
        mov     esi, DWORD PTR [eax]  ; esi = B::vtbl
        call    int A::renew(void)    ; Changes the member, vtable and esi are no longer valid
        mov     ecx, DWORD PTR [edi]
        push    eax
        call    DWORD PTR [esi]       ; Calls wrong stuff (B::vtbl[0])
        pop     edi
        pop     esi
        ret     0

Is this optimization allowed by the standard or is it an undefined behaviour? I was unable to get similar assembly with GCC or clang.

undefined-behavior visual-c++ c++
2 Answers

Just for perfect clarity, here's the Order of evaluation document Jarod42 already linked, and the relevant quote:

14) In a function-call expression, the expression that names the function is sequenced before every argument expression and every default argument.

So we should read the statement

return member->foo(renew());

as

return function-call-expression;

where function-call-expression is

{function-naming-expression member->foo} ( {argument-expression renew()} )

so, the function-naming-expression member->foo is sequenced-before the argument expression. The doc already linked says

If A is sequenced before B, then evaluation of A will be complete before evaluation of B begins.

so we have to completely evaluate member->foo first. I think it should expand like

// 1. evaluate function-naming-expression
auto tmp_this_member = this->member;
int (B::*tmp_foo)(int) = tmp_this_member->foo;

// 2. evaluate argument expression
int tmp_argument = this->renew();

// 3. make the function call
(tmp_this_member->*tmp_foo) ( tmp_argument );

... which is exactly what you see. This is the sequencing required by C++17, and prior to that the sequencing and behaviour were both undefined.

tl;dr the compiler is right, and that code would be nasty even if it worked.

user_2128580

Whereas order of evaluation is implementation specific prior to C++17, C++17 imposes some ordering, see evaluation order.

so in

this->member->foo(renew());

renew() might be called before evaluating this->member (prior C++17).

To guaranty order prior, C++17, you have to split into several different statement:

auto m = this->member;
auto param = renew(); // m is now pointing on deleted memory
m->foo(param);        // UB.

or, for the other order:

auto param = renew();
this->member->foo(param);
user_26845390
Related