[llvm] r190418 - Debug Info: define a DIRef template.
David Blaikie
dblaikie at gmail.com
Tue Sep 10 17:38:46 PDT 2013
On Tue, Sep 10, 2013 at 5:24 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
> On Tue, Sep 10, 2013 at 4:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>>
>> On Tue, Sep 10, 2013 at 3:31 PM, Manman Ren <manman.ren at gmail.com> wrote:
>>
>>>
>>>
>>>
>>> On Tue, Sep 10, 2013 at 12:54 PM, David Blaikie <dblaikie at gmail.com>wrote:
>>>
>>>> I assume the specializations I've asked about are there because you
>>>> didn't have a good way to generalize over scopes and types. What sort of
>>>> solutions did you consider to avoid this duplication?
>>>>
>>> By duplication, do you mean the template?
>>>
>>
>> The explicit specializations of the
>>
>>
>>>
>>>> On Sep 10, 2013 11:35 AM, "Manman Ren" <manman.ren at gmail.com> wrote:
>>>> >
>>>> > Author: mren
>>>> > Date: Tue Sep 10 13:30:07 2013
>>>> > New Revision: 190418
>>>> >
>>>> > URL: http://llvm.org/viewvc/llvm-project?rev=190418&view=rev
>>>> > Log:
>>>> > Debug Info: define a DIRef template.
>>>> >
>>>> > Specialize the constructors for DIRef<DIScope> and DIRef<DIType> to
>>>> make sure
>>>> > the Value is indeed a scope ref and a type ref.
>>>> >
>>>> > Use DIScopeRef for DIScope::getContext and DIType::getContext and use
>>>> DITypeRef
>>>> > for getContainingType and getClassType.
>>>> >
>>>> > DIScope::generateRef now returns a DIScopeRef instead of a "Value *"
>>>> for
>>>> > readability and type safety.
>>>> >
>>>> > Modified:
>>>> > llvm/trunk/include/llvm/DebugInfo.h
>>>> > llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>> > llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>>> > llvm/trunk/lib/IR/DebugInfo.cpp
>>>> >
>>>> > Modified: llvm/trunk/include/llvm/DebugInfo.h
>>>> > URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo.h?rev=190418&r1=190417&r2=190418&view=diff
>>>> >
>>>> ==============================================================================
>>>> > --- llvm/trunk/include/llvm/DebugInfo.h (original)
>>>> > +++ llvm/trunk/include/llvm/DebugInfo.h Tue Sep 10 13:30:07 2013
>>>> > @@ -17,6 +17,7 @@
>>>> > #ifndef LLVM_DEBUGINFO_H
>>>> > #define LLVM_DEBUGINFO_H
>>>> >
>>>> > +#include "llvm/Support/Casting.h"
>>>> > #include "llvm/ADT/DenseMap.h"
>>>> > #include "llvm/ADT/SmallPtrSet.h"
>>>> > #include "llvm/ADT/SmallVector.h"
>>>> > @@ -46,7 +47,7 @@ namespace llvm {
>>>> > class DILexicalBlockFile;
>>>> > class DIVariable;
>>>> > class DIType;
>>>> > - class DIScopeRef;
>>>> > + class DIScope;
>>>> > class DIObjCProperty;
>>>> >
>>>> > /// Maps from type identifier to the actual MDNode.
>>>> > @@ -56,9 +57,10 @@ namespace llvm {
>>>> > /// This should not be stored in a container, because the
>>>> underlying MDNode
>>>> > /// may change in certain situations.
>>>> > class DIDescriptor {
>>>> > - // Befriends DIScopeRef so DIScopeRef can befriend the protected
>>>> member
>>>> > - // function: getFieldAs<DIScopeRef>.
>>>> > - friend class DIScopeRef;
>>>> > + // Befriends DIRef so DIRef can befriend the protected member
>>>> > + // function: getFieldAs<DIRef>.
>>>> > + template <typename T>
>>>> > + friend class DIRef;
>>>> > public:
>>>> > enum {
>>>> > FlagPrivate = 1 << 0,
>>>> > @@ -151,10 +153,6 @@ namespace llvm {
>>>> > void dump() const;
>>>> > };
>>>> >
>>>> > - /// npecialize getFieldAs to handle fields that are references to
>>>> DIScopes.
>>>> > - template <>
>>>> > - DIScopeRef DIDescriptor::getFieldAs<DIScopeRef>(unsigned Elt)
>>>> const;
>>>> > -
>>>> > /// DISubrange - This is used to represent ranges, for array
>>>> bounds.
>>>> > class DISubrange : public DIDescriptor {
>>>> > friend class DIDescriptor;
>>>> > @@ -192,6 +190,10 @@ namespace llvm {
>>>> > bool Verify() const;
>>>> > };
>>>> >
>>>> > + template <typename T>
>>>> > + class DIRef;
>>>> > + typedef DIRef<DIScope> DIScopeRef;
>>>>
>>>> Where's the corresponding dityperef? Could we just declare it here for
>>>> consistency?
>>>>
>>> Sure, I can move it here. Right now, DITypeRef is after def of DIType.
>>>
>>
>> Given we already have a forward declaration of DIType up the top, we
>> could move the typedef up here I think.
>>
>>
>>> > +
>>>> > /// DIScope - A base class for various scopes.
>>>> > class DIScope : public DIDescriptor {
>>>> > protected:
>>>> > @@ -208,23 +210,7 @@ namespace llvm {
>>>> >
>>>> > /// Generate a reference to this DIScope. Uses the type
>>>> identifier instead
>>>> > /// of the actual MDNode if possible, to help type uniquing.
>>>> > - Value *generateRef();
>>>> > - };
>>>> > -
>>>> > - /// Represents reference to a DIScope, abstracts over direct and
>>>> > - /// identifier-based metadata scope references.
>>>> > - class DIScopeRef {
>>>> > - template <typename DescTy>
>>>> > - friend DescTy DIDescriptor::getFieldAs(unsigned Elt) const;
>>>> > - friend DIScopeRef DIScope::getContext() const;
>>>> > -
>>>> > - /// Val can be either a MDNode or a MDString, in the latter,
>>>> > - /// MDString specifies the type identifier.
>>>> > - const Value *Val;
>>>> > - explicit DIScopeRef(const Value *V);
>>>> > - public:
>>>> > - DIScope resolve(const DITypeIdentifierMap &Map) const;
>>>> > - operator Value *() const { return const_cast<Value*>(Val); }
>>>> > + DIScopeRef generateRef();
>>>> > };
>>>> >
>>>> > /// DIType - This is a wrapper for a type.
>>>> > @@ -241,7 +227,7 @@ namespace llvm {
>>>> > /// Verify - Verify that a type descriptor is well formed.
>>>> > bool Verify() const;
>>>> >
>>>> > - DIScopeRef getContext() const { return
>>>> getFieldAs<DIScopeRef>(2); }
>>>> > + DIScopeRef getContext() const;
>>>>
>>>> Why did this become our of line?
>>>>
>>> Because of dependency issues,
>>>
>>
>> What's the dependency cycle?
>>
>>
>>> we can't construct a DIScopeRef here without a full definition.
>>> And the full def of DIRef needs to be after the full def of DIType since
>>> we now have the def of DIRef::resolve in the header file.
>>>
>>
>> This seems a bit strange, since DIRef shouldn't be specific to type or
>> scope, ideally.
>>
>
> It was the assertion in resolve:
>
> assert(DIType(Iter->second).isType() &&
> > + "MDNode in DITypeIdentifierMap should be a DIType.");
>
> which requires the full def of DIType to compile.
> But we should be able to change from DIType().isType() to
> DIDescriptor().isType().
>
Right
> Then the def of DIRef will only need to be after DIScope because of
> > + friend DIScopeRef DIScope::getContext() const;
> > + friend DIScopeRef DIScope::generateRef();
>
Why would it need to be after those? It'll be calling them on a dependent
expression of type T, rather than type DIScope, right? So the template can
appear before the definition of DIScope.
Also - I'm still a bit dubious of "generateRef" - it should at least be
const & I still think "getRef" might make more sense. Or we could make a
public (explicit) ctor for DIRef<T> from const T&? But I suppose that'd be
different between a DIScope and a DIType, etc... so that's not great.
>
>
> Manman
>
>
>>
>> So I assume the reason that DIRef is specific to type is for use of
>> DIDescriptor::isType rather than isScope. Is there some way we could
>> generalize over these based on the template parameter to DIRef?
>>
>> Eric - this is a question for you too. If the "isFoo" functions were
>> instead members of the relevant DI* functions ("isValid" or is that too
>> subtly different to "Verify"?) then we could use "bool T::is()" (bikeshed
>> as appropriate) in this template.
>>
>> Of course that would mean rewriting existing calls to x.isFoo() functions
>> into x.is() or Foo(x).is(), or keeping the duplicates (which would seem
>> a bit unfortunate).
>>
>>
>>> Moving resolve to the header file is to avoid a link error.
>>>
>> > StringRef getName() const { return getStringField(3);
>>>> }
>>>> > unsigned getLineNumber() const { return
>>>> getUnsignedField(4); }
>>>> > uint64_t getSizeInBits() const { return getUInt64Field(5); }
>>>> > @@ -297,6 +283,53 @@ namespace llvm {
>>>> > void replaceAllUsesWith(MDNode *D);
>>>> > };
>>>> >
>>>> > + /// Represents reference to a DIDescriptor, abstracts over direct
>>>> and
>>>> > + /// identifier-based metadata references.
>>>> > + template <typename T>
>>>> > + class DIRef {
>>>> > + template <typename DescTy>
>>>> > + friend DescTy DIDescriptor::getFieldAs(unsigned Elt) const;
>>>> > + friend DIScopeRef DIScope::getContext() const;
>>>> > + friend DIScopeRef DIScope::generateRef();
>>>> > +
>>>> > + /// Val can be either a MDNode or a MDString, in the latter,
>>>> > + /// MDString specifies the type identifier.
>>>> > + const Value *Val;
>>>> > + explicit DIRef(const Value *V);
>>>> > + public:
>>>> > + T resolve(const DITypeIdentifierMap &Map) const {
>>>> > + if (!Val)
>>>> > + return T();
>>>> > +
>>>> > + if (const MDNode *MD = dyn_cast<MDNode>(Val))
>>>> > + return T(MD);
>>>> > +
>>>> > + const MDString *MS = cast<MDString>(Val);
>>>> > + // Find the corresponding MDNode.
>>>> > + DITypeIdentifierMap::const_iterator Iter = Map.find(MS);
>>>> > + assert(Iter != Map.end() && "Identifier not in the type map?");
>>>> > + assert(DIType(Iter->second).isType() &&
>>>> > + "MDNode in DITypeIdentifierMap should be a DIType.");
>>>>
>>>> Could we write this generically somehow? This looks like a type-secific
>>>> (doesn't work for scopes) implementation.
>>>>
>>> The MDNode found in the TypeIdentifierMap should be a DIType, right?
>>>
>>>> > + return T(Iter->second);
>>>> > + }
>>>> > + operator Value *() const { return const_cast<Value*>(Val); }
>>>> > + };
>>>> > +
>>>> > + /// Specialize getFieldAs to handle fields that are references to
>>>> DIScopes.
>>>> > + template <>
>>>> > + DIScopeRef DIDescriptor::getFieldAs<DIScopeRef>(unsigned Elt)
>>>> const;
>>>> > + /// Specialize DIRef constructor for DIScopeRef.
>>>>
>>>> Why?
>>>>
>>> Just to have more specific checking of the passed-in Value: use
>>> isScopeRef in the constructor.
>>>
>>>> > + template <>
>>>> > + DIRef<DIScope>::DIRef(const Value *V);
>>>> > +
>>>> > + typedef DIRef<DIType> DITypeRef;
>>>> > + /// Specialize getFieldAs to handle fields that are references to
>>>> DITypes.
>>>> > + template <>
>>>> > + DITypeRef DIDescriptor::getFieldAs<DITypeRef>(unsigned Elt) const;
>>>> > + /// Specialize DIRef constructor for DITypeRef.
>>>> > + template <>
>>>> > + DIRef<DIType>::DIRef(const Value *V);
>>>>
>>>> And this ?
>>>>
>>> To have more specific checking of the passed-in Value: use isTypeRef in
>>> the constructor.
>>>
>>> Manman
>>>
>>>> > +
>>>> > /// DIBasicType - A basic type, like 'int' or 'float'.
>>>> > class DIBasicType : public DIType {
>>>> > public:
>>>> > @@ -328,9 +361,9 @@ namespace llvm {
>>>> > /// associated with one.
>>>> > MDNode *getObjCProperty() const;
>>>> >
>>>> > - DIScopeRef getClassType() const {
>>>> > + DITypeRef getClassType() const {
>>>> > assert(getTag() == dwarf::DW_TAG_ptr_to_member_type);
>>>> > - return getFieldAs<DIScopeRef>(10);
>>>> > + return getFieldAs<DITypeRef>(10);
>>>> > }
>>>> >
>>>> > Constant *getConstant() const {
>>>> > @@ -358,8 +391,8 @@ namespace llvm {
>>>> > void setTypeArray(DIArray Elements, DIArray TParams = DIArray());
>>>> > void addMember(DIDescriptor D);
>>>> > unsigned getRunTimeLang() const { return getUnsignedField(11); }
>>>> > - DIScopeRef getContainingType() const {
>>>> > - return getFieldAs<DIScopeRef>(12);
>>>> > + DITypeRef getContainingType() const {
>>>> > + return getFieldAs<DITypeRef>(12);
>>>> > }
>>>> > void setContainingType(DICompositeType ContainingType);
>>>> > DIArray getTemplateParams() const { return
>>>> getFieldAs<DIArray>(13); }
>>>> > @@ -426,8 +459,8 @@ namespace llvm {
>>>> > unsigned getVirtuality() const { return getUnsignedField(10); }
>>>> > unsigned getVirtualIndex() const { return getUnsignedField(11); }
>>>> >
>>>> > - DIScopeRef getContainingType() const {
>>>> > - return getFieldAs<DIScopeRef>(12);
>>>> > + DITypeRef getContainingType() const {
>>>> > + return getFieldAs<DITypeRef>(12);
>>>> > }
>>>> >
>>>> > unsigned getFlags() const {
>>>> >
>>>> > Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>> > URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=190418&r1=190417&r2=190418&view=diff
>>>> >
>>>> ==============================================================================
>>>> > --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>>>> > +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Tue Sep 10
>>>> 13:30:07 2013
>>>> > @@ -2644,8 +2644,3 @@ void DwarfDebug::emitDebugStrDWO() {
>>>> >
>>>> InfoHolder.emitStrings(Asm->getObjFileLowering().getDwarfStrDWOSection(),
>>>> > OffSec, StrSym);
>>>> > }
>>>> > -
>>>> > -/// Find the MDNode for the given scope reference.
>>>> > -DIScope DwarfDebug::resolve(DIScopeRef SRef) const {
>>>> > - return SRef.resolve(TypeIdentifierMap);
>>>> > -}
>>>> >
>>>> > Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>>> > URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h?rev=190418&r1=190417&r2=190418&view=diff
>>>> >
>>>> ==============================================================================
>>>> > --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h (original)
>>>> > +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h Tue Sep 10
>>>> 13:30:07 2013
>>>> > @@ -684,7 +684,10 @@ public:
>>>> > unsigned getDwarfVersion() const { return DwarfVersion; }
>>>> >
>>>> > /// Find the MDNode for the given scope reference.
>>>> > - DIScope resolve(DIScopeRef SRef) const;
>>>> > + template <typename T>
>>>> > + T resolve(DIRef<T> Ref) const {
>>>> > + return Ref.resolve(TypeIdentifierMap);
>>>> > + }
>>>> >
>>>> > /// isSubprogramContext - Return true if Context is either a
>>>> subprogram
>>>> > /// or another context nested inside a subprogram.
>>>> >
>>>> > Modified: llvm/trunk/lib/IR/DebugInfo.cpp
>>>> > URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=190418&r1=190417&r2=190418&view=diff
>>>> >
>>>> ==============================================================================
>>>> > --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
>>>> > +++ llvm/trunk/lib/IR/DebugInfo.cpp Tue Sep 10 13:30:07 2013
>>>> > @@ -725,13 +725,13 @@ void DICompositeType::addMember(DIDescri
>>>> >
>>>> > /// Generate a reference to this DIType. Uses the type identifier
>>>> instead
>>>> > /// of the actual MDNode if possible, to help type uniquing.
>>>> > -Value *DIScope::generateRef() {
>>>> > +DIScopeRef DIScope::generateRef() {
>>>> > if (!isCompositeType())
>>>> > - return *this;
>>>> > + return DIScopeRef(*this);
>>>> > DICompositeType DTy(DbgNode);
>>>> > if (!DTy.getIdentifier())
>>>> > - return *this;
>>>> > - return DTy.getIdentifier();
>>>> > + return DIScopeRef(*this);
>>>> > + return DIScopeRef(DTy.getIdentifier());
>>>> > }
>>>> >
>>>> > /// \brief Set the containing type.
>>>> > @@ -1432,26 +1432,14 @@ void DIVariable::printExtendedName(raw_o
>>>> > }
>>>> > }
>>>> >
>>>> > -DIScopeRef::DIScopeRef(const Value *V) : Val(V) {
>>>> > +/// Specialize constructor to make sure it has the correct type.
>>>> > +template <>
>>>> > +DIRef<DIScope>::DIRef(const Value *V) : Val(V) {
>>>> > assert(isScopeRef(V) && "DIScopeRef should be a MDString or
>>>> MDNode");
>>>> > }
>>>> > -
>>>> > -/// Given a DITypeIdentifierMap, tries to find the corresponding
>>>> > -/// DIScope for a DIScopeRef.
>>>> > -DIScope DIScopeRef::resolve(const DITypeIdentifierMap &Map) const {
>>>> > - if (!Val)
>>>> > - return DIScope();
>>>> > -
>>>> > - if (const MDNode *MD = dyn_cast<MDNode>(Val))
>>>> > - return DIScope(MD);
>>>> > -
>>>> > - const MDString *MS = cast<MDString>(Val);
>>>> > - // Find the corresponding MDNode.
>>>> > - DITypeIdentifierMap::const_iterator Iter = Map.find(MS);
>>>> > - assert(Iter != Map.end() && "Identifier not in the type map?");
>>>> > - assert(DIType(Iter->second).isType() &&
>>>> > - "MDNode in DITypeIdentifierMap should be a DIType.");
>>>> > - return DIScope(Iter->second);
>>>> > +template <>
>>>> > +DIRef<DIType>::DIRef(const Value *V) : Val(V) {
>>>> > + assert(isTypeRef(V) && "DITypeRef should be a MDString or MDNode");
>>>> > }
>>>> >
>>>> > /// Specialize getFieldAs to handle fields that are references to
>>>> DIScopes.
>>>> > @@ -1459,3 +1447,12 @@ template <>
>>>> > DIScopeRef DIDescriptor::getFieldAs<DIScopeRef>(unsigned Elt) const {
>>>> > return DIScopeRef(getField(DbgNode, Elt));
>>>> > }
>>>> > +/// Specialize getFieldAs to handle fields that are references to
>>>> DITypes.
>>>> > +template <>
>>>> > +DITypeRef DIDescriptor::getFieldAs<DITypeRef>(unsigned Elt) const {
>>>> > + return DITypeRef(getField(DbgNode, Elt));
>>>> > +}
>>>> > +
>>>> > +DIScopeRef DIType::getContext() const {
>>>> > + return getFieldAs<DIScopeRef>(2);
>>>> > +}
>>>> >
>>>> >
>>>> > _______________________________________________
>>>> > llvm-commits mailing list
>>>> > llvm-commits at cs.uiuc.edu
>>>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130910/577caed1/attachment.html>
More information about the llvm-commits
mailing list