[llvm] r190322 - Debug Info: Rename DITypeRef to DIScopeRef.
David Blaikie
dblaikie at gmail.com
Mon Sep 9 13:40:38 PDT 2013
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/20130909/d56b8a8e/attachment.html>
More information about the llvm-commits
mailing list