[llvm] r204162 - Debug info: Remove OdrMemberMap from DwarfDebug, it's not necessary.

Eric Christopher echristo at gmail.com
Fri Mar 21 11:06:29 PDT 2014


So where are we with this patch series then, it seems like nearly all
of it should have been reverted at this point except for:

a) Converting things to use references where applicable,
b) Handling the retained type map - Dave's email in this thread seems
to be the right path, and
c) We should keep the testcases :)

So, what's up and where are we?

-eric

On Fri, Mar 21, 2014 at 10:59 AM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
> On Fri, Mar 21, 2014 at 10:44 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Fri, Mar 21, 2014 at 10:34 AM, Manman Ren <manman.ren at gmail.com> wrote:
>> >
>> >
>> >
>> > On Tue, Mar 18, 2014 at 10:41 AM, Adrian Prantl <aprantl at apple.com>
>> > wrote:
>> >>
>> >> Author: adrian
>> >> Date: Tue Mar 18 12:41:15 2014
>> >> New Revision: 204162
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=204162&view=rev
>> >> Log:
>> >> Debug info: Remove OdrMemberMap from DwarfDebug, it's not necessary.
>> >> Follow-up to r203982.
>> >>
>> >> Modified:
>> >>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> >>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>> >>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>> >>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h
>> >>
>> >> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=204162&r1=204161&r2=204162&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>> >> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Tue Mar 18
>> >> 12:41:15
>> >> 2014
>> >> @@ -369,7 +369,6 @@ bool DwarfDebug::isSubprogramContext(con
>> >>  // scope then create and insert DIEs for these variables.
>> >>  DIE *DwarfDebug::updateSubprogramScopeDIE(DwarfCompileUnit *SPCU,
>> >>                                            DISubprogram SP) {
>> >> -  SP = SPCU->getOdrUniqueSubprogram(resolve(SP.getContext()), SP);
>> >>    DIE *SPDie = SPCU->getDIE(SP);
>> >>
>> >>    assert(SPDie && "Unable to find subprogram DIE!");
>> >> @@ -604,7 +603,8 @@ DIE *DwarfDebug::constructScopeDIE(Dwarf
>> >>    if (!Scope || !Scope->getScopeNode())
>> >>      return NULL;
>> >>
>> >> -  DIScope DS(Scope->getScopeNode());
>> >> +  // Unique scope where applicable.
>> >> +  DIScope DS(resolve(DIScope(Scope->getScopeNode()).getRef()));
>> >
>> >
>> > Just to recapture what Adrian and I discussed earlier. We try not to
>> > unique
>> > types in the backend. All references to
>> > DITypes with an identifier should be converted to DIRef if possible
>> > (with
>> > the exception of the retained type list).
>>
>> OK - so I'm a bit confused by this code, then. Are you suggesting that
>> "we try not to unique types in the backend, but this case above is an
>> exception that we need to do in the backend for <reason>" or are you
>> saying that this code isn't an instance of what you would call
>> "uniquing types in the backend".
>
>
> Hi David,
>
> I think the answer is the paragraph right below :]
>  > Adrian tried reverting r203982, the testing case for r203982 will still
>> work, it is working because of his earlier commit on converting
>> DIVariable's
>> type to use DIRef.
>
> To clarify, I meant reverting both r203982 and this commit (this commit is a
> revert of r203982 + changes), but keeping the testing cases for r203982.
> Basically with Adrian's work on converting DIVariable's type to use DIRef,
> testing case for r203982 will just work!
>
> Thanks,
> Manman
>
>
>>
>> It looks as if we still want to change this code so it doesn't loop
>> back through "uniquing" by getting a ref to the current entity and
>> "uniquing" it. Instead we should change getScopeNode to return an
>> MDNode or MDString as other "refs" do - and resolve it directly
>> (rather than having it reference the MDNode directly, querying that
>> for the name, then looking the name up to find (possibly the same)
>> MDNode)?
>>
>> >
>> >
>> > Adrian tried reverting r203982, the testing case for r203982 will still
>> > work, it is working because of his earlier commit on converting
>> > DIVariable's
>> > type to use DIRef.
>> >
>> >>
>> >>
>> >>    SmallVector<DIE *, 8> Children;
>> >>    DIE *ObjectPointer = NULL;
>> >>
>> >> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h?rev=204162&r1=204161&r2=204162&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h (original)
>> >> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h Tue Mar 18 12:41:15
>> >> 2014
>> >> @@ -346,9 +346,6 @@ class DwarfDebug : public AsmPrinterHand
>> >>    /// of in DwarfCompileUnit.
>> >>    DenseMap<const MDNode *, DIE *> MDTypeNodeToDieMap;
>> >>
>> >> -  // Used to unique C++ member function declarations.
>> >> -  StringMap<const MDNode *> OdrMemberMap;
>> >> -
>> >>    // List of all labels used in aranges generation.
>> >>    std::vector<SymbolCU> ArangeLabels;
>> >>
>> >> @@ -700,11 +697,6 @@ public:
>> >>      return MDTypeNodeToDieMap.lookup(TypeMD);
>> >>    }
>> >>
>> >> -  /// \brief Look up or create an entry in the OdrMemberMap.
>> >> -  const MDNode *&getOrCreateOdrMember(StringRef Key) {
>> >> -    return OdrMemberMap.GetOrCreateValue(Key).getValue();
>> >> -  }
>> >> -
>> >>    /// \brief Emit all Dwarf sections that should come prior to the
>> >>    /// content.
>> >>    void beginModule();
>> >>
>> >> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=204162&r1=204161&r2=204162&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)
>> >> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Tue Mar 18 12:41:15
>> >> 2014
>> >> @@ -1427,24 +1427,6 @@ DIE *DwarfUnit::getOrCreateNameSpace(DIN
>> >>    return NDie;
>> >>  }
>> >>
>> >> -/// Unique C++ member function declarations based on their
>> >> -/// context and mangled name.
>> >> -DISubprogram
>> >> -DwarfUnit::getOdrUniqueSubprogram(DIScope Context, DISubprogram SP)
>> >> const
>> >> {
>> >> -  if (!hasODR() ||
>> >> -      !Context.isCompositeType() ||
>> >> -      SP.getLinkageName().empty() ||
>> >> -      SP.isDefinition())
>> >> -    return SP;
>> >> -  // Create a key with the UID of the parent class and this SP's name.
>> >> -  Twine Key = SP.getContext().getName() + SP.getLinkageName();
>> >> -  const MDNode *&Entry = DD->getOrCreateOdrMember(Key.str());
>> >> -  if (!Entry)
>> >> -    Entry = &*SP;
>> >> -
>> >> -  return DISubprogram(Entry);
>> >> -}
>> >> -
>> >>  /// getOrCreateSubprogramDIE - Create new DIE using SP.
>> >>  DIE *DwarfUnit::getOrCreateSubprogramDIE(DISubprogram SP) {
>> >>    // Construct the context before querying for the existence of the
>> >> DIE
>> >> in case
>> >> @@ -1452,8 +1434,10 @@ DIE *DwarfUnit::getOrCreateSubprogramDIE
>> >>    // declarations).
>> >>    DIScope Context = resolve(SP.getContext());
>> >>    DIE *ContextDIE = getOrCreateContextDIE(Context);
>> >> +
>> >>    // Unique declarations based on the ODR, where applicable.
>> >> -  SP = getOdrUniqueSubprogram(Context, SP);
>> >> +  SP = DISubprogram(DD->resolve(SP.getRef()));
>> >
>> >
>> > This line of code does nothing, since we don't have identifiers for
>> > DISubprogram.
>>
>> Nice catch - yeah... this is more of that loop-back "uniquing" anyway
>> - it'd be nice to get rid of any code like this in favor of fixing the
>> ways objects are referenced. (I understand there's a reasonable
>> exception when visiting the retained types list, potentially - though
>> we could think about that a bit further too)
>>
>> So we should remove this line of code, right?
>>
>> - David
>>
>> >
>> > Thanks,
>> > Manman
>> >
>> >>
>> >> +  assert(SP.Verify());
>> >>
>> >>    DIE *SPDie = getDIE(SP);
>> >>    if (SPDie)
>> >>
>> >> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h?rev=204162&r1=204161&r2=204162&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h (original)
>> >> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h Tue Mar 18 12:41:15
>> >> 2014
>> >> @@ -487,28 +487,6 @@ public:
>> >>
>> >>    virtual DwarfCompileUnit &getCU() = 0;
>> >>
>> >> -  /// \brief Return whether this compilation unit has the
>> >> -  /// one-definition-rule (ODR).  In C++ this allows the compiler to
>> >> -  /// perform type unique during LTO.
>> >> -  bool hasODR() const {
>> >> -    switch (getLanguage()) {
>> >> -    case dwarf::DW_LANG_C_plus_plus:
>> >> -    case dwarf::DW_LANG_C_plus_plus_03:
>> >> -    case dwarf::DW_LANG_C_plus_plus_11:
>> >> -      // For all we care, the C++ part of the language has the ODR and
>> >> -      // ObjC methods are not represented in a way that they could be
>> >> -      // confused with C++ member functions.
>> >> -    case dwarf::DW_LANG_ObjC_plus_plus:
>> >> -      return true;
>> >> -    default:
>> >> -      return false;
>> >> -    }
>> >> -  }
>> >> -
>> >> -  /// \brief Unique C++ member function declarations based on their
>> >> -  /// context+mangled name.
>> >> -  DISubprogram getOdrUniqueSubprogram(DIScope Context, DISubprogram
>> >> SP)
>> >> const;
>> >> -
>> >>  protected:
>> >>    /// getOrCreateStaticMemberDIE - Create new static data member DIE.
>> >>    DIE *getOrCreateStaticMemberDIE(DIDerivedType DT);
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>



More information about the llvm-commits mailing list