<p dir="ltr"><br>
On Aug 22, 2013 12:45 AM, "Timur Iskhodzhanov" <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br>
><br>
><br>
><br>
><br>
> 2013/8/22 David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>><br>
>> On Wed, Aug 21, 2013 at 10:33 AM, Timur Iskhodzhanov<br>
>> <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br>
>> > Author: timurrrr<br>
>> > Date: Wed Aug 21 12:33:16 2013<br>
>> > New Revision: 188909<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=188909&view=rev">http://llvm.org/viewvc/llvm-project?rev=188909&view=rev</a><br>
>> > Log:<br>
>> > [CGF] Get rid of passing redundant VTable pointer around in CodeGenFunction::InitializeVTablePointer[s]<br>
>><br>
>> Strangely, this appears to have regressed debug info (probably based<br>
>> on a debug info feature I implemented over the weekend).<br>
>><br>
>> <a href="http://lab.llvm.org:8011/builders/clang-x86_64-ubuntu-gdb-75/builds/8031">http://lab.llvm.org:8011/builders/clang-x86_64-ubuntu-gdb-75/builds/8031</a><br>
>><br>
>> I don't really know how your change connects to this, but I'll<br>
>> investigate, etc - no action required on your part, just wanted to<br>
>> note it down here in case anyone was looking at the bots, seeing weird<br>
>> debug info, etc.<br>
><br>
> <br>
> Ouch, check-all doesn't detect that...<br>
> Thanks for taking this!</p>
<p dir="ltr">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.</p>
<p dir="ltr">> <br>
>><br>
>> ><br>
>> > Modified:<br>
>> > cfe/trunk/lib/CodeGen/CGClass.cpp<br>
>> > cfe/trunk/lib/CodeGen/CodeGenFunction.h<br>
>> ><br>
>> > Modified: cfe/trunk/lib/CodeGen/CGClass.cpp<br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=188909&r1=188908&r2=188909&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=188909&r1=188908&r2=188909&view=diff</a><br>
>> > ==============================================================================<br>
>> > --- cfe/trunk/lib/CodeGen/CGClass.cpp (original)<br>
>> > +++ cfe/trunk/lib/CodeGen/CGClass.cpp Wed Aug 21 12:33:16 2013<br>
>> > @@ -1852,7 +1852,6 @@ void<br>
>> > CodeGenFunction::InitializeVTablePointer(BaseSubobject Base,<br>
>> > const CXXRecordDecl *NearestVBase,<br>
>> > CharUnits OffsetFromNearestVBase,<br>
>> > - llvm::Constant *VTable,<br>
>> > const CXXRecordDecl *VTableClass) {<br>
>> > const CXXRecordDecl *RD = Base.getBase();<br>
>> ><br>
>> > @@ -1875,6 +1874,7 @@ CodeGenFunction::InitializeVTablePointer<br>
>> > // And load the address point from the VTT.<br>
>> > VTableAddressPoint = Builder.CreateLoad(VTT);<br>
>> > } else {<br>
>> > + llvm::Constant *VTable = CGM.getVTables().GetAddrOfVTable(VTableClass);<br>
>> > uint64_t AddressPoint =<br>
>> > CGM.getVTableContext().getVTableLayout(VTableClass).getAddressPoint(Base);<br>
>> > VTableAddressPoint =<br>
>> > @@ -1919,7 +1919,6 @@ CodeGenFunction::InitializeVTablePointer<br>
>> > const CXXRecordDecl *NearestVBase,<br>
>> > CharUnits OffsetFromNearestVBase,<br>
>> > bool BaseIsNonVirtualPrimaryBase,<br>
>> > - llvm::Constant *VTable,<br>
>> > const CXXRecordDecl *VTableClass,<br>
>> > VisitedVirtualBasesSetTy& VBases) {<br>
>> > // If this base is a non-virtual primary base the address point has already<br>
>> > @@ -1927,7 +1926,7 @@ CodeGenFunction::InitializeVTablePointer<br>
>> > if (!BaseIsNonVirtualPrimaryBase) {<br>
>> > // Initialize the vtable pointer for this base.<br>
>> > InitializeVTablePointer(Base, NearestVBase, OffsetFromNearestVBase,<br>
>> > - VTable, VTableClass);<br>
>> > + VTableClass);<br>
>> > }<br>
>> ><br>
>> > const CXXRecordDecl *RD = Base.getBase();<br>
>> > @@ -1970,7 +1969,7 @@ CodeGenFunction::InitializeVTablePointer<br>
>> > I->isVirtual() ? BaseDecl : NearestVBase,<br>
>> > BaseOffsetFromNearestVBase,<br>
>> > BaseDeclIsNonVirtualPrimaryBase,<br>
>> > - VTable, VTableClass, VBases);<br>
>> > + VTableClass, VBases);<br>
>> > }<br>
>> > }<br>
>> ><br>
>> > @@ -1979,16 +1978,12 @@ void CodeGenFunction::InitializeVTablePo<br>
>> > if (!RD->isDynamicClass())<br>
>> > return;<br>
>> ><br>
>> > - // Get the VTable.<br>
>> > - llvm::Constant *VTable = CGM.getVTables().GetAddrOfVTable(RD);<br>
>> > -<br>
><br>
><br>
> You might consider adding<br>
><br>
> CGM.getVTables().GenerateClassData(RD);</p>
<p dir="ltr">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.</p>
<p dir="ltr">Thanks for the suggestion/pointer</p>
<p dir="ltr">> here with a comment why it's needed.<br>
> Writing a "check-clang" test is also encouraged :)</p>
<p dir="ltr">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.</p>
<p dir="ltr">> <br>
>><br>
>> > // Initialize the vtable pointers for this class and all of its bases.<br>
>> > VisitedVirtualBasesSetTy VBases;<br>
>> > InitializeVTablePointers(BaseSubobject(RD, CharUnits::Zero()),<br>
>> > /*NearestVBase=*/0,<br>
>> > /*OffsetFromNearestVBase=*/CharUnits::Zero(),<br>
>> > - /*BaseIsNonVirtualPrimaryBase=*/false,<br>
>> > - VTable, RD, VBases);<br>
>> > + /*BaseIsNonVirtualPrimaryBase=*/false, RD, VBases);<br>
>> > }<br>
>> ><br>
>> > llvm::Value *CodeGenFunction::GetVTablePtr(llvm::Value *This,<br>
>> ><br>
>> > Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h<br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=188909&r1=188908&r2=188909&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=188909&r1=188908&r2=188909&view=diff</a><br>
>> > ==============================================================================<br>
>> > --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original)<br>
>> > +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Wed Aug 21 12:33:16 2013<br>
>> > @@ -1180,7 +1180,6 @@ public:<br>
>> > void InitializeVTablePointer(BaseSubobject Base,<br>
>> > const CXXRecordDecl *NearestVBase,<br>
>> > CharUnits OffsetFromNearestVBase,<br>
>> > - llvm::Constant *VTable,<br>
>> > const CXXRecordDecl *VTableClass);<br>
>> ><br>
>> > typedef llvm::SmallPtrSet<const CXXRecordDecl *, 4> VisitedVirtualBasesSetTy;<br>
>> > @@ -1188,7 +1187,6 @@ public:<br>
>> > const CXXRecordDecl *NearestVBase,<br>
>> > CharUnits OffsetFromNearestVBase,<br>
>> > bool BaseIsNonVirtualPrimaryBase,<br>
>> > - llvm::Constant *VTable,<br>
>> > const CXXRecordDecl *VTableClass,<br>
>> > VisitedVirtualBasesSetTy& VBases);<br>
>> ><br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > cfe-commits mailing list<br>
>> > <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
</p>