Sign in to follow this  
Kwizatz

From derived class to void* to base class bug.

Recommended Posts

I have a weird bug which may not be so weird. I have a base class "Placeable" which manages matrix transformations and a Mesh Instance class that inherits from "Placeable" and manages mesh information. In order to expose the object to Lua, I have to do the equivalent of this:
*meshinstance = new MeshInstance;
void* voidptr = (void*) *meshinstance;

// Later

Placeable* placeable = (Placeable*) voidptr;




This, doesn't work for these particular classes, it works for all other classes in my engine, and it even worked for these before I removed my scene graph. The problem I am having is that for some reason when the meshinstance object is created in memory, it has some sort of element in front of the matrix, this seems to be handled well when down casting from MeshInstance to Placeable, but when going from void to Placeable, the matrix is offset by 4 bytes to the left, and chaos ensues. When the void pointer is cast into a MeshInstance, there are no issues. Can anyone with some more knowledge of C++ help me out here? what can I do? what am I missing? I find it really odd that 2 other classes inherit from Placeable and use the same function that makes the cast, but show no issues at all. I think this may be some sort of alignment issue, but I don't really know, could be a Visual C++ 2005 bug too. [sad] Thanks in Advance. [Edited by - Kwizatz on August 17, 2009 2:42:13 AM]

Share this post


Link to post
Share on other sites
Are you using multiple inheritance by any chance?

In the following snippet, the pointers itest and otest will point to different adresses becauses the base offset for the class-specific fields in the object is different:

int main() {
fstream test;

istream *itest = &test;
ostream *otest = &test;
}


You can use a dynamic_cast<> instead of the C-style cast in this case, at a small performance penalty.

Share this post


Link to post
Share on other sites
You should be using c++ type of cast instead of old c style cast. You should probably be using static_cast in this example for efficiency reasons instead of dynamic_cast.

static_cast will adjust the basepointer so that it points to the correct class.But there are times when this might fail, like for instance when dealing with multiple inheritance. Then you will need to use dynamic_cast, which is significantly slower, but it also does runtime checking of the cast and returns a nullpointer if the cast cannot be made.

Try this:


*meshinstance = new MeshInstance;
void* voidptr = static_cast<void*>(*meshinstance);

// Later

Placeable* placeable = static_cast<Placeable*>(voidptr);


Pointers to the same instance of an object but with different types can differ. C type casts does not always adjust for this, but static_cast and dynamic_cast should.

A static_cast to void* returns the beginning of allocated memory for that object.

You should read up in c++ casts (static_cast, dynamic_cast, const_cast and reinterpret_cast) and use them appropriately instead of old unpredictable c type casts.

Share this post


Link to post
Share on other sites
The only safe conversion is T* --> void* --> T*. Conversions like T* --> void* --> U* do not always work, even if there is an inheritance relationship that would allow T* --> U*, because such conversions sometimes require additional knowledge that is not available if you go through void*.

Although I can understand that you need a cast to void* in order to work with Lua, you still should know in your own mind what type the void* actually represents, and make the appropriate conversions. For instance, if the void* is intended to be a Placeable*, then you should write:

MeshInstance *meshinstance = new MeshInstance;
void* voidptr = static_cast<void*>(static_cast<Placeable*>(meshinstance));

Share this post


Link to post
Share on other sites
Alright, thanks I'll use C++ casts and see how it goes, I am not using multiple inheritance, though a Character class is derived from MeshInstance, I saw the problem originally on that class, and I though maybe that was the problem, but it shows on MeshInstance as well, which is a direct descendant of Placeable and Placeable is not derived from any class.

Anyway, Thanks!.

Share this post


Link to post
Share on other sites
Well, it doesn't seem to work, the casts do the same thing as before and the problem with down casting before storing the address is that I will also need to up cast to the original class, so its the same problem [sad].

So what I am going to do is create a struct with pointers to all possible classes and store that instead IE:


struct Pointers
{
Placeable* placeable;
MeshInstance* meshinstance;
Character* character;
};

Pointers pointers;
MeshInstance* meshinstance = new MeshInstance;
pointers.placeable = (Placeable*) meshinstance;
pointers.meshinstance = meshinstance;
pointers.character = NULL;

// Later
Placeable* placeable = pointers.placeable;



Otherwise I'd have to copy functions over from Placeable to its derived classes, making its existence moot.

I guess I could turn it into a template instead of a base class too.

Share this post


Link to post
Share on other sites
I'm not a C++ guru so I may be way out in left field on this, but wouldn't a union serve you better than a struct, if that's the approach you're going to take?

Share this post


Link to post
Share on other sites
Quote:
Original post by Kwizatz
Well, it doesn't seem to work, the casts do the same thing as before and the problem with down casting before storing the address is that I will also need to up cast to the original class, so its the same problem [sad].
Do you mean this doesn't work:

MeshInstance *mesh = new MeshInstance();
void *voidptr = static_cast<void *>(static_cast<Placeable *>(mesh));

//...

Placeable *placeable = reinterpret_cast<Placeable *>(voidptr);


? As far as I can see, that should work just fine. You could then use dynamic_cast to get back to your MeshInstance.

Share this post


Link to post
Share on other sites
Try this code to ensure there's nothing weird going on, debugging step by step:


Placeable *placeable = 0;
MeshInstance *meshinstance = new MeshInstance();

void* voidptr = (void*)meshinstance;

placeable = (Placeable*) voidptr;

if( placeable != meshinstance )
printf( "Error 01" );

placeable = dynamic_cast<Placeable*>(voidptr);

if( placeable != meshinstance )
printf( "Error 02" );

placeable = static_cast<Placeable*>(reinterpret_cast<MeshInstance*>(voidptr));

if( placeable != meshinstance )
printf( "Error 03" );

placeable = reinterpret_cast<Placeable*>(voidptr);

if( placeable != meshinstance )
printf( "Error 04" );

printf( "Test ended" );




Your code should work. Because it isn't working, you either doing something very complex or you have a serious bug somewhere else.

Cheers
Dark Sylinc

Edit: Fixed a cast, it wouldn't compile. Added test 04

Share this post


Link to post
Share on other sites
Quote:
Original post by Dragon88
I'm not a C++ guru so I may be way out in left field on this, but wouldn't a union serve you better than a struct, if that's the approach you're going to take?


No, an union uses the same memory location for all of its members, so it would be equivalent to setting all of its members to the same pointer, no improvement over the void*.

I decided against this anyway, although tedious, it would be better to use Lua's own metatables for type checking.

Share this post


Link to post
Share on other sites
Quote:
Original post by Codeka
Do you mean this doesn't work:


No, I tried the static cast just on the later part of the code, I will try that, I am not that familiar with these casts.

Thanks.

Edit:

Ok, I went back and tried what you suggested, casting the MeshInstance down to Placeable before storage works, but using dynamic_cast as follows:


MeshInstance* meshinstance = dynamic_cast<MeshInstance*>(placeable);



throws the error:

error C2683: 'dynamic_cast' : 'Placeable' is not a polymorphic type

its odd because if I do:


MeshInstance* meshinstance = (MeshInstance*)(placeable);



Everything works fine... until its time to delete the object, then this exception is thrown:

Quote:

Unhandled exception at 0x004f5591 in AeonEngine.exe: 0xC0000005: Access violation reading location 0x3f800000.


Which makes sence since I am calling delete on the memory address of the Placeable base class, not the original MeshInstance address.


[Edited by - Kwizatz on August 16, 2009 7:53:41 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Matias Goldberg
Your code should work. Because it isn't working, you either doing something very complex or you have a serious bug somewhere else.


That's what I though [smile], after all derived classes should append their member variables after those of the base class right?

Anyway, I am getting

error C2681: 'void *' : invalid expression type for dynamic_cast

at

placeable = dynamic_cast<Placeable*>(voidptr);

on your test.

Edit: Test 1 fails, the pointers aren't the same, which is what I noticed before starting the thread, I commented the other tests.

Share this post


Link to post
Share on other sites
Ok, to elaborate a bit further on the complexity of this, I am posting the actual functions that I am using, first there is a static member function to create the instance of MeshInstance:


// the static keyword is in the class declaration, but its added here for completeness
static int MeshInstance::New(lua_State* L)
{
// this returns a chunk of memory to store user data, in this case, I just request for enough space for a MeshInstance pointer, hence the pointer to pointer.
MeshInstance **meshinstance = (MeshInstance **)lua_newuserdata(L, sizeof(MeshInstance*));
// create the instance
*meshinstance = new MeshInstance;
// assign the metatable to the user object
luaL_getmetatable(L, "AeonEngine.MeshInstance");
lua_setmetatable(L, -2);
}



So far, pretty simple, then I have a SetPosition function in Placeable that is exposed to Lua, so when the function is called on a MeshInstance, or any other class derived from placeable the following function is called:


static int Placeable::SetPosition(lua_State* L)
{
// returns the same pointer we created before to the chunk of data,
// lua_touserdata returns void*
Placeable** placeable = (Placeable**)lua_touserdata(L,1);
// Call the Placeable::SetPosition() function,
// but the pointer to the actual object is bad, points to an address 4 bytes short of the actual Placeable object data.
(*placeable)->SetPosition((float) lua_tonumber(L,2),(float) lua_tonumber(L,3),(float) lua_tonumber(L,4));
return 1;
}



Now, I am pretty sure the problem is not on those functions because if I do this in the first one:


static int MeshInstance::New(lua_State* L)
{
MeshInstance **meshinstance = (MeshInstance **)lua_newuserdata(L, sizeof(MeshInstance*));
*meshinstance = new MeshInstance;
void* voidptr = (void*) *meshinstance;
Placeable* placeable = (Placeable*) voidptr;
luaL_getmetatable(L, "AeonEngine.MeshInstance");
lua_setmetatable(L, -2);
}



The placeable variable is just as bad as the one retrieved later.

Share this post


Link to post
Share on other sites
Quote:
Original post by Kwizatz
Quote:
Original post by Dragon88
I'm not a C++ guru so I may be way out in left field on this, but wouldn't a union serve you better than a struct, if that's the approach you're going to take?


No, an union uses the same memory location for all of its members, so it would be equivalent to setting all of its members to the same pointer, no improvement over the void*.


It has the same improvements over void* that your structure did. All you did with your structure is a C-style cast to the different type pointers. So you end up with 3 elements in the structure all carrying the same address (unless I've lost my mind somewhere along the way and C-style casts resolve to different addresses for different types). A union would be indentical, except you'd only have to write one value to it. The way the address is interpreted is going to depend on the type of the pointer, but that's going to be the same with the struct and the union approaches.

In short I don't think either of them works for what you're trying to do, if you're going to use C-style casts.

Share this post


Link to post
Share on other sites
Quote:
Original post by Dragon88
It has the same improvements over void* that your structure did. All you did with your structure is a C-style cast to the different type pointers. So you end up with 3 elements in the structure all carrying the same address (unless I've lost my mind somewhere along the way and C-style casts resolve to different addresses for different types).


In this case, that is exactly what is happening, the 2 pointers point at different addresses within the same object:


MeshInstance* meshinstance = new MeshInstance; // lets say new returns address 0x00000048
Placeable* placeable = (Placeable*) meshinstance; // placeable points to 0x0000004c WTF? [headshake]
void* voidptr = (void*) meshinstance; // voidptr points to 0x00000048
placeable = (Placeable*) voidptr;
// now placeable points to 0x00000048
//but it is off, if I change matrix[12] using this pointer,
// I really change matrix[11], and matrix[0] contains garbage from the point of view of Placeable.




I tried to reproduce it with a small program, but was unable to.

Share this post


Link to post
Share on other sites
EDIT: Replaced the code with an example that doesn't use multiple inheritance.
#include <iostream>

class Placeable {
char c;
};

class MeshInstance : public virtual Placeable {
};

int main() {
MeshInstance *mi = new MeshInstance;
std::cout << (void *)mi << '\n';
Placeable *p = (Placeable *)mi;
std::cout << (void *)p << '\n'; // This prints something different!
}



Does that show the "problem" you are talking about?

There is nothing to see here. You are doing things with pointers that are not supposed to work, and they are not working.

Share this post


Link to post
Share on other sites
Quote:
Original post by alvaro
Does that show the "problem" you are talking about?


Yes, it does, I guess I couldn't reproduce it because I wasn't adding "virtual" to the derived class.

I'll just use Lua's metatables to decipher the object type, I am guessing using "placement new" will show the same symptoms, am I right?

I'll just modify your test program to make sure,

Thanks!

Edit:
I'd still like to know why this happens, if you care to explain [smile].

More Edit:

placement new does the same, no way out of checking Lua metatables, though it may be useful to get rid of the double pointer, oh well, thanks again.


#include <iostream>

class Placeable {
char c;
};

class MeshInstance : public virtual Placeable {
};

int main()
{
MeshInstance *mi;
Placeable *p;
void* voidMeshInstance = malloc(sizeof(MeshInstance));
mi = new (voidMeshInstance) MeshInstance();
p = (Placeable *)mi;
std::cout << (void *)mi << '\n';
std::cout << (void *)p << '\n'; // This prints something different!
mi->~MeshInstance();
free(voidMeshInstance);
}


Share this post


Link to post
Share on other sites
The way virtual inheritance is laid out, you'll basically pointer to the base class as part of the data structure. In plain vanilla inheritance, the pointer points just a few bytes into the structure, where you'll find the instance of the base class. In more complicated cases (e.g., the diamond multiple inheritance configuration), several of these pointers could point to the same place.

Could you do this?
void* voidptr = (void*) (Placeable*) *meshinstance;

Share this post


Link to post
Share on other sites
Quote:
Original post by Kwizatz
throws the error:

error C2683: 'dynamic_cast' : 'Placeable' is not a polymorphic type

its odd because if I do:

*** Source Snippet Removed ***

Everything works fine... until its time to delete the object, then this exception is thrown:


If you derive classes and handle them through base class pointers, ALWAYS make the destructor virtual in the base class. You also need to have at least one virtual function in your base class for them to be polymorphic anyway. Guess which one is the best candidate? ,-)

Multiple virtual inheritance might interfere in some ways, though if you do make it polymorphic and use the correct C++ casts I'd consider any compiler not handling it correctly as suspicious. But whatever you do, I would not cast the void* brute force to the derived class, because I wouldn't be sure that this whole multiple inheritance might not add some meta-info somewhere in the beginning or result in a structure that simply doesn't look as you expect.

Make it polymorphic, cast void* to base* (or whatever you originally casted to void*) and dynamic_cast to derived*. If that doesn't work you can at least say you tried to make it as clean as possible.

Share this post


Link to post
Share on other sites
Quote:
Original post by Kwizatz
I'd still like to know why this happens, if you care to explain [smile].


Ok. I managed to reproduce your issue in a test code.

I know what's going on.
The problem arises when the Derived classes uses at least one virtual function, but the Base class doesn't.
When doing the unsafe conversion, the compiler automatically "warps" (offsets) the pointer. But the "warp" has to be done in the same order. The order is up to you, and that's why it's considered unsafe.

The following class will cause your problem:

class Base
{
protected:
int myVar;

public:
Base() : myVar(1)
{
}

~Base()
{
}
};

class Derived : public Base
{
protected:
int myVar;
public:
Derived() : myVar(0)
{
}

~Derived()
{
}

virtual void myFunc()
{
}
};





Here are some examples on usage:

//OK
Derived *A = new Derived(); //A points to 0x00000020
Base *B = A; //B points to 0x00000020 + 4 = 0x00000024
void *P = (void*)A; //P points to 0x00000020
Base *B2= (Derived*)P; //B2 points to 0x00000020 + 4 = 0x00000024
Derived *A2= (Derived*)P; //A2 points to 0x00000020

//WRONG:
Base *B2= (Base*)P; //B2 points to 0x00000020!!





Alternatively, you can store P using B (which is even worse):

//OK
Derived *A = new Derived(); //A points to 0x00000020
Base *B = A; //B points to 0x00000020 + 4 = 0x00000024
void *P = (void*)B; //P points to 0x00000024
Base *B2= (Base*)P; //B2 points to 0x00000024
Derived *A2= (Derived*)(Base*)P; //A2 points to 0x00000024 - 4 = 0x00000020

//WRONG:
Base *B2= (Derived*)P; //B2 points to 0x00000024 + 4 = 0x00000028!!
Derived *A2= (Derived*)P; //A2 points to 0x00000024!!





In the end, you'll see it's up to you to mantain safety: You have to know how it was casted when it was stored as a VOID, and now how you need to cast it back.

In your particular case, the bug should be fixed by doing:

*meshinstance = new MeshInstance;
void* voidptr = (void*)*meshinstance;

// Later

Placeable* placeable = (MeshInstance*) voidptr; //Note how I changed the casting





Hope this clears your doubts and be usefull to you
Cheers
Dark Sylinc

Edit: This problem won't happen if both base and derived classes have virtual memeber functions. (At least on x86 Windows environments)

[Edited by - Matias Goldberg on August 17, 2009 10:24:42 AM]

Share this post


Link to post
Share on other sites
Thank you all, I'll reply to all of you on this same post [smile].

alvaro:

Thanks, I did some tests and found out the problem had to do with missing virtual declarations, I did what you told me to before, resulted in exceptions when the time came for deletion.

Trienco:

Yes, you're correct, when I had the scene graph these kind of functions and some others were part of the "Node" class, that class had virtual Update and Render functions and when I removed the node class I moved the necessary functions for transformation into Placeable... none of them required overriding so no virtuals, I'll add the virtual destructor, which I also overlooked since it does nothing.

Matias Goldberg:

Thank you for the through explanation, read above as for why you both are correct, and I am a dummy [lol], this really took me by surprise because no issues showed when I had Node->MeshInstance->Character, and I couldn't find exactly what I was doing different, like an if(walking=true) statement.

Your solution (Placeable* placeable = (MeshInstance*) voidptr) wouldn't really work because there is no warranty that the pointer points to a MeshInstance, it could be a Character, which, granted is derived from MeshInstance, but could be also BSPInstance which is derived from Placeable but not MeshInstance or Camera which is also only derived from Placeable.

Again, thanks to all.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this