[PATCH] Fix vbtable indices if a class shares the vbptr with a non-virtual base

[Reid Kleckner]

================
Comment at: lib/AST/VTableBuilder.cpp:3314-3316
@@ +3313,5 @@
+    const ASTRecordLayout &BaseLayout = Context.getASTRecordLayout(CurBase);
+    if (!BaseLayout.hasVBPtr() ||
+        DerivedVBPtrOffset != BaseOffset + BaseLayout.getVBPtrOffset())
+      break;
+
----------------
Timur Iskhodzhanov wrote:
> Reid Kleckner wrote:
> > I think this can just be:
> > 
> >   if (CurBase->getNumVBases() == 0)
> >     continue;
> > 
> > No need to get the ASTRecordLayout of the nvbases.
> Err, no - it leads to wrong getVBTableIndex() return values which result in later assertion failures in the VBTableBuilder.
> 
> After some thought, I've decided to bring back "continue;" rather than "break;".
> This makes this code work correctly regardless of the order of nvbases returned by bases_begin()/end().
> I don't think this orded is defined in such a way that the derived class is guaranteed to share its vbptr with first nvbase with vbases...
> See the test I've added in the new iteration. It happens to work correctly now, but I'm not sure it's guaranteed to work with "break;" in the future.
> 
> I don't think we really want to optimize this way too much as the results are cached and there shouldn't be many classes with many nvbases.
I think the problem with my suggestion is the case you identified: when the primary base is the second nvbase, we probably override its vbtable instead.

I don't think comparing offsets is a reliable, clean, or understandable way to identify which nvbase holds our vbtable, something which record layout already figured out for us.  I'm not really concerned with performance.

We need something in ASTRecordLayout similar to PrimaryBase but for vbtables.  Probably BaseWithVBTable.


http://llvm-reviews.chandlerc.com/D2089