[PATCH] Fix vbtable indices if a class shares the vbptr with a non-virtual base
timurrrr at google.com
Thu Nov 7 06:44:02 PST 2013
Ah, I see.
This turned out to be easier than I thought, so here we go:
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.
More information about the cfe-commits