<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>