12 replies to this topic

[Stainless]

In some code I am porting, they have used a table for setting parameters read from xml.

So the table is something like this

{"X",offsetof(Particle,X),TYPE_FLOAT},

The trouble is that the Particle class has a constructor. This makes it a none POD type and the offsetof macro really shouldn't be used.

Have other people come across this? I am wondering if it is one of those cases where the C++ spec is needlessly pedantic and the compiler handles it without a hiccup, or if it is a bit of code that's causing an obscure bug that is really hard to pin down.

I could see the result of the macro returns an offset that is incorrect, it depends on where the compiler puts the address of the constructor. If it puts it at the start of the memory block, then the offset may be pointing at the jump table instead of member variables, I guess this isn't happening as I would expect it to crash.

Or it could be allowing for the jump table, and giving correct values. Or the jump table could be after the member variables.

Maybe I'm paranoid and I should just ignore it, but just because I'm paranoid doesn't mean no one's out to get me

[TheNut]

Well, according to the spec non-POD types have undefined behaviour when using offsetof. As you say, it depends on the compiler and whether or not you're feeling adventurous. This hackish macro IMO should never have existed and you have every right to be paranoid when code you're porting is using such dangerous behaviour. If this is the source of your problems, I would invest the time to write a proper deserializer. If it happens to work fine on your builds, then since you're just porting code I would document the nature of that code for future reference, log debug info so you can validate the data and memory, and move on.

[}:+()___ (Smile)]

Existence of constructor doesn't necessary break PODness, only nontrivial copy constructor does so. offsetof never returns incorrect offset (unless library writers break it intentionally). I think you can safely set public members through offsetof of any type of class, even pure virtual.

[Kenneth Gorking]

I use offsetof extensively in my scripting system, and I'm taking addresses from classes with tons of functions and virtual functions. There has never been a problem, neither on msvc nor gcc.
All the macro does, is taking the address of variable in a zero-address struct, ie . (size_t)&reinterpret_cast<const volatile char&>((((Particle*)0)->X))

offsetof will never return the addres of the virtual table (which only exists in classes with virtual functions, btw.), unless it's some crummy homebrew version

[Stainless]

Kenneth, I really wouldn't like using offsetof with virtual functions. Virtual classes implies inheritance and then things get really wierd.

Do you know that if you do

  Subclass* p1 = new Subclass();
  Superclass* p2 = p1;

...then p1 and p2 can have *different values*?

Which means, of course, that doing (Superclass*)(void*)p1 leaves you with a duff pointer and crashes if you call through it.

There is lot's of hackish code in this project that makes my teeth itch. things like ...

void DeleteItem(void * item)
{
   delete [] item;
}

Just horrible.

But it is working on lots of platforms.

Working in some languages is like going shooting on a military range, all the safety systems are there but it's possible to kill yourself if you really want to.

C++ not only hands you the gun, it loads it with armour piercing ammo, chambers a round, and turns the safety off then tosses you the gun from yards away.

[EddyCharly]

Dunno if it helps, but a more elegant way would be to use pointer to data member instead of the offsetof macro.

[.oisyn]

TheNut, on 14 July 2012 - 10:43 AM, said:

Well, according to the spec non-POD types have undefined behaviour when using offsetof. As you say, it depends on the compiler and whether or not you're feeling adventurous. This hackish macro IMO should never have existed and you have every right to be paranoid when code you're porting is using such dangerous behaviour.

I'm sorry but this is just a load of bollocks. Yes, it is undefined behaviour, but you should question *WHY* it is undefined behaviour. I can't think of any reason that would actually be valid in practice. I'm affraid this exactly is one of those cases where the standard is needlessly pedantic.

That said, whether the use of offsetof() in this particular case is the best design decision is altogether another story. But in the interest of working with a system that's already there, you won't run into any problems by simply using your own offsetof() macro that doesn't have the same requirements upheld by the compiler. Also, POD rules have been releaved in C++11, I wonder whether the standard still finds it UB.

[}:+()___ (Smile)]

It's undefined behaviour due to virtual inheritance. But why standard says it's works only for PODs instead of explicit mention of members of abstract base? I cannot comprehend it. (IMO it's virtual inheritance that defective by design, not offsetof.)

[Kenneth Gorking]

Stainless, on 16 July 2012 - 08:26 AM, said:

Kenneth, I really wouldn't like using offsetof with virtual functions. Virtual classes implies inheritance and then things get really wierd.

Do you know that if you do

  Subclass* p1 = new Subclass();
  Superclass* p2 = p1;

...then p1 and p2 can have *different values*?

That only happens with multiple inheritance, and that's what it is supposed to do, to function correctly. Nothing weird about it. I also don't see what this has to do with taking addresses of variables.

Stainless, on 16 July 2012 - 08:26 AM, said:

Which means, of course, that doing (Superclass*)(void*)p1 leaves you with a duff pointer and crashes if you call through it.

Code like that deserves to crash, and any developer that writes code like that, deserves a firm pimp-slapping.

Stainless, on 16 July 2012 - 08:26 AM, said:

Working in some languages is like going shooting on a military range, all the safety systems are there but it's possible to kill yourself if you really want to.
C++ not only hands you the gun, it loads it with armour piercing ammo, chambers a round, and turns the safety off then tosses you the gun from yards away.

C++ is a powerful language, but your analogy is a bit drastic. I wonder what analogy you would use for assembly

[.oisyn]

Kenneth Gorking, on 16 July 2012 - 08:01 PM, said:

That only happens with multiple inheritance, and that's what it is supposed to do, to function correctly. Nothing weird about it. I also don't see what this has to do with taking addresses of variables.

Actually that's not true. It can also happen with single inheritance when the derived class has a vtable but the base class has not

[Stainless]

@Kenneth

not allowed to pimp slap programmers anymore, call it "violence in the work place" and you get arrested. What is the world coming to...

I think the whole problem with the offsetof macro is that it was designed to work with structs, when the whole distinction between structs and classes blurred, they just allowed it to work on classes as well. Nothing wrong with that at all. However once you start adding vtables to a class structure the calculation of a relative address becomes a slightly more complex problem, so they stuck a warning on it just in case a compiler screws up.

I wish I could change the design of the code, but I cannot. So I have gone through all the classes that are used this way and removed the constructor from them. Then in the bit of code that creates the instances of the objects, I initialise the member variables.

It's got rid of the warnings, decreased compilation time, but not sorted out any of the bugs I was looking at.

[.oisyn]

Stainless, on 17 July 2012 - 08:41 AM, said:

when the whole distinction between structs and classes blurred, they just allowed it to work on classes as well. Nothing wrong with that at all.

Well there never has been a real distinction between structs and classes in the standard. The *one and only* difference is default access specification.

Quote

However once you start adding vtables to a class structure the calculation of a relative address becomes a slightly more complex problem.

That's actually only the case when you're using virtual base classes. Otherwise, vtable or not, members of classes/structs are at fixed offsets from the start of the struct/class. And even with virtual base classes it only applies to (direct or inherited) members of that virtual base class.

Quote

I wish I could change the design of the code, but I cannot. So I have gone through all the classes that are used this way and removed the constructor from them. Then in the bit of code that creates the instances of the objects, I initialise the member variables.

I would just copy the implementation of offsetof() to your own. The only reason it complains is because it has semantic knowledge of offsetof(). As soon as you rename that macro, the warnings vanish but the code would work just as well as before

BTW, I just checked, in C++11 the requirements for offsetof() have been changed from POD type to standard layout type. A standard layout type is a class that:
— has no non-static data members of type non-standard-layout class (or array of such types) or reference,
— has no virtual functionsand no virtual base classes,
— has the same access control for all non-static data members,
— has no non-standard-layout base classes,
— either has no non-static data members in the most derived class and at most one base class with non-static data members, or has no base classes with non-static data members, and
— has no base classes of the same type as the first non-static data member.

So your code would be totally fine when compiled with a C++11 compiler.

[Stainless]

Yes, I love the idea that I can just upgrade the compiler and the problems go away. Sadly, upgrading the compiler means I have to kick a guy called Marko who lives in Japan and make him write me a new one.

Since he's just done that and given me a lovely new compiler that can do a debug build in a few minutes instead of the "few hours" the last one did, I'm a little loath to tell him it's all got to be done again.

No, me spending a few hours changing source code is probably better.

It's interesting that when you profile the compiler, one of the single slowest operations is sending text to the VS output window. A single string can take over 100ms ..... that's an age when you compare it to the work the compiler is doing.