[llvm] r190322 - Debug Info: Rename DITypeRef to DIScopeRef.
Manman Ren
manman.ren at gmail.com
Tue Sep 10 11:34:27 PDT 2013
In r190418.
Thanks,
Manman
On Mon, Sep 9, 2013 at 1:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Sep 9, 2013 at 1:29 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>>
>>
>>
>> On Mon, Sep 9, 2013 at 1:12 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>>
>>>
>>>
>>> On Mon, Sep 9, 2013 at 12:03 PM, Manman Ren <manman.ren at gmail.com>wrote:
>>>
>>>> Author: mren
>>>> Date: Mon Sep 9 14:03:51 2013
>>>> New Revision: 190322
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=190322&view=rev
>>>> Log:
>>>> Debug Info: Rename DITypeRef to DIScopeRef.
>>>>
>>>
>>> This is a little unfortunate to drop some of the type safety/implicit
>>> documentation by muddying every DIType member in the DI* hierarchy with
>>> DIScopes. Did you consider other designs that would avoid this
>>> conflation/ambiguity? Might it be worth having two types (DIScopeRef and
>>> DITypeRef - could even be typedefs of specializations of a common template)?
>>>
>> Yes, I thought about DITypeRef being a subclass of DIScopeRef, but since
>> they resolve in the same way, and
>> the only difference is during verification, I choose to implement two
>> different verification helpers (isScopeRef vs isTypeRef).
>>
>> DITypeRef should differ from DIScopeRef in the constructor: assertion on
>> isTypeRef instead of on isScopeRef.
>>
>
> Honestly the input safety is less concerning than the output. Having the
> "resolve" functions return DIScope when sometimes they're specifically
> known to return a DIType seems like a step backwards (granted I'm not sure
> why the resolve function was returning a raw MDNode* previously - it
> should've been DIType).
>
> The fact that all DITypes are also DIScopes is a quirk of a weird/bad
> inheritance hierarchy. I'd like to not rely on that whenever possible
> because it creates strange/non-sensical statements ("get me the scope from
> this return type?" what does that even mean?) which will make the entirety
> of DwarfDebug harder to understand & maintain going forwards.
>
> As a quick hypothetical, consider:
>
> template<typename T>
> class DIRef {
> template<typename DescTy>
>
> friend DescTy DIDescriptor::getFieldAs(unsigned Elt) const;
>
> const Value *Val;
> explicit DIRef(const Value *V);
> public:
> T resolve(const DITypeIdentifierMap &Map) const;
> operator Value *() const { return const_cast<Value*>(Val); }
> };
>
> typedef DIRef<DIScope> DIScopeRef;
> typedef DIRef<DIType> DITypeRef;
>
> Would that work/make sense? & then we could use a DITypeRef where we know
> it's only a type, not some other arbitrary scope, and DIScopeRef where it's
> a Scope. We could either use a more specialized template (template<typename
> T> T resolve(DIRef<T>); or a nested typedef provided by DIRef:
> template<typename T> T::RefType resolve(T); )
>
> & looking at DIType/ScopeRef, when did it grow a conversion to Value*?
> should that be private & exposed only to DIBuilder via friendship? It seems
> a bit dangerous (conversion to raw pointer - allows boolean testability,
> etc... ).
>
>
>>
>>
>>>
>>>>
>>>> A reference to a scope is more general than a reference to a type since
>>>> DIType is a subclass of DIScope.
>>>>
>>>> A reference to a type can be either an identifier for the type or
>>>> the DIType itself, while a reference to a scope can be either an
>>>> identifier for the type (when the scope is indeed a type) or the
>>>> DIScope itself. A reference to a type and a reference to a scope
>>>> will be resolved in the same way. The only difference is in the
>>>> verifier when a field is a reference to a type (i.e. the containing
>>>> type field of a DICompositeType) or a field is a reference to a scope
>>>> (i.e. the context field of a DIType).
>>>>
>>>> This is to get ready for switching DIType::getContext to return
>>>> DIScopeRef instead of DIScope.
>>>>
>>>> Tighten up isTypeRef and isScopeRef to make sure the identifier is not
>>>> empty and the MDNode is DIType for TypeRef and DIScope for ScopeRef.
>>>>
>>>> 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=190322&r1=190321&r2=190322&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/include/llvm/DebugInfo.h (original)
>>>> +++ llvm/trunk/include/llvm/DebugInfo.h Mon Sep 9 14:03:51 2013
>>>> @@ -46,7 +46,7 @@ namespace llvm {
>>>> class DILexicalBlockFile;
>>>> class DIVariable;
>>>> class DIType;
>>>> - class DITypeRef;
>>>> + class DIScopeRef;
>>>> class DIObjCProperty;
>>>>
>>>> /// Maps from type identifier to the actual MDNode.
>>>> @@ -56,9 +56,9 @@ namespace llvm {
>>>> /// This should not be stored in a container, because the underlying
>>>> MDNode
>>>> /// may change in certain situations.
>>>> class DIDescriptor {
>>>> - // Befriends DITypeRef so DITypeRef can befriend the protected
>>>> member
>>>> - // function: getFieldAs<DITypeRef>.
>>>> - friend class DITypeRef;
>>>> + // Befriends DIScopeRef so DIScopeRef can befriend the protected
>>>> member
>>>> + // function: getFieldAs<DIScopeRef>.
>>>> + friend class DIScopeRef;
>>>> public:
>>>> enum {
>>>> FlagPrivate = 1 << 0,
>>>> @@ -151,9 +151,9 @@ namespace llvm {
>>>> void dump() const;
>>>> };
>>>>
>>>> - /// Specialize getFieldAs to handle fields that are references to
>>>> DITypes.
>>>> + /// npecialize getFieldAs to handle fields that are references to
>>>> DIScopes.
>>>> template <>
>>>> - DITypeRef DIDescriptor::getFieldAs<DITypeRef>(unsigned Elt) const;
>>>> + DIScopeRef DIDescriptor::getFieldAs<DIScopeRef>(unsigned Elt) const;
>>>>
>>>> /// DISubrange - This is used to represent ranges, for array bounds.
>>>> class DISubrange : public DIDescriptor {
>>>> @@ -205,6 +205,25 @@ namespace llvm {
>>>> DIScope getContext() const;
>>>> StringRef getFilename() const;
>>>> StringRef getDirectory() const;
>>>> +
>>>> + /// 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;
>>>> +
>>>> + /// 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); }
>>>> };
>>>>
>>>> /// DIType - This is a wrapper for a type.
>>>> @@ -271,32 +290,12 @@ namespace llvm {
>>>> /// isUnsignedDIType - Return true if type encoding is unsigned.
>>>> bool isUnsignedDIType();
>>>>
>>>> - /// Generate a reference to this DIType. Uses the type identifier
>>>> instead
>>>> - /// of the actual MDNode if possible, to help type uniquing.
>>>> - DITypeRef generateRef();
>>>> -
>>>> /// replaceAllUsesWith - Replace all uses of debug info referenced
>>>> by
>>>> /// this descriptor.
>>>> void replaceAllUsesWith(DIDescriptor &D);
>>>> void replaceAllUsesWith(MDNode *D);
>>>> };
>>>>
>>>> - /// Represents reference to a DIType, abstracts over direct and
>>>> - /// identifier-based metadata type references.
>>>> - class DITypeRef {
>>>> - template <typename DescTy>
>>>> - friend DescTy DIDescriptor::getFieldAs(unsigned Elt) const;
>>>> - friend DITypeRef DIType::generateRef();
>>>> -
>>>> - /// TypeVal can be either a MDNode or a MDString, in the latter,
>>>> - /// MDString specifies the type identifier.
>>>> - const Value *TypeVal;
>>>> - explicit DITypeRef(const Value *V);
>>>> - public:
>>>> - DIType resolve(const DITypeIdentifierMap &Map) const;
>>>> - operator Value *() const { return const_cast<Value*>(TypeVal); }
>>>> - };
>>>> -
>>>> /// DIBasicType - A basic type, like 'int' or 'float'.
>>>> class DIBasicType : public DIType {
>>>> public:
>>>> @@ -328,9 +327,9 @@ namespace llvm {
>>>> /// associated with one.
>>>> MDNode *getObjCProperty() const;
>>>>
>>>> - DITypeRef getClassType() const {
>>>> + DIScopeRef getClassType() const {
>>>> assert(getTag() == dwarf::DW_TAG_ptr_to_member_type);
>>>> - return getFieldAs<DITypeRef>(10);
>>>> + return getFieldAs<DIScopeRef>(10);
>>>> }
>>>>
>>>> Constant *getConstant() const {
>>>> @@ -358,8 +357,8 @@ namespace llvm {
>>>> void setTypeArray(DIArray Elements, DIArray TParams = DIArray());
>>>> void addMember(DIDescriptor D);
>>>> unsigned getRunTimeLang() const { return getUnsignedField(11); }
>>>> - DITypeRef getContainingType() const {
>>>> - return getFieldAs<DITypeRef>(12);
>>>> + DIScopeRef getContainingType() const {
>>>> + return getFieldAs<DIScopeRef>(12);
>>>> }
>>>> void setContainingType(DICompositeType ContainingType);
>>>> DIArray getTemplateParams() const { return
>>>> getFieldAs<DIArray>(13); }
>>>> @@ -426,8 +425,8 @@ namespace llvm {
>>>> unsigned getVirtuality() const { return getUnsignedField(10); }
>>>> unsigned getVirtualIndex() const { return getUnsignedField(11); }
>>>>
>>>> - DITypeRef getContainingType() const {
>>>> - return getFieldAs<DITypeRef>(12);
>>>> + DIScopeRef getContainingType() const {
>>>> + return getFieldAs<DIScopeRef>(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=190322&r1=190321&r2=190322&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Mon Sep 9
>>>> 14:03:51 2013
>>>> @@ -2633,7 +2633,7 @@ void DwarfDebug::emitDebugStrDWO() {
>>>> OffSec, StrSym);
>>>> }
>>>>
>>>> -/// Find the MDNode for the given type reference.
>>>> -MDNode *DwarfDebug::resolve(DITypeRef TRef) const {
>>>> - return TRef.resolve(TypeIdentifierMap);
>>>> +/// 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=190322&r1=190321&r2=190322&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h (original)
>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h Mon Sep 9 14:03:51
>>>> 2013
>>>> @@ -683,8 +683,8 @@ public:
>>>> /// Returns the Dwarf Version.
>>>> unsigned getDwarfVersion() const { return DwarfVersion; }
>>>>
>>>> - /// Find the MDNode for the given type reference.
>>>> - MDNode *resolve(DITypeRef TRef) const;
>>>> + /// Find the MDNode for the given scope reference.
>>>> + DIScope resolve(DIScopeRef SRef) const;
>>>>
>>>> };
>>>> } // End of namespace llvm
>>>>
>>>> Modified: llvm/trunk/lib/IR/DebugInfo.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=190322&r1=190321&r2=190322&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
>>>> +++ llvm/trunk/lib/IR/DebugInfo.cpp Mon Sep 9 14:03:51 2013
>>>> @@ -426,17 +426,26 @@ static bool fieldIsMDString(const MDNode
>>>> return !Fld || isa<MDString>(Fld);
>>>> }
>>>>
>>>> -/// Check if a value can be a TypeRef.
>>>> +/// Check if a value can be a reference to a type.
>>>> static bool isTypeRef(const Value *Val) {
>>>> - return !Val || isa<MDString>(Val) || isa<MDNode>(Val);
>>>> + return !Val ||
>>>> + (isa<MDString>(Val) &&
>>>> !cast<MDString>(Val)->getString().empty()) ||
>>>> + (isa<MDNode>(Val) && DIType(cast<MDNode>(Val)).isType());
>>>> }
>>>>
>>>> -/// Check if a field at position Elt of a MDNode can be a TypeRef.
>>>> +/// Check if a field at position Elt of a MDNode can be a reference to
>>>> a type.
>>>> static bool fieldIsTypeRef(const MDNode *DbgNode, unsigned Elt) {
>>>> Value *Fld = getField(DbgNode, Elt);
>>>> return isTypeRef(Fld);
>>>> }
>>>>
>>>> +/// Check if a value can be a ScopeRef.
>>>> +static bool isScopeRef(const Value *Val) {
>>>> + return !Val ||
>>>> + (isa<MDString>(Val) &&
>>>> !cast<MDString>(Val)->getString().empty()) ||
>>>> + (isa<MDNode>(Val) && DIScope(cast<MDNode>(Val)).isScope());
>>>> +}
>>>> +
>>>> /// Verify - Verify that a type descriptor is well formed.
>>>> bool DIType::Verify() const {
>>>> if (!isType())
>>>> @@ -710,13 +719,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.
>>>> -DITypeRef DIType::generateRef() {
>>>> +Value *DIScope::generateRef() {
>>>>
>>>
>>> Why did the return type of this function change?
>>>
>> This function is used by DIBuilder only, which uses the return value as a
>> "Value *", so I feel
>> it is unnecessary to invoke the constructor then another conversion
>> operator.
>>
>
> Except it's a public member of DIType - I rather like the type safety
> (even if it weren't a public member). The conversions are fairly trivial &
> shouldn't be hurting performance.
>
>
>>
>>
>>>
>>>
>>>> if (!isCompositeType())
>>>> - return DITypeRef(*this);
>>>> + return *this;
>>>> DICompositeType DTy(DbgNode);
>>>> if (!DTy.getIdentifier())
>>>> - return DITypeRef(*this);
>>>> - return DITypeRef(DTy.getIdentifier());
>>>> + return *this;
>>>> + return DTy.getIdentifier();
>>>> }
>>>>
>>>> /// \brief Set the containing type.
>>>> @@ -1428,33 +1437,30 @@ void DIVariable::printExtendedName(raw_o
>>>> }
>>>> }
>>>>
>>>> -DITypeRef::DITypeRef(const Value *V) : TypeVal(V) {
>>>> - assert(isTypeRef(V) && "DITypeRef should be a MDString or MDNode");
>>>> +DIScopeRef::DIScopeRef(const Value *V) : Val(V) {
>>>> + assert(isScopeRef(V) && "DIScopeRef should be a MDString or MDNode");
>>>> }
>>>>
>>>> /// Given a DITypeIdentifierMap, tries to find the corresponding
>>>> -/// DIType for a DITypeRef.
>>>> -DIType DITypeRef::resolve(const DITypeIdentifierMap &Map) const {
>>>> - if (!TypeVal)
>>>> - return NULL;
>>>> -
>>>> - if (const MDNode *MD = dyn_cast<MDNode>(TypeVal)) {
>>>> - assert(DIType(MD).isType() &&
>>>> - "MDNode in DITypeRef should be a DIType.");
>>>> - return MD;
>>>> - }
>>>> +/// DIScope for a DIScopeRef.
>>>> +DIScope DIScopeRef::resolve(const DITypeIdentifierMap &Map) const {
>>>> + if (!Val)
>>>> + return DIScope();
>>>>
>>>
>>> Why this rather than the original "return NULL;"?
>>>
>>>
>>>> +
>>>> + if (const MDNode *MD = dyn_cast<MDNode>(Val))
>>>> + return DIScope(MD);
>>>>
>>>
>>> Why this rather than the original "return MD;"?
>>>
>>>
>>>>
>>>> - const MDString *MS = cast<MDString>(TypeVal);
>>>> + 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 Iter->second;
>>>> + return DIScope(Iter->second);
>>>>
>>>
>>> Why did this explicit ctor invocation get added?
>>>
>>> I'm guessing because DIScope's DIScope(MDNode*) ctor is explicit? Unlike
>>> all the other DI* types MDNode* ctors? Perhaps we could change it to be
>>> consistent (assuming there is such consistency).
>>>
>> True, unfortunately, I didn't find any consistency ;) Most of the
>> constructors are explicit, except DIType.
>>
>
> OK - if DIType is the exception, rather than the rule, that's fine - I'm
> happy with more explicitness. Implicit DIFoo->MDNode* is pretty dodgy
> anyway.
>
>
>>
>> Thanks,
>> Manman
>>
>>
>>>
>>>
>>>
>>>> }
>>>>
>>>> -/// Specialize getFieldAs to handle fields that are references to
>>>> DITypes.
>>>> +/// Specialize getFieldAs to handle fields that are references to
>>>> DIScopes.
>>>> template <>
>>>> -DITypeRef DIDescriptor::getFieldAs<DITypeRef>(unsigned Elt) const {
>>>> - return DITypeRef(getField(DbgNode, Elt));
>>>> +DIScopeRef DIDescriptor::getFieldAs<DIScopeRef>(unsigned Elt) const {
>>>> + return DIScopeRef(getField(DbgNode, Elt));
>>>> }
>>>>
>>>>
>>>> _______________________________________________
>>>> 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/23f4d634/attachment.html>
More information about the llvm-commits
mailing list