<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Oct 7, 2013 at 2:43 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">> + /// If the scope node has a name, return that, else return an empty string.<br>
> + StringRef getName() const;<br>
> StringRef getFilename() const;<br>
> StringRef getDirectory() const;<br>
><br>
> @@ -227,6 +230,16 @@ template <typename T> class DIRef {<br>
><br>
> public:<br>
> T resolve(const DITypeIdentifierMap &Map) const;<br>
> + StringRef getName() const {<br>
> + if (!Val)<br>
> + return StringRef();<br>
> +<br>
> + if (const MDNode *MD = dyn_cast<MDNode>(Val))<br>
> + return T(MD).getName();<br>
> +<br>
> + const MDString *MS = cast<MDString>(Val);<br>
> + return MS->getString();<br>
> + }<br>
<br>
</div>Please take this out of line.<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> operator Value *() const { return const_cast<Value *>(Val); }<br>
> };<br>
><br>
> @@ -300,9 +313,6 @@ public:<br>
> bool isStaticMember() const { return (getFlags() & FlagStaticMember) != 0; }<br>
> bool isValid() const { return DbgNode && isType(); }<br>
><br>
> - /// isUnsignedDIType - Return true if type encoding is unsigned.<br>
> - bool isUnsignedDIType();<br>
> -<br>
> /// replaceAllUsesWith - Replace all uses of debug info referenced by<br>
> /// this descriptor.<br>
> void replaceAllUsesWith(DIDescriptor &D);<br>
> @@ -330,11 +340,7 @@ class DIDerivedType : public DIType {<br>
> public:<br>
> explicit DIDerivedType(const MDNode *N = 0) : DIType(N) {}<br>
><br>
> - DIType getTypeDerivedFrom() const { return getFieldAs<DIType>(9); }<br>
> -<br>
> - /// getOriginalTypeSize - If this type is derived from a base type then<br>
> - /// return base type size.<br>
> - uint64_t getOriginalTypeSize() const;<br>
> + DITypeRef getTypeDerivedFrom() const { return getFieldAs<DITypeRef>(9); }<br>
><br>
> /// getObjCProperty - Return property node, if this ivar is<br>
> /// associated with one.<br>
><br>
> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp?rev=192018&r1=192017&r2=192018&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp?rev=192018&r1=192017&r2=192018&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp (original)<br>
> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp Fri Oct 4 20:43:03 2013<br>
> @@ -520,7 +520,7 @@ void CompileUnit::addBlockByrefAddress(c<br>
><br>
> if (Tag == dwarf::DW_TAG_pointer_type) {<br>
> DIDerivedType DTy = DIDerivedType(Ty);<br>
> - TmpTy = DTy.getTypeDerivedFrom();<br>
> + TmpTy = DD->resolve(DTy.getTypeDerivedFrom());<br>
<br>
</div></div>Please use the accessor I added.<br></blockquote><div>Done </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> isPointer = true;<br>
> }<br>
><br>
> @@ -587,9 +587,10 @@ void CompileUnit::addBlockByrefAddress(c<br>
> }<br>
><br>
> /// isTypeSigned - Return true if the type is signed.<br>
> -static bool isTypeSigned(DIType Ty, int *SizeInBits) {<br>
> +static bool isTypeSigned(DwarfDebug *DD, DIType Ty, int *SizeInBits) {<br>
> if (Ty.isDerivedType())<br>
> - return isTypeSigned(DIDerivedType(Ty).getTypeDerivedFrom(), SizeInBits);<br>
> + return isTypeSigned(DD, DD->resolve(DIDerivedType(Ty).getTypeDerivedFrom()),<br>
> + SizeInBits);<br>
<br>
</div>Again.<br></blockquote><div>Done </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> if (Ty.isBasicType())<br>
> if (DIBasicType(Ty).getEncoding() == dwarf::DW_ATE_signed<br>
> || DIBasicType(Ty).getEncoding() == dwarf::DW_ATE_signed_char) {<br>
> @@ -599,6 +600,51 @@ static bool isTypeSigned(DIType Ty, int<br>
> return false;<br>
> }<br>
><br>
> +/// Return true if type encoding is unsigned.<br>
> +static bool isUnsignedDIType(DwarfDebug *DD, DIType Ty) {<br>
> + DIDerivedType DTy(Ty);<br>
> + if (DTy.isDerivedType())<br>
> + return isUnsignedDIType(DD, DD->resolve(DTy.getTypeDerivedFrom()));<br>
> +<br>
<br>
</div>Again, and everywhere else, please make sure you get them all.<br></blockquote><div>Done </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> + DIBasicType BTy(Ty);<br>
> + if (BTy.isBasicType()) {<br>
> + unsigned Encoding = BTy.getEncoding();<br>
> + if (Encoding == dwarf::DW_ATE_unsigned ||<br>
> + Encoding == dwarf::DW_ATE_unsigned_char ||<br>
> + Encoding == dwarf::DW_ATE_boolean)<br>
> + return true;<br>
> + }<br>
> + return false;<br>
> +}<br>
> +<br>
> +/// If this type is derived from a base type then return base type size.<br>
> +static uint64_t getOriginalTypeSize(DwarfDebug *DD, DIDerivedType Ty) {<br>
<br>
</div>Since we're moving and changing this one, how about getBaseTypeSize?<br></blockquote><div>Done </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also we might want to just make this a method on DwarfDebug since it<br>
has the information to connect the various references.<br></blockquote><div><br></div><div>We have a few static helper functions: isTypeSigned, getBaseTypeSize, and isUnsignedDIType.</div><div>isTypeUnitScoped traces along the context chain while the above three traces along the derived-from chain.</div>
<div>Should we make all four member functions then? DwarfDebug or CompileUnit?</div><div><br></div><div>I will rename isUnsignedDIType to isTypeUnsigned.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> - bool SignedConstant = isTypeSigned(Ty, &SizeInBits);<br>
> + bool SignedConstant = isTypeSigned(DD, Ty, &SizeInBits);<br>
<br>
</div>Same with isTypeSigned.<br></blockquote><div>See above. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
<br>
> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=192018&r1=192017&r2=192018&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=192018&r1=192017&r2=192018&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)<br>
> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Fri Oct 4 20:43:03 2013<br>
> @@ -149,13 +149,13 @@ DIType DbgVariable::getType() const {<br>
> uint16_t tag = Ty.getTag();<br>
><br>
> if (tag == dwarf::DW_TAG_pointer_type)<br>
> - subType = DIDerivedType(Ty).getTypeDerivedFrom();<br>
> + subType = DD->resolve(DIDerivedType(Ty).getTypeDerivedFrom());<br>
><br>
> DIArray Elements = DICompositeType(subType).getTypeArray();<br>
> for (unsigned i = 0, N = Elements.getNumElements(); i < N; ++i) {<br>
> DIDerivedType DT = DIDerivedType(Elements.getElement(i));<br>
> if (getName() == DT.getName())<br>
> - return (DT.getTypeDerivedFrom());<br>
> + return (DD->resolve(DT.getTypeDerivedFrom()));<br>
<br>
</div>Please add a resolve function to DbgVariable so that the interface is<br>
the same here as well.<br></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
This part of the change could have been done separately and would have<br>
made reviewing cleaner and easier.<br>
<div class="im"><br>
> +// If the scope node has a name, return that, else return an empty string.<br>
> +StringRef DIScope::getName() const {<br>
> + if (isType())<br>
> + return DIType(DbgNode).getName();<br>
> + if (isSubprogram())<br>
> + return DISubprogram(DbgNode).getName();<br>
> + if (isNameSpace())<br>
> + return DINameSpace(DbgNode).getName();<br>
> + assert((isLexicalBlock() || isLexicalBlockFile() || isFile() ||<br>
> + isCompileUnit()) && "Unhandled type of scope.");<br>
> + return StringRef();<br>
> +}<br>
> +<br>
<br>
</div>This could have been broken out separately as well.<br>
<br>
Please also construct the small testcase that accurately tests the<br>
changes you made without relying the linking of two modules.<br></blockquote><div>Incoming.</div><div><br></div><div>Manman <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks.<br>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span></blockquote></div><br></div></div>