<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 21, 2014 at 1:14 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">On Mon, Jul 21, 2014 at 10:39 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>

><br>
><br>
><br>
> On Mon, Jul 14, 2014 at 11:32 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
>><br>
>><br>
>> We still have access to types via MDNodes directly and the assertion that<br>
>> assumes all accesses to DITypes are accessing the resolved DIType will fire<br>
>><br>
>> i.e assert(Ty == resolve(Ty.getRef()))<br>
>><br>
>> One example is the access to DIType via DIArray in SubroutineType. If all<br>
>> elements in the type array are DITypes we can create a DITypeArray and use<br>
>> that for SubroutineType's type array instead. But we currently have<br>
>> unspecified parameter in the type array and it is not a DIType.<br>
><br>
><br>
> I am going to work on a patch that adds DITypeArray (each element will be<br>
> DITypeRef, SubroutineType's type array will be DITypeArray) and adds<br>
> DITrivialType that extends from DIType (unspecified parameter will be<br>
> DITrivialType).<br>
> If you  have opinions against it, please let me know,<br>
<br>
</div>We haven't bothered using typed arrays in DebugInfo yet (as you say,<br>
we just have DIArray) so I have two thoughts<br>
<br>
1) why does this one case need fixing/changing? Is it because we have<br>
things that aren't DIDescriptors inside the DIArray? (the strings that<br>
refer to types). Given how loosely typed DIDescriptor is (it doesn't<br>
check that it's a valid DIDescriptor) I assume this doesn't actually<br>
cause a problem, though it's certainly not nice. So we could just<br>
leave it as-is, pass DIArray's element to "resolve" (it'd implicitly<br>
convert the DIDescriptor back to a raw MDNode*), then perhaps we'd<br>
need to make DITypeRef's ctor public so it could be used here. Not<br>
suggesting that's ideal, though.<br></blockquote><div><br></div><div>I should have provided an example to help understanding the issue :)</div><div><br></div><div><div>When processing the following type node, we throw an assertion failure assert(Ty == resolve(Ty.getRef()))</div>
<div>!{i32 786436, metadata <badref>, null, metadata !"SpuPacketType", i32 102, i64 32, i64 32, i32 0, i32 0, null, metadata <badref>, i32 0, null, null, metadata !"_ZTS13SpuPacketType"} ; [ DW_TAG_enumeration_type ] [SpuPacketType] [line 102, size 32, align 32, offset 0] [def] [from ]</div>
<div><br></div><div>There are two type nodes with the same identifier:</div><div>!473 = metadata !{i32 786436, metadata !474, null, metadata !"SpuPacketType", i32 102, i64 32, i64 32, i32 0, i32 0, null, metadata !475, i32 0, null, null, metadata !"_ZTS13SpuPacketType"} ; [ DW_TAG_enumeration_type ] [SpuPacketType] [line 102, size 32, align 32, offset 0] [def] [from ]</div>
<div>!6695 = metadata !{i32 786436, metadata !6696, null, metadata !"SpuPacketType", i32 102, i64 32, i64 32, i32 0, i32 0, null, metadata !475, i32 0, null, null, metadata !"_ZTS13SpuPacketType"} ; [ DW_TAG_enumeration_type ] [SpuPacketType] [line 102, size 32, align 32, offset 0] [def] [from ]</div>
<div><br></div><div>The only difference between these two is the file node</div><div>!474 = metadata !{metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71/SPU/SPUPacket.h", metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71"}</div>
<div>!6696 = metadata !{metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71/SPU/../SPU/SPUPacket.h", metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71"}</div></div><div><br>
</div><div>We have direct access to 473 via 580's type array.</div><div>!580 = metadata !{i32 786453, i32 0, null, metadata !"", i32 0, i64 0, i64 0, i64 0, i32 0, null, metadata !581, i32 0, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]</div>
<div>!581 = metadata !{metadata !124, metadata !575, metadata !473, metadata !582, metadata !212, metadata !128, metadata !584}</div><div><br></div><div>MDNode 473 will be resolved to MDNode 6695 and the assertion "assert(Ty == resolve(Ty.getRef()))" will fire.</div>
<div><br></div><div>-------------------------------------------------</div><div>To fix the problem, we need to remove the direct access to MDNode 473 by replacing MDNode 581 from</div><div>metadata !{metadata !124, metadata !575, metadata !473, metadata !582, metadata !212, metadata !128, metadata !584}<br>
</div><div>to</div><div>metadata !{metadata !124, metadata !575, metadata !"_ZTS13SpuPacketType", metadata !582, metadata !212, metadata !128, metadata !584}<br></div><div><br></div><div>And treat the field {metadata !"_ZTS13SpuPacketType"} as DITypeRef.</div>
<div><br></div><div>-------------------------------------------------<br></div><div>If we have DIDescriptorRef and all elements currently inside DIArray are DIDescirptors, we can make DIArray an array of DIDescriptorRef.</div>
<div>I don't think it is a good idea to add DIDescriptorRef (it makes our types loose) and am not sure about the 2nd condition.</div><div><br></div><div>So I proposed to add DITypeArray (or DITypedArray<DITypeRef> as David suggested, where all elements are DITypeRef), DICompositeType::getTypeArray() will return DITypeArray and DITypeArray::getElement(unsigned) will return DITypeRef.</div>
<div><br></div><div>This is actually more complicated than I thought, not all DICompositeType's getTypeArray() can return an array of DITypeRefs. For example, getTypeArray() of ArrayType and VectorType can not return an array of DITypeRefs.</div>
<div>We can fix that by extending DICompositeType to DISubroutineType and only DISubroutineType::getTypeArray() will return DITypeArray.</div><div>Even for SubroutineType, elements of the type array can be unspecified parameters which can't be DITypeRefs. That is why I was thinking about making unspecified parameters trivial DITypes.</div>
<div><br></div><div>Thanks a lot,</div><div>Manman</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<br>
2) If we're going to fix DIArray apparent type safety (it's not safe -<br>
just convenient), perhaps we could just template it? (to avoid churn,<br>
we could leave DIArray as a typedef of DITypedArray<DIDescriptor> for<br>
example, and then have DITypedArray<DITypeRef> which is your<br>
DITypeArray (again, provided via typedef)). It's so small though, that<br>
I'm not too fussed if we write it out again as you've proposed.<br>
<span class=""><font color="#888888"><br>
- David<br>
</font></span><div class=""><div class="h5"><br>
><br>
> Thanks,<br>
> Manman<br>
><br>
>><br>
>> What are your thoughts? Suggestions are welcome.<br>
>><br>
>> Is it a good idea to canonicalize file names (i.e dA/B.h should be<br>
>> equivalent to dA/../dA/B.h)? This will reduce the chance of having two<br>
>> DITypes that should be equivalent with equivalent file names.<br>
>><br>
>> Thanks,<br>
>> Manman<br>
><br>
><br>
</div></div></blockquote></div><br></div></div>