Re: [Boost-bugs] [Boost C++ Libraries] #3604: Access violation on diamond inheritance

Subject: Re: [Boost-bugs] [Boost C++ Libraries] #3604: Access violation on diamond inheritance
From: Boost C++ Libraries (noreply_at_[hidden])
Date: 2009-11-30 18:22:24

#3604: Access violation on diamond inheritance
----------------------------------+-----------------------------------------
  Reporter:  kondo@…              |       Owner:  ramey         
      Type:  Support Requests     |      Status:  closed        
 Milestone:  Boost 1.41.0         |   Component:  serialization 
   Version:  Boost 1.40.0         |    Severity:  Not Applicable
Resolution:  invalid              |    Keywords:                
----------------------------------+-----------------------------------------
Changes (by ramey):

  * type:  Bugs => Support Requests
  * severity:  Problem => Not Applicable

Comment:

 Q1: Now I'm wondering about this test.  It currently passes on all
 platforms but it has failed for unexplained reasons in the past.  I'm not
 really sure about this.  Your test case is more ambiguous in that there
 are several "bottom" objects besides "Target". Perhaps this is what makes
 the difference.  Also, "Target" derives from another "bottom" class.
 Perhaps this can be made to work by avoiding this situation.

 Q2: When I run this with the MSVC debugger an examine pVBASE .  It shows
 garbage in the derived class.  This is not what I expect to see.  When you
 export in the other sequence, it shows OK.  This is what led me to try the
 static downcast (which is similar to what the serialization library does
 to make this work).  The static downcast shows a compile time error which
 leads me to believe that we're depending on undefined behavior.  What I
 believe is happening is that the data looks like

 when we

 EXPORT(Target)
 EXPORT(sub1)

 Target object:
     VBase data
     Mid2 data
     Sub2 data
     target data

 Sub1 object
     pointer to VBase data
     Mid1 data
     Sub1 data

 So we can downcast from pVbase to pTarget because downcast just adjusts
 the pointer according to the size of the embedded data structures.

 But when we do

 EXPORT(sub1)
 EXPORT(Target)

 Sub1 object
     VBase data
     Mid1 data
     Sub1 data

 Target object:
     pointer VBase data
     Mid2 data
     Sub2 data
     target data

 and we try to downcast from pVBase to pTarget it doesnt work because the
 address cannot be properly adjusted.  The difference is that the single
 instance of Vbase data is included only in the first instance created.

 So that's my guess.  That is I believe that my test and your example work
 depend upon C++ undefined behavior.  I think that knowing this will permit
 you to tweak your structure so that it does what you want on all
 compilers.  But you'll really be going beyond the guarentees of the C++
 language.

 BTW:C++ is now so complex in it's details that I don't have 100%
 confidence when I'm dealing with these kinds of details.  So you're free
 be skeptical of my assessment.

 Q3: Assuming I'm correct in my answer to Q2, there's two approaches:

 a) Code strictly according to standard C++ and don't depend on any
 undefined behavior.  That makes your program "provably correct"
 b) Make it work and move on.

 Generally I prefer a).  Unfortunately, sometimes I have to do b).  One
 such case is the EXPORT functionality which is very convenient (perhaps
 necessary) but relies on undefined C++ behavior from all compilers.  If
 you need export you're already in b) territory to some extent. A couple of
 options.

 a) craft your application in accordance with the hypothesis in my answer
 to Q2 which would mean : export only one bottom class.  This probably
 would be OK.

 b) avoid virtual base class by including a static member in the Base
 class.  Extra work and not exactly equivalent but it might be a better
 choice.  This would not depend on any undefined C++ behavior.

 c) note that this whole issue comes from the usage of the EXPORT.  If you
 were to avoid using this, there would no problem.  This does however
 required "pre-registration" using Archive::register.  This is not as
 convenient as EXPORT, but it has the advantage that it doesn't depended on
 undefined C++ behavior such as EXPORT does so it is not as "fragile".

 I think any the above would likely work well.

 c) restructure the class hierarchy so that there is one (non-virtual) base
 class which is used as the "universal pointer".  Lower classes would use
 multiple in heritance to "add-in" the virtual base class functionality.

 d) note that I addressed some similar issues with
 boost::serialization::singleton<T> template.

 These later approaches would be more experimental.

 Good luck.

 Robert Ramey

-- 
Ticket URL: <https://svn.boost.org/trac/boost/ticket/3604#comment:6>
Boost C++ Libraries <http://www.boost.org/>
Boost provides free peer-reviewed portable C++ source libraries.