A serious VC++ Compiler Bug

Neville Franks

Hi everyone, This reminds me of the bad old days where compiler bugs were the norm. I've actually been impressed with how robust the VC compiler is, until today. I waisted about 4 hours trying to work out why some code was crashing and burning big time. The following code snippet clearly and simply demonstrates the problem. Changing one line of code (keyword) masks the problem, and changing another gets the compiler to tell you that you this won't work. A valuable lesson learnt, if nothing else. Anyone know how best to report this to MS. I'll share my answers once some of you have responded (excluding my friend, guru Russell :rolleyes: ). ----------------- // What does ShowVar1() print? #include class A { public: A() { m_iVar1 = 69; } ~A() {} int m_iVar1; }; class B; class C { public: A *pA; B* GetB() { return (B*)pA; } }; class B : public A { public: B() { m_iVar2 = 36; } virtual ~B() {} void ShowVar1(); int m_iVar2; }; void B::ShowVar1() { printf( "m_iVar1: %d\n", m_iVar1 ); } main() { B b; C c; c.pA = &b; c.GetB()->ShowVar1(); return 0; } ---------------- Neville Franks, Author of ED for Windows www.getsoft.com

Erik Funkenbusch

Actually, it's not really a bug, but rather a side-effect. The problem is that you are casting your A* to a B* at a time when the class B has not been fully defined (there is only the forward declaration), so the compiler doesn't know at that point that B is actually derived from A, and that it has to reposition the pointer to the A* base class. You are essentially forcing the compiler to cast to something that it doesn't know about, and like a dutiful citizen, it does what you tell it to do (This is why casts are nasty and to be avoided at all costs). If you move the declaration of C to below the declaration of B, then you get the correct result. One could argue that the compiler should know better, but this is solidly in the realm of "undefined behavior" and as such, the compiler is allowed to do just about anything, including giving a wrong result.

Lost User

This must be one of the simplest examples why you should avoid old style casts. If you used static_cast instead this code wouldn't even compile.

Neville Franks

Hi Eric, You're solution is indeed one fix for this. Casting pointers doesn't change the pointers value, it only keeps the compiler happy, so casting to B when B isn't defined shouldn't make any difference to the compiled code. There is a more subtle problem with this code, where you can get the correct result without defining B before C. Other compilers do produce the correct result. Neville Franks, Author of ED for Windows www.getsoft.com

Neville Franks

Nenda, You're absolutely right that using a static cast would have forced class B to be defined before class C which resolves the issue, however this does not take away from the fact that as the code stands it doesn't work, and as far as I'm concerned it should. GetB() does not access any members of class B or class A it simply returns a correctly casted pointer. The other sting in the tail is that if you either add a virtual function to class A or remove the remove the virtual function from class B the code works correctly. I'm afraid I can't agree that this isn't a bug. I certainly can't find any discussion in any of my C++ books that indicates this code is not valid. Further the compiler compiles it without a error, yet it doesn't produce the expects results. The lessons to be learnt are a) always use the new style casts, b) either always define classes before they are used (which the new casts will force you to do) or if you can't easily do this then move the function definition out of the class so that it appears in a source file after the class is defined. Neville Franks, Author of ED for Windows www.getsoft.com

Erik Funkenbusch

Casting pointers *DOES* change it's value in many circumstances. When you cast a pointer to a base class, you return a pointer to it's base class, which is at a different address than your derived class. Don't believe me? Look in the debugger. Move your declarion of C to after your declaration of B, then put a breakpoint on your assignment of b to c.pA. Look at the address of b, and look at the address contained in c.pA after the assignment. You'll see that they're different. Now, let's get back to your original example. When accessing an m_iVar1 from a pointer to an A object, let's say the data is at offset A pointer + 4. Now, when accessing it from a B object, the compiler has to change that offset to be B pointer + 4 (the offset to A) + 4. Now, looking at the cast, the compiler does not yet know that B is derived from A, therefore it assumes you want to reinterpret_cast an A* to a B*, which doesn't adjust the pointer backwards to account for A being a subclass of B because the compiler does not yet know it. It treats the A* as a B*, thus when you try to access m_iVar1, it generates an address read of B pointer + 4 + 4 *FROM THE ACUTAL POINTER TO A, rather than the pointer to B which is 4 bytes before it memory*, so the compiler actually access B's m_iVar2, since this happens to be at the location that is A + 4 + 4. This is all illustrated in this layout in psuedo-C++. class B { b's vtable class A { a's vtable int m_iVar1; } int m_iVar2; } As you can see, a pointer to A is at a different memory address, and when you tell the compiler (via the C style cast) that you want to treat an A* as a B* when it doesn't yet know that B is derived from A, it can only generate code based on A. This is a result of the fact that C-style casts do different things in different situations. In this situation, when B is not yet defined, it acts as a reinterpret cast rather than a static_cast, and doesn't do any adjusting of pointers to account for it. Now, some compilers might actually be able to account for this situation, but it's all basically implemenation dependant and undefined by the C++ standard, so it's legal for the compiler to do just about anything, including correctly figure it out, or incorrectly work with the information it has. So, this was a round about way to say, it's not a bug. It's undefined behavior and you can't depend on it working any given way at all, ever.

Lost User

Sorry, this isn't a bug in the compiler, it is a bug in your source. Just because it works on other platforms just means that due to the way the implemented their v-tables, you got lucky. You can't blindly cast from one class type to another and expect it to work when the compiler has no clue about a class's definition.

Philippe Mori

I think that the compiler should give a warning at level 4 (I haven't verify if it does). I know that delete may not call the destructor on forward declared classes (at least with some compilers) and VC will give a warning (at least at level 4) in that situation. I really think that tou should made an habit to uses new-style cast as often as possible. For ex. in a class similar to auto_ptr that we uses at work, I have put a static_cast in the destructor to ensure that the code won't compile for forward declared classes not yet defined. It is not a bug in the compiler according to the standard specification but I think that it the compiler should report an warning even though it may not be required by the standard (and it should also do at level 3 at least when the result is not the expected one). Philippe

Russell Robinson

Is this a bug or isn't it? Well let's have a look at some information from Stroustrup & Ellis's "The Annotated C++ Reference Manual". Page 68: "... It is guaranteed that a pointer to an object of a given size may be converted to a pointer to an object of the same or smaller size and back again without change." Now, of course, because the derived class isn't defined, the compiler doesn't yet know the size of it. But that's the compiler's problem. The guarantee is that this should work; it's up to the compiler to either (a) honour that guarantee or (b) report that it cannot and fail compilation. Also Page 68: "A pointer to a class B may be explicitly converted to a pointer to a class D that has B as a direct or indirect base class if an unambiguous conversion from D to B exists and if B is not a virtual base class." This page mentions difficulties when the derived class is not yet defined, but only in the case of multiple inheritence. Nev's example is single inheritence so the warning doesn't apply. As for this idea that some people have mentioned about addresses changing between base and derived class, what a load of rubbish! :( Have a look at Pages 218 to 219 of the same book...."Objects of derived classes are composed by concatenating the members of the base classes and of the derived class itself." If you're seeing the addresses change, you're seeing the compiler's bug in action. Not the way it *should* work. It's fundamental to the design of C++ that objects of a derived class encapsulate the objects of its base classes. Order of the objects, and the exact implementation is left to the compiler, but the concatenation isn't! There's only one conclusion possible given the above information...this is a compiler bug! Would anyone care to quote their "standard specifications" to prove otherwise? I absolutely agree that the new C++-style casting operators are the best way to avoid this problem, but that doesn't mean the problem should be there in the first place. Russell Robinson (russellr@rootsoftware.com) Author of TTMaker (Advanced Timetabling Software) http://www.rootsoftware.com

Neville Franks

My understanding is that a Derived class "IsA" Base class so I have some (great) difficulty with your suggestion that a pointer to class B is at a different address to a pointer to its base class, A. IMHO what you're seeing is a manifestation of the problem, in that the debugger is lying to you. You may also like to read the post from Russell Roninson below. Neville Franks, Author of ED for Windows www.getsoft.com

Erik Funkenbusch

A derived class is indeed also a base class. But you are confusing the high level concept of "is a" with the low level concept that the compiler must make the interfaces for the base class compatible with pointers to it. I'll answer Russell as well, but let's just say that this is undefined behavior. The C++ standard, nor the ARM specify how C++ must lay out it's objects. Read my reply to Russell for more information.

Erik Funkenbusch

First, the ARM is almost 10 years old and certainly doesn't dictate how modern C++ behaves, but still, in many ways it's still accurate. You can't use it as a definitive answer to whether something is legal anymore though. Now, to address your points Your reference from page 68 about a pointer being guaranteed to be convertable is fine. The pointers *ARE* convertable, the problem here is that the pointer is being cast at a time where the class is undefined. As such, the compiler doesn't know that A is a smaller size than B, so it cannot perform the guaranteed operation. Also, I think you are confusing what the meaning of "without change" means. It doesn't mean that the pointer won't change, it means that the end result is the same. Your reference to pages 218 and 219 actually support my argument. Indeed, objects of derived classes *ARE* composed by concatenating the members of the base classes and the derived class. That doesn't change the fact that the assembly code generated to dereference a member from a given interface can, and often is different depending on which interface you are using. The thing that throws a monkey wrench into the works is the vtables of the classes. Since the vtables generally appear at the beginning of the objects layout, that means that each base class with a vtable is at a different offset of the previous class in the hiearchy chain. ie.

class C {
vtable
class B {
vtable
class A {
vtable
int a;
}
int b;
}
int c;
}

So literally, A's vtable is 8 bytes into the pointer to C, while B's vtable is 4 bytes into the pointer to C and C's vtable begins at C + 0. Now, when we want to access a from a C pointer, a begins at C + 12. But, if we look at A seperately, a exists as A + 4. Thus, if we're accessing a through an A pointer, the client code expects a to be 4 bytes up in memory from the pointer to A, but this isn't the case when we are using a C object, it's 12 bytes. The compiler must do some magic, which is, when casting C to A, it moves the pointer ahead 8 bytes. You can see this yourself in the debugger by examing a C object and looking at it's physical memory address for each member and the vtables. To further complicate matters, consider a fuction which takes an A*. void f(A* myA); This code must work with *ALL* A*'s whether they are direct instatiations of A, or B's or C's derived from A. The code doesn't change just because we've passed a C or

Lost User

Erik is totally right on this one guys. I would add, but Erik has covered the subject very well.

Russell Robinson

Erik, thanks for your detailed reply. Your argument is strong, and it almost had me convinced..... I submit for your examination the following program:

#include class A
{
public:
int aval;
A()
{
aval = 0x1234;
}
};

class B : public A
{
public:
virtual ~B()
{
}
int bval;
B()
{
bval = 0x4567;
}
};

void
main()
{
B b;
A* pA = &b;
B* pB = &b;

    printf("pA=0x%x\\n",pA);
    printf("pB=0x%x\\n",pB);

}

I now retract my comment that addresses don't change between the base and derived class. This is clearly implementation dependent. :-O What I should have said is that addresses don't *have to* change between the base and derived class, depending on compiler implementation. With the GNU C++ compiler, for instance, this program prints the same address. The base class (A) and the derived class (B) are at the same address. With the MSVC++ compiler, however, the base class and the derived class are at different addresses, and the difference is the vftable pointer for class B. So, the memory map for a B object using the MSVC++ compiler is:

      --------------
     |  vft pointer |
     |--------------|
     |  A.aval      |
     |--------------|
     |  B.bval      |
      --------------

The memory map using the GNU C++ compiler is:

      --------------
     |  A.aval      |
     |--------------|
     |  B.bval      |
      --------------

I've no idea where the GNU C++ compiler puts the vftable for B, but I think it's a global and is not carried around with the class object. Interestingly, if you remove the virtual function from class B, the memory map in MSVC++ becomes identical to the GNU C++ memory map. In other words, as you pointed out, the vftables screw around with the simple concatenation of class derivation in the Microsoft implementation. Your quote from the C++ standard was: An rvalue of type "pointer to cv D" where D is a class type, can be converted to an rvalue of type "pointer to cv B," where B is a base class of of D. [...] The result of the conversion is a pointer to the base class sub-object of the derived class object. Your comment was: "Clearly this is saying that when converting from a derived class to a base class, the result is a different pointer

Lost User

I am afraid that Eric is right, despite the apparent incosistencies in the various quotes from the ARM (and any other sources). It is always a dangerous game to try and 'prove' or 'disprove' various C++ details via quotes from the standard - even those who have been actively involved in the standard often have trouble reconciling apparently conflicting details! However, this point is very clear - the details of how an object is laid out in memory is undefined by the standard, has never been defined, and (most likely) never will be. This MUST lead to the inevitable consequence where old style 'C' casts can create 'legal' C code, but invalid C++ code. Really, the underlying point here is that C++ is a mixed language - some parts are inherited from the 'old word' of C, and much is new - and mixing the two leads to conflicts like this. I have no doubt that this is not a bug - it is a consequence of Microsofts implementation, and the only part of the C++ standard is seems to be conflicting with is clearly a rule that exists purely to support old style C code. Remember, the ARM is essentially a 'guide', rather than a completely compressive set of unambiguous rules. I am sure the way out of this is not to change the compiler to implement the implied hehaviour of the statement "you can convert a pointer to an object of a given size to a pointer to an object of same or smaller size and back again without change", but rather to re-word the ARM refer to more clearly indicate the intent - that this applies in the absence of Vtables. Again, I stress that the ARM is NOT a definitive set of rules that must be followed, and I firmly believe this is one example (there would be many more) where the words, even thought they are 'firm', are simply inaccurate. Bottom line - don't use C style casts on objects. Stroupstrup says don't do it, and that should be good enough for most of us! Perhaps Eric's quote would have been better phrased as : "Clearly this is saying that when converting from a derived class to a base class, the result 'might be' (rather than 'is') a different pointer to a different object."

Erik Funkenbusch

You are correct that the pointer does not *HAVE* to change, and while some of my statements might have been construed to mean that I said that, it wasn't my intent. My intent was to say that the standard specifically allows a converted pointer to point to a different memory address, and that VC is in fact doing this, and it's perfectly legal for it to do so. And again, the real problem here is one of casting a pointer to an incomplete type to another pointer with a C-style cast rather than a static_cast. C-style casts can and do perform the equivelant of a reinterpret cast when the type is incomplete. This too is legal, otherwise you would never be able to cast a void* to anything, since a void* is a pointer to an incomplete type that cannot be completed. There is nothing here which violates the C++ standard. There may be things which do not appear to jive with the ARM, but the ARM is not the definitive word. The ARM talks about things which are implementation defined, the standard specifically doesn't. Yes, the compiler probably should at least issue a warning about this. But then, when using casts, you tell the compiler "I know what i'm doing, so let me do it". The C++ sepcification doesn't say anything about this issue that I can find. It says only that a proper cv qualified type can be converted to its base class. The issue is that an incomplete type (as a forward declaration is) is not a proper cv qualified type. Thus, at the point of the translation unit, only the rules about incomplete types apply regardless of whether when the type is completed it is a proper cv type.

Russell Robinson

Erik and Mr Anonymous are both saying very sensible things. However, I still disagree. Mr A's point about using the standards to prove correctness or otherwise is well taken, but one does have to start somewhere. Yes, the bottom line is don't use C-style casts...I've agreed with that since Nev discovered this problem. However, it still all comes down to this: Does the C++ language explicitly allow you to convert pointers in the following ways?

to a same-size or smaller object pointer and back without change; and
to the base class and back without change

These are high-level language concepts and, I believe, are not labelled as implementation-dependent in any C++ specification. Does the language specify this or not? It certainly used to, does it still? Only the standards can tell us (I'm still waiting for my copy to arrive from ANSI). You can have at least 4 answers to this question:

It used to specify this but doesn't now;
It still specifies this without exception;
It specifies this with certain exceptions;
It's completely implementation dependent.

Erik and Mr A have to first say where they sit on this question. If they answer 1, 3 or 4, then they are being consistent with everything they've argued. But the standard will say whether they are right or wrong. ARM says they're wrong; it indicates no implementation-dependency here. I say 2, in the case Nev has brought to our attention, and probably 3 in the case of multiple inheritence. I say it's a high-level concept that is not implementation-dependent. This is what all this comes down to....is this area specified as implementation-dependent or not. Does the latest standard say "these conversions are implementation-dependent"? Or, does the latest standard explicitly revoke these conversions? If it's not implementation-dependent, then the MSVC++ compiler is inconsistent - it works sometimes and not others. The GNU compiler, on the other hand, works right in all cases. The MS compiler has no bug here if and only if the standard says these conversions are implementation-dependent. Erik and Mr A seem to be arguing that "the MS compiler's inconsistent implementation of these conversions is due to their implementation of class derivation. Therefore, these conversions must be implementation-dependent". In other words: "if Microsoft does it this way, they must be right and that must be what the standards' specifications mean". I don't agree with that reasoning. Final point: Erik

Erik Funkenbusch

Section 5.2.9 clause 8 says: "An rvalue of type "pointer to cv1 B", where B is a class type, can be converted to an rvalue of type "pointer to cv2 D", where D is a class derived from B, if a valid standard conversion from "pointer to D" to "pointer to B" exists, cv2 is the same cv-qualification as, or greater cv-qualification than, cv1, and B is not a virtual base class of D. [...] If the rvalue of type "pointer to cv1 B" points to a B that is actually a sub-object of an object of type D, the resulting pointer points to the enclosing object of type D. Otherwise, the result of the cast is undefined." Now, what this is saying is that you can do this:

class B
{
};

class D : public B
{
}

D d;

B* pb = &d;
D* pd = static_cast<D*>(pb);

It also says that pd should be the same address as &D, but says that pb may be a different address but must return the address of the D object when converted from a B to a D. The key here is the use of the words "cv-qualification". An incomplete type is not the same cv-type as the same type that's been completed. This is as close to what the ARM says as you're gonna get in the standard. Further, section 5.4 defines "Explicit type conversion cast notation" and refers to what we call C-style casts. clause 6 says: "The operand of a cast using the cast notation can be an rvalue of type "pointer to incomplete class type". The destination type of the cast using the cast notation can be "pointer to incomplete class type". In such cases, even if there is an inheritance relationship between the source and the destination classes, whether the static_cast or reinterpret_cast interpretation is used is unspecified". Section 3.9 clause 6 says: "A class that has been declared but not defined [...] is an incompletely-defined object type. Incompletely-defined types and the void types are incomplete types. In other words, it's unspecified and implementation dependant whether the implementation uses static_cast or reinterpret_cast on a forward declared type before it's defined, and as we've already abundantly demostranted, it's completely legal for a compiler to give incorrect results when you reinterpret_cast a derived class pointer to a base class pointer, since reinterpret_cast doesn't adjust the pointer to the base classes subobject.

Lost User

Mr Anonymous here. What more can I say - Erik (sorry about the bad spelling yesterday) says it much clearer that I have attempted to. This seems as clear cut as you can get in these matters. And just to restate my only point - this is really an interaction between 'old' and 'new'. If the old 'C' style cast was not used, this would not have arisen. The new casts were added to enable the user and the compiler to understand each other clearly. The old casts are there to allow ported 'C' code to compile easily. I don't believe the intention of the ARM was ever to try and resolve any and all differences that may come from mixing these systems. To my mind the ARM is not talking directly to this issue - the "you can cast between objects of the same size" quote is a generalised statement, that lacks qualifiers. Probably because the ARM is quite 'old' in terms of the current level of C++ and the standard. And, as Erik demonstrates, the standard does address these details, and seems pretty clear. And to those who say 'the language allows it, so I should be able to use it' - there is very little doubt that for all it's strengths C++ is an inconsistent and confusing language. Stroupstrup himself in "The Design and Evolution of C++" points out numerous areas where he was faced with difficult decisions between theory, implementation and 'C' compatibility. His point, and I agree, is that C++ is a very good answer to a very difficult question ("How to create an OOP language that is also 'C' compatible"). But despite the general success, if you poke into the dark corners of the language you will find problems. In this case, the new style casts are directly supplied to try address these kinds of issues.

Russell Robinson

My ANSI standards have finally arrived! Erik, thanks for quoting the relevent sections. I think we are getting closer to the truth on this matter now. You quoted Section 5.2.9 clause 8, then suggested that cv-qualification is an issue here. Specifically, you said "An incomplete type is not the same cv-type as the same type that's been completed." I don't believe that's true. CV-qualification (whether something is const and/or volatile) is not determined or affected by whether a type is complete or incomplete. Section 3.9.3 clause 1 says "A type mentioned in 3.9.1 and 3.9.2 is a cv-unqualified type" and Section 3.9.2 specifies all pointer to objects, whether they are complete or otherwise. So we have a pointer to a complete object type and a pointer to an incomplete object type - their cv-qualification is identical. Therefore, Section 5.2.9 clause 8 actually supports my case now. However, I think the key to this is Section 5.4, as you quoted it. As you pointed out, the standard says that it's unspecified whether the static_cast or reinterpret_cast is used when a pointer is to an incomplete class. This is precisely the case that Nev had.

B\*  GetB() const { (B\*)pA; }

According to Section 5.4 clause 6, the compiler is free to implement this as:

B\*  GetB() const { return static\_cast<B\*>(pA); }

or

B\*  GetB() const { return reinterpret\_cast<B\*>(pA); }

MSVC choose the reinterpret_cast implementation according to its freedom under this Section 5.4 clause 6. Because the type is incomplete, the compiler can't use a static_cast without generating an error message and terminating the compilation. So, MS chose to use a reinterpret_cast. (I hope we all wish they'd made the opposite choice.) The problem is that with the change in the memory map between classes with virtuals and classes without, the choice to use a reinterpret_cast is a choice fraught with danger. The danger being illustrated by the problem that Nev encountered. So, do I think this is a compiler bug or not? The compiler is clearly doing what it is allowed to do by the latest standard. Like the word "weed", the word "bug" often has a subjective meaning rather than a clear objective meaning. Whether this is a bug or not depends on the intentions of the compiler's implementors. However, if I'd written the compiler, then found this problem, I'd consider:

the danger to be too great
the memory map decision had an unexpected implication