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

[Timur Iskhodzhanov]

Ah, I see.
This turned out to be easier than I thought, so here we go:
http://llvm-reviews.chandlerc.com/D2120

2013/11/5 Reid Kleckner <rnk at google.com>:
>
>
> ================
> 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