views:

96

answers:

4

I've been messing about with member-function pointers in relation to a previous question. In the code below I call methods on a class (B) that change a variable (count) in it, but I never make an instance of this class. Why does this work?

#include <iostream>
#include <string>
#include <map>

class A;
typedef int (A::*MEMFUNC)(int, int);

#define HANDLER(aclass, aproc) (MEMFUNC)(&aclass::aproc)

enum
{
    ADD=1,
    MUL,
    SUB,
    DIV
};

class B
{
    int count;
public:
    B() : count(0) {}
    ~B() {}
    int multiply(int x, int y) { count++; return x*y*count; }
    int divide(int x, int y) { count++; if (y!=0) return (x/y)*count; else return 0; }
};

class A
{
    std::map< int, MEMFUNC > funcs;
public:
    A() { AddLocals(); }
    ~A() {}
    int CallLocal(int nID, int x, int y)
    {
        MEMFUNC f = funcs[nID];
        if (f) return (this->*f)(x, y);
        else return 0;
    }
    void AddLocals()
    {
        Add(ADD, HANDLER(A, plus));
        Add(MUL, HANDLER(B, multiply));
        Add(SUB, HANDLER(A, subtract));
        Add(DIV, HANDLER(B, divide));
    }
    void Add(int nID, MEMFUNC f) { funcs[nID] = f; }
    int plus(int x, int y) { return x+y; }
    int subtract(int x, int y) { return x-y; }

};

int main()
{
    A aA;
    int a,b,c,d;

    a = aA.CallLocal(ADD,8,2);
    b = aA.CallLocal(MUL,8,2);
    c = aA.CallLocal(SUB,8,2);
    d = aA.CallLocal(DIV,8,2);

    std::cout << "a = " << a << "\n" 
              << "b = " << b << "\n" 
              << "c = " << c << "\n" 
              << "d = " << d << "\n";


    return 0;
}

(sorry, me again, but this member-function pointers are making me itch)

+9  A: 

Your cast in the HANDLER macro def tells the compiler "Shut up! I know what I'm doing!".

So the compiler shuts up.

You still have Undefined Behavior, but one property of UB is that in some cases it does what you'd naïvely expect, or what you'd want it to do.

But don't be surprised if such code crashes, or causes crashes or mysterious incorrect result in apparently totally unrelated code.

Or, for example, causes nasal demons to fly out your nose.

Cheers & hth.

Alf P. Steinbach
+1 for nasal daemons
Armen Tsirunyan
LOL, yes, I'm wondering why using the this-pointer in A successfully calls members in B. 'T'is passing strange ...
slashmais
The function pointers should be 'wild', but they're calling the correct code. Your answer does not explain this.
slashmais
@slashmais: In practice, the "correct" code gets executed, and it returns the "correct" result, because the function has the same signature as the typedef. But it will also corrupt the stack without telling you.
Oli Charlesworth
@slashmais: The answer is entirely correct. Your code is unsafely casting a member function of `B` to a member function of `A`, then calling it on an instance of `A`. Treating an object of one type as an object of an incompatible type gives undefined behaviour, which could do anything at all. What it's almost certainly doing here is corrupting `aA.funcs`, which could be harmless, or could cause all kinds of bad behaviour. You should avoid casts unless you need them and can prove that they're correct, and avoid C-style casts like the plague - this question is a good example of why.
Mike Seymour
@Mike Seymour: visitor came up with the same answer, what remains now is where does it find the code it executes? might it be that, since it is such simple functions, that they got inlined?
slashmais
@slashmais: We're in the realms of undefined behaviour, so there's no guaranteed mechanism for finding the code. However, the member function pointer probably points to the expected code; it's just called with `this` pointing to an object of the wrong type. But there's no guarantee that another compiler will do the same thing; your code is invalid, and its behaviour is undefined.
Mike Seymour
+1  A: 

C-casting lets you get away with all sorts of horrid behaviour but doesn't mean it's ok to do it so simply don't.

Get rid of your macro completely and do not cast. You can probably use boost::function and boost::bind to get the behaviour you actually want.

CashCow
+1  A: 

The result is just undefined behavior. For example, I get that b = 2083899728 and d = -552766888.

The persistent thing you are manipulating is most likely an int's worth of bytes in the map instance of A (because if the object were indeed a B, then that's the offset where the count member would be located.

In my stdlib implementation, the first member of map is the comparison function, in this case an instance of std::less<int>. Its size is 1, but there must be unused padding bytes after that to align the other members of map. That is, (at least) the first four bytes of this instantiation of std::map contains just garbage that is not used for anything (std::less doesn't have data members and doesn't store state, it just takes space in the map). That would explain why the code doesn't crash - it is modifying a part of the map instance which doesn't affect the functioning of the map.

Add more data members in B before count, and now count++ will affect crucial parts of the map's internal representation and you can get a crash.

visitor
slashmais
At machine level it's just bytes and offsets. The machine basically interprets a call to `B::multiply` as "increment an integer at offset 0 (as `count` is the first member) from the `this` pointer, then multiply it by the values of the function arguments and return the result". Except you don't have a B instance, and the bytes it uses are not really the `count` member of a B instance.
visitor
+1  A: 

Your code is invoking undefined behaviour by trying to call a member of class B using an object of class A. We can try to explain how the compiler can come to the behaviour you have observed, but there is no guarantee that you will get the same behaviour if you change anything (add/remove a member, change compiler settings or use a different compiler).

With the cast in the HANDLER macro, you are telling the compiler not to warn you about the use of incompatible types but just to do as you tell it to. In this case, you tell the compiler to reinterpret the address of a member of any class as being the address of a member of class A.

When you later try to call, for example, B::multiply, that function does not know that it is not working on an object of class B, so it will happily clobber the bytes of aA that would correspond to the B::count member if it had been a B object. Most likely, these bytes are actually being used by A::funcs, but apparently not for anything critical. If you change class A to:

class A
{
    int count;
    std::map< int, MEMFUNC > funcs;
public:
    A() : count(0) { AddLocals(); }
    ~A() {}
    int CallLocal(int nID, int x, int y)
    {
        MEMFUNC f = funcs[nID];
        if (f) return (this->*f)(x, y);
        else return 0;
    }
    int Count()
    {
        return count;
    }
    void AddLocals()
    {
        Add(ADD, HANDLER(A, plus));
        Add(MUL, HANDLER(B, multiply));
        Add(SUB, HANDLER(A, subtract));
        Add(DIV, HANDLER(B, divide));
    }
    void Add(int nID, MEMFUNC f) { funcs[nID] = f; }
    int plus(int x, int y) { return x+y; }
    int subtract(int x, int y) { return x-y; }

};

then printing the result of aA.Count() at various places might show the effect.

The compiler is calling the expected function, because they are non-virtual member functions.
The only difference between non-member functions and non-virtual member functions is in the hidden argument that feeds the this pointer in a member function. So, if you take the address of a non-virtual member function, you will get a fixed address that is distinct for every function.
If the member functions had been virtual, then the compiler would, most likely, have returned the index into the v-table as a pointer for that function (together with some kind of indication that it is a v-table offset). Then the code can determine at the call-site if it can do a direct call to the member function or if it needs to do an indirect call through the v-table of the object the function is called on.

Bart van Ingen Schenau