r188909 - [CGF] Get rid of passing redundant VTable pointer around in CodeGenFunction::InitializeVTablePointer[s]
David Blaikie
dblaikie at gmail.com
Thu Aug 22 06:04:19 PDT 2013
On Aug 22, 2013 12:45 AM, "Timur Iskhodzhanov" <timurrrr at google.com> wrote:
>
>
>
>
> 2013/8/22 David Blaikie <dblaikie at gmail.com>
>>
>> On Wed, Aug 21, 2013 at 10:33 AM, Timur Iskhodzhanov
>> <timurrrr at google.com> wrote:
>> > Author: timurrrr
>> > Date: Wed Aug 21 12:33:16 2013
>> > New Revision: 188909
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=188909&view=rev
>> > Log:
>> > [CGF] Get rid of passing redundant VTable pointer around in
CodeGenFunction::InitializeVTablePointer[s]
>>
>> Strangely, this appears to have regressed debug info (probably based
>> on a debug info feature I implemented over the weekend).
>>
>> http://lab.llvm.org:8011/builders/clang-x86_64-ubuntu-gdb-75/builds/8031
>>
>> I don't really know how your change connects to this, but I'll
>> investigate, etc - no action required on your part, just wanted to
>> note it down here in case anyone was looking at the bots, seeing weird
>> debug info, etc.
>
>
> Ouch, check-all doesn't detect that...
> Thanks for taking this!
Yeah, we have the gdb 7.5 test suite running for things we never considered
testing in check-all. Whenever it finds bugs we add check-all test cases to
verify ythe bug fix. You're not expected to run the gdb suite, especially
on this change that looks rather innocuous for debug info.
>
>>
>> >
>> > Modified:
>> > cfe/trunk/lib/CodeGen/CGClass.cpp
>> > cfe/trunk/lib/CodeGen/CodeGenFunction.h
>> >
>> > Modified: cfe/trunk/lib/CodeGen/CGClass.cpp
>> > URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=188909&r1=188908&r2=188909&view=diff
>> >
==============================================================================
>> > --- cfe/trunk/lib/CodeGen/CGClass.cpp (original)
>> > +++ cfe/trunk/lib/CodeGen/CGClass.cpp Wed Aug 21 12:33:16 2013
>> > @@ -1852,7 +1852,6 @@ void
>> > CodeGenFunction::InitializeVTablePointer(BaseSubobject Base,
>> > const CXXRecordDecl
*NearestVBase,
>> > CharUnits
OffsetFromNearestVBase,
>> > - llvm::Constant *VTable,
>> > const CXXRecordDecl
*VTableClass) {
>> > const CXXRecordDecl *RD = Base.getBase();
>> >
>> > @@ -1875,6 +1874,7 @@ CodeGenFunction::InitializeVTablePointer
>> > // And load the address point from the VTT.
>> > VTableAddressPoint = Builder.CreateLoad(VTT);
>> > } else {
>> > + llvm::Constant *VTable =
CGM.getVTables().GetAddrOfVTable(VTableClass);
>> > uint64_t AddressPoint =
>> >
CGM.getVTableContext().getVTableLayout(VTableClass).getAddressPoint(Base);
>> > VTableAddressPoint =
>> > @@ -1919,7 +1919,6 @@ CodeGenFunction::InitializeVTablePointer
>> > const CXXRecordDecl
*NearestVBase,
>> > CharUnits
OffsetFromNearestVBase,
>> > bool
BaseIsNonVirtualPrimaryBase,
>> > - llvm::Constant *VTable,
>> > const CXXRecordDecl
*VTableClass,
>> > VisitedVirtualBasesSetTy&
VBases) {
>> > // If this base is a non-virtual primary base the address point has
already
>> > @@ -1927,7 +1926,7 @@ CodeGenFunction::InitializeVTablePointer
>> > if (!BaseIsNonVirtualPrimaryBase) {
>> > // Initialize the vtable pointer for this base.
>> > InitializeVTablePointer(Base, NearestVBase,
OffsetFromNearestVBase,
>> > - VTable, VTableClass);
>> > + VTableClass);
>> > }
>> >
>> > const CXXRecordDecl *RD = Base.getBase();
>> > @@ -1970,7 +1969,7 @@ CodeGenFunction::InitializeVTablePointer
>> > I->isVirtual() ? BaseDecl : NearestVBase,
>> > BaseOffsetFromNearestVBase,
>> > BaseDeclIsNonVirtualPrimaryBase,
>> > - VTable, VTableClass, VBases);
>> > + VTableClass, VBases);
>> > }
>> > }
>> >
>> > @@ -1979,16 +1978,12 @@ void CodeGenFunction::InitializeVTablePo
>> > if (!RD->isDynamicClass())
>> > return;
>> >
>> > - // Get the VTable.
>> > - llvm::Constant *VTable = CGM.getVTables().GetAddrOfVTable(RD);
>> > -
>
>
> You might consider adding
>
> CGM.getVTables().GenerateClassData(RD);
I expect this would probably make the tests pass, but I admit I would be a
little confused as to why it's needed/will nerd to think about it further.
If there are cases where we will never emit the vtable for a dynamic class,
even though that class is used, I want to understand how we get away with
that without breaking the program, etc.
Thanks for the suggestion/pointer
> here with a comment why it's needed.
> Writing a "check-clang" test is also encouraged :)
Yep, as mentioned, we add them when we realize there's something to test,
it's just impractical to do so preemptively for the whole gdb test suite.
>
>>
>> > // Initialize the vtable pointers for this class and all of its
bases.
>> > VisitedVirtualBasesSetTy VBases;
>> > InitializeVTablePointers(BaseSubobject(RD, CharUnits::Zero()),
>> > /*NearestVBase=*/0,
>> >
/*OffsetFromNearestVBase=*/CharUnits::Zero(),
>> > - /*BaseIsNonVirtualPrimaryBase=*/false,
>> > - VTable, RD, VBases);
>> > + /*BaseIsNonVirtualPrimaryBase=*/false, RD,
VBases);
>> > }
>> >
>> > llvm::Value *CodeGenFunction::GetVTablePtr(llvm::Value *This,
>> >
>> > Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h
>> > URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=188909&r1=188908&r2=188909&view=diff
>> >
==============================================================================
>> > --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original)
>> > +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Wed Aug 21 12:33:16 2013
>> > @@ -1180,7 +1180,6 @@ public:
>> > void InitializeVTablePointer(BaseSubobject Base,
>> > const CXXRecordDecl *NearestVBase,
>> > CharUnits OffsetFromNearestVBase,
>> > - llvm::Constant *VTable,
>> > const CXXRecordDecl *VTableClass);
>> >
>> > typedef llvm::SmallPtrSet<const CXXRecordDecl *, 4>
VisitedVirtualBasesSetTy;
>> > @@ -1188,7 +1187,6 @@ public:
>> > const CXXRecordDecl *NearestVBase,
>> > CharUnits OffsetFromNearestVBase,
>> > bool BaseIsNonVirtualPrimaryBase,
>> > - llvm::Constant *VTable,
>> > const CXXRecordDecl *VTableClass,
>> > VisitedVirtualBasesSetTy& VBases);
>> >
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130822/04c3449c/attachment.html>
More information about the cfe-commits
mailing list