<div dir="ltr"><br><div>The first patch is in r214111.</div><div><br></div><div>Thanks,</div><div>Manman</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 21, 2014 at 4:59 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Jul 21, 2014 at 4:55 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>

><br>
><br>
><br>
> On Mon, Jul 21, 2014 at 3:54 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Mon, Jul 21, 2014 at 3:48 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> ><br>
>> > On Mon, Jul 21, 2014 at 3:41 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Mon, Jul 21, 2014 at 3:35 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>><br>
>> >> wrote:<br>
>> >> ><br>
>> >> ><br>
>> >> ><br>
>> >> > On Mon, Jul 21, 2014 at 1:14 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> On Mon, Jul 21, 2014 at 10:39 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>><br>
>> >> >> wrote:<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > On Mon, Jul 14, 2014 at 11:32 AM, Manman Ren<br>
>> >> >> > <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>><br>
>> >> >> > wrote:<br>
>> >> >> >><br>
>> >> >> >><br>
>> >> >> >> We still have access to types via MDNodes directly and the<br>
>> >> >> >> assertion<br>
>> >> >> >> that<br>
>> >> >> >> assumes all accesses to DITypes are accessing the resolved DIType<br>
>> >> >> >> will<br>
>> >> >> >> fire<br>
>> >> >> >><br>
>> >> >> >> i.e assert(Ty == resolve(Ty.getRef()))<br>
>> >> >> >><br>
>> >> >> >> One example is the access to DIType via DIArray in<br>
>> >> >> >> SubroutineType.<br>
>> >> >> >> If<br>
>> >> >> >> all<br>
>> >> >> >> elements in the type array are DITypes we can create a<br>
>> >> >> >> DITypeArray<br>
>> >> >> >> and<br>
>> >> >> >> use<br>
>> >> >> >> that for SubroutineType's type array instead. But we currently<br>
>> >> >> >> 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<br>
>> >> >> > will<br>
>> >> >> > be<br>
>> >> >> > DITypeRef, SubroutineType's type array will be DITypeArray) and<br>
>> >> >> > adds<br>
>> >> >> > DITrivialType that extends from DIType (unspecified parameter will<br>
>> >> >> > be<br>
>> >> >> > DITrivialType).<br>
>> >> >> > If you  have opinions against it, please let me know,<br>
>> >> >><br>
>> >> >> 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<br>
>> >> >> have<br>
>> >> >> things that aren't DIDescriptors inside the DIArray? (the strings<br>
>> >> >> 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>
>> >> ><br>
>> >> ><br>
>> >> > I should have provided an example to help understanding the issue :)<br>
>> >> ><br>
>> >> > When processing the following type node, we throw an assertion<br>
>> >> > failure<br>
>> >> > assert(Ty == resolve(Ty.getRef()))<br>
>> >> > !{i32 786436, metadata <badref>, null, metadata !"SpuPacketType", i32<br>
>> >> > 102,<br>
>> >> > i64 32, i64 32, i32 0, i32 0, null, metadata <badref>, i32 0, null,<br>
>> >> > null,<br>
>> >> > metadata !"_ZTS13SpuPacketType"} ; [ DW_TAG_enumeration_type ]<br>
>> >> > [SpuPacketType] [line 102, size 32, align 32, offset 0] [def] [from ]<br>
>> >> ><br>
>> >> > There are two type nodes with the same identifier:<br>
>> >> > !473 = metadata !{i32 786436, metadata !474, null, metadata<br>
>> >> > !"SpuPacketType", i32 102, i64 32, i64 32, i32 0, i32 0, null,<br>
>> >> > metadata<br>
>> >> > !475, i32 0, null, null, metadata !"_ZTS13SpuPacketType"} ; [<br>
>> >> > DW_TAG_enumeration_type ] [SpuPacketType] [line 102, size 32, align<br>
>> >> > 32,<br>
>> >> > offset 0] [def] [from ]<br>
>> >> > !6695 = metadata !{i32 786436, metadata !6696, null, metadata<br>
>> >> > !"SpuPacketType", i32 102, i64 32, i64 32, i32 0, i32 0, null,<br>
>> >> > metadata<br>
>> >> > !475, i32 0, null, null, metadata !"_ZTS13SpuPacketType"} ; [<br>
>> >> > DW_TAG_enumeration_type ] [SpuPacketType] [line 102, size 32, align<br>
>> >> > 32,<br>
>> >> > offset 0] [def] [from ]<br>
>> >> ><br>
>> >> > The only difference between these two is the file node<br>
>> >> > !474 = metadata !{metadata<br>
>> >> ><br>
>> >> ><br>
>> >> > !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71/SPU/SPUPacket.h",<br>
>> >> > metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71"}<br>
>> >> > !6696 = metadata !{metadata<br>
>> >> ><br>
>> >> ><br>
>> >> > !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71/SPU/../SPU/SPUPacket.h",<br>
>> >> > metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71"}<br>
>> >> ><br>
>> >> > We have direct access to 473 via 580's type array.<br>
>> >> > !580 = metadata !{i32 786453, i32 0, null, metadata !"", i32 0, i64<br>
>> >> > 0,<br>
>> >> > i64<br>
>> >> > 0, i64 0, i32 0, null, metadata !581, i32 0, null, null, null} ; [<br>
>> >> > DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]<br>
>> >> > !581 = metadata !{metadata !124, metadata !575, metadata !473,<br>
>> >> > metadata<br>
>> >> > !582, metadata !212, metadata !128, metadata !584}<br>
>> >> ><br>
>> >> > MDNode 473 will be resolved to MDNode 6695 and the assertion<br>
>> >> > "assert(Ty<br>
>> >> > ==<br>
>> >> > resolve(Ty.getRef()))" will fire.<br>
>> >> ><br>
>> >> > -------------------------------------------------<br>
>> >> > To fix the problem, we need to remove the direct access to MDNode 473<br>
>> >> > by<br>
>> >> > replacing MDNode 581 from<br>
>> >> > metadata !{metadata !124, metadata !575, metadata !473, metadata<br>
>> >> > !582,<br>
>> >> > metadata !212, metadata !128, metadata !584}<br>
>> >> > to<br>
>> >> > metadata !{metadata !124, metadata !575, metadata<br>
>> >> > !"_ZTS13SpuPacketType",<br>
>> >> > metadata !582, metadata !212, metadata !128, metadata !584}<br>
>> >> ><br>
>> >> > And treat the field {metadata !"_ZTS13SpuPacketType"} as DITypeRef.<br>
>> >> ><br>
>> >> > -------------------------------------------------<br>
>> >> > If we have DIDescriptorRef and all elements currently inside DIArray<br>
>> >> > are<br>
>> >> > DIDescirptors, we can make DIArray an array of DIDescriptorRef.<br>
>> >> > I don't think it is a good idea to add DIDescriptorRef (it makes our<br>
>> >> > types<br>
>> >> > loose) and am not sure about the 2nd condition.<br>
>> >> ><br>
>> >> > So I proposed to add DITypeArray (or DITypedArray<DITypeRef> as David<br>
>> >> > suggested, where all elements are DITypeRef),<br>
>> >> > DICompositeType::getTypeArray() will return DITypeArray and<br>
>> >> > DITypeArray::getElement(unsigned) will return DITypeRef.<br>
>> >> ><br>
>> >> > This is actually more complicated than I thought, not all<br>
>> >> > DICompositeType's<br>
>> >> > getTypeArray() can return an array of DITypeRefs. For example,<br>
>> >> > getTypeArray() of ArrayType and VectorType can not return an array of<br>
>> >> > DITypeRefs.<br>
>> >><br>
>> >> Why can't they?<br>
>> ><br>
>> ><br>
>> > For ArrayType, we create it like this:<br>
>> >   SmallVector<llvm::Value *, 8> Subscripts;<br>
>> > ...<br>
>> > Subscripts.push_back(DBuilder.getOrCreateSubrange(0, Count));<br>
>> > ...<br>
>> >   llvm::DIArray SubscriptArray = DBuilder.getOrCreateArray(Subscripts);<br>
>> ><br>
>> > The elements of getTypeArray() are DISubranges, even though the function<br>
>> > is<br>
>> > called getTypeArray :)<br>
>><br>
>> Yeah, that seems pretty bogus. They could use a separate type with its<br>
>> own array handling, perhaps.<br>
>><br>
>> ><br>
>> >><br>
>> >><br>
>> >> > We can fix that by extending DICompositeType to DISubroutineType and<br>
>> >> > only<br>
>> >> > DISubroutineType::getTypeArray() will return DITypeArray.<br>
>><br>
>> Should it be a composite type at all? (I haven't looked/thought about<br>
>> this much, but figure it's worth asking - since DISubroutineType would<br>
>> still be a DICompositeType and DICompositeType's getTypeArray would<br>
>> still return bogus data, if we did that inheritance - but it's not the<br>
>> worst thing we've got, just still not very nice)<br>
><br>
><br>
> CompositeTypes have TypeArray, RunTimeLang, ContainingType and<br>
> TemplateParams, if SubroutineType uses these,<br>
> extending from CompositeType may be better.<br>
><br>
> I agree that it is weird to have both DICompositeType::getTypeArray()<br>
> returning DIArray and DISubroutineType::getTypeArray() returning<br>
> DITypeArray.<br>
> Maybe change DICompositeType::getTypeArray() to DICompositeType::getArray()?<br>
<br>
</div></div>Yep - if, for most composites, this contains things other than types,<br>
it seems appropriate to call it something else. Maybe "getMembers"<br>
(getArray seems vague, though the array is repurposed for fairly<br>
disparate things in the various composite types, as you've<br>
mentioned... so maybe getArray is OK too).<br>
<div class=""><br>
> If you have a strong preference, let me know,<br>
<br>
</div>No terribly strong preferences, given the vagaries of the data structure.<br>
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> Thanks,<br>
> Manman<br>
><br>
>><br>
>> >> > Even for SubroutineType, elements of the type array can be<br>
>> >> > unspecified<br>
>> >> > parameters which can't be DITypeRefs.<br>
>> >><br>
>> >> Again - I'm just missing why this is the case. DITypeRefs can be<br>
>> >> direct references to types (such as file-internal C++ user defined<br>
>> >> types) so there's always a safe fallback, isn't there?<br>
>> ><br>
>> ><br>
>> > If a SubroutineType's getTypeArray() contains unspecified parameter<br>
>> > (which<br>
>> > is a DIDescriptor, not a DIType), we can't say<br>
>> > DISubroutineType::getTypeArray() will return DITypeArray,<br>
>> > since we assume DITypeArray (or DITypedArray<DITypeRef>) have all<br>
>> > elements<br>
>> > being DITypeRefs.<br>
>><br>
>> Yeah, I agree with you there - we should just build unspecified<br>
>> parameters as some trivial DIType, most likely.<br>
>><br>
>> Thanks for all the work/explanations,<br>
>><br>
>> - David<br>
>><br>
>> ><br>
>> > Thanks,<br>
>> > Manman<br>
>> >><br>
>> >><br>
>> >> > That is why I was thinking about<br>
>> >> > making unspecified parameters trivial DITypes.<br>
>> >> ><br>
>> >> > Thanks a lot,<br>
>> >> > Manman<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> 2) If we're going to fix DIArray apparent type safety (it's not safe<br>
>> >> >> -<br>
>> >> >> just convenient), perhaps we could just template it? (to avoid<br>
>> >> >> churn,<br>
>> >> >> we could leave DIArray as a typedef of DITypedArray<DIDescriptor><br>
>> >> >> for<br>
>> >> >> example, and then have DITypedArray<DITypeRef> which is your<br>
>> >> >> DITypeArray (again, provided via typedef)). It's so small though,<br>
>> >> >> that<br>
>> >> >> I'm not too fussed if we write it out again as you've proposed.<br>
>> >> >><br>
>> >> >> - David<br>
>> >> >><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<br>
>> >> >> >> be<br>
>> >> >> >> equivalent to dA/../dA/B.h)? This will reduce the chance of<br>
>> >> >> >> having<br>
>> >> >> >> two<br>
>> >> >> >> DITypes that should be equivalent with equivalent file names.<br>
>> >> >> >><br>
>> >> >> >> Thanks,<br>
>> >> >> >> Manman<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> ><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>