On Mon, Jul 21, 2014 at 10:39 AM, Manman Ren <manman.ren at gmail.com> wrote:> > > > On Mon, Jul 14, 2014 at 11:32 AM, Manman Ren <manman.ren at gmail.com> wrote: >> >> >> We still have access to types via MDNodes directly and the assertion that >> assumes all accesses to DITypes are accessing the resolved DIType will fire >> >> i.e assert(Ty == resolve(Ty.getRef())) >> >> One example is the access to DIType via DIArray in SubroutineType. If all >> elements in the type array are DITypes we can create a DITypeArray and use >> that for SubroutineType's type array instead. But we currently have >> unspecified parameter in the type array and it is not a DIType. > > > I am going to work on a patch that adds DITypeArray (each element will be > DITypeRef, SubroutineType's type array will be DITypeArray) and adds > DITrivialType that extends from DIType (unspecified parameter will be > DITrivialType). > If you have opinions against it, please let me know,We haven't bothered using typed arrays in DebugInfo yet (as you say, we just have DIArray) so I have two thoughts 1) why does this one case need fixing/changing? Is it because we have things that aren't DIDescriptors inside the DIArray? (the strings that refer to types). Given how loosely typed DIDescriptor is (it doesn't check that it's a valid DIDescriptor) I assume this doesn't actually cause a problem, though it's certainly not nice. So we could just leave it as-is, pass DIArray's element to "resolve" (it'd implicitly convert the DIDescriptor back to a raw MDNode*), then perhaps we'd need to make DITypeRef's ctor public so it could be used here. Not suggesting that's ideal, though. 2) If we're going to fix DIArray apparent type safety (it's not safe - just convenient), perhaps we could just template it? (to avoid churn, we could leave DIArray as a typedef of DITypedArray<DIDescriptor> for example, and then have DITypedArray<DITypeRef> which is your DITypeArray (again, provided via typedef)). It's so small though, that I'm not too fussed if we write it out again as you've proposed. - David> > Thanks, > Manman > >> >> What are your thoughts? Suggestions are welcome. >> >> Is it a good idea to canonicalize file names (i.e dA/B.h should be >> equivalent to dA/../dA/B.h)? This will reduce the chance of having two >> DITypes that should be equivalent with equivalent file names. >> >> Thanks, >> Manman > >
On Mon, Jul 21, 2014 at 1:14 PM, David Blaikie <dblaikie at gmail.com> wrote:> On Mon, Jul 21, 2014 at 10:39 AM, Manman Ren <manman.ren at gmail.com> wrote: > > > > > > > > On Mon, Jul 14, 2014 at 11:32 AM, Manman Ren <manman.ren at gmail.com> > wrote: > >> > >> > >> We still have access to types via MDNodes directly and the assertion > that > >> assumes all accesses to DITypes are accessing the resolved DIType will > fire > >> > >> i.e assert(Ty == resolve(Ty.getRef())) > >> > >> One example is the access to DIType via DIArray in SubroutineType. If > all > >> elements in the type array are DITypes we can create a DITypeArray and > use > >> that for SubroutineType's type array instead. But we currently have > >> unspecified parameter in the type array and it is not a DIType. > > > > > > I am going to work on a patch that adds DITypeArray (each element will be > > DITypeRef, SubroutineType's type array will be DITypeArray) and adds > > DITrivialType that extends from DIType (unspecified parameter will be > > DITrivialType). > > If you have opinions against it, please let me know, > > We haven't bothered using typed arrays in DebugInfo yet (as you say, > we just have DIArray) so I have two thoughts > > 1) why does this one case need fixing/changing? Is it because we have > things that aren't DIDescriptors inside the DIArray? (the strings that > refer to types). Given how loosely typed DIDescriptor is (it doesn't > check that it's a valid DIDescriptor) I assume this doesn't actually > cause a problem, though it's certainly not nice. So we could just > leave it as-is, pass DIArray's element to "resolve" (it'd implicitly > convert the DIDescriptor back to a raw MDNode*), then perhaps we'd > need to make DITypeRef's ctor public so it could be used here. Not > suggesting that's ideal, though. >I should have provided an example to help understanding the issue :) When processing the following type node, we throw an assertion failure assert(Ty == resolve(Ty.getRef())) !{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 ] There are two type nodes with the same identifier: !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 ] !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 ] The only difference between these two is the file node !474 = metadata !{metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71/SPU/SPUPacket.h", metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71"} !6696 = metadata !{metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71/SPU/../SPU/SPUPacket.h", metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71"} We have direct access to 473 via 580's type array. !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 ] !581 = metadata !{metadata !124, metadata !575, metadata !473, metadata !582, metadata !212, metadata !128, metadata !584} MDNode 473 will be resolved to MDNode 6695 and the assertion "assert(Ty =resolve(Ty.getRef()))" will fire. ------------------------------------------------- To fix the problem, we need to remove the direct access to MDNode 473 by replacing MDNode 581 from metadata !{metadata !124, metadata !575, metadata !473, metadata !582, metadata !212, metadata !128, metadata !584} to metadata !{metadata !124, metadata !575, metadata !"_ZTS13SpuPacketType", metadata !582, metadata !212, metadata !128, metadata !584} And treat the field {metadata !"_ZTS13SpuPacketType"} as DITypeRef. ------------------------------------------------- If we have DIDescriptorRef and all elements currently inside DIArray are DIDescirptors, we can make DIArray an array of DIDescriptorRef. 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. 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. 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. We can fix that by extending DICompositeType to DISubroutineType and only DISubroutineType::getTypeArray() will return DITypeArray. 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. Thanks a lot, Manman> > 2) If we're going to fix DIArray apparent type safety (it's not safe - > just convenient), perhaps we could just template it? (to avoid churn, > we could leave DIArray as a typedef of DITypedArray<DIDescriptor> for > example, and then have DITypedArray<DITypeRef> which is your > DITypeArray (again, provided via typedef)). It's so small though, that > I'm not too fussed if we write it out again as you've proposed. > > - David > > > > > Thanks, > > Manman > > > >> > >> What are your thoughts? Suggestions are welcome. > >> > >> Is it a good idea to canonicalize file names (i.e dA/B.h should be > >> equivalent to dA/../dA/B.h)? This will reduce the chance of having two > >> DITypes that should be equivalent with equivalent file names. > >> > >> Thanks, > >> Manman > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140721/4ed67fa6/attachment.html>
On Mon, Jul 21, 2014 at 3:35 PM, Manman Ren <manman.ren at gmail.com> wrote:> > > > On Mon, Jul 21, 2014 at 1:14 PM, David Blaikie <dblaikie at gmail.com> wrote: >> >> On Mon, Jul 21, 2014 at 10:39 AM, Manman Ren <manman.ren at gmail.com> wrote: >> > >> > >> > >> > On Mon, Jul 14, 2014 at 11:32 AM, Manman Ren <manman.ren at gmail.com> >> > wrote: >> >> >> >> >> >> We still have access to types via MDNodes directly and the assertion >> >> that >> >> assumes all accesses to DITypes are accessing the resolved DIType will >> >> fire >> >> >> >> i.e assert(Ty == resolve(Ty.getRef())) >> >> >> >> One example is the access to DIType via DIArray in SubroutineType. If >> >> all >> >> elements in the type array are DITypes we can create a DITypeArray and >> >> use >> >> that for SubroutineType's type array instead. But we currently have >> >> unspecified parameter in the type array and it is not a DIType. >> > >> > >> > I am going to work on a patch that adds DITypeArray (each element will >> > be >> > DITypeRef, SubroutineType's type array will be DITypeArray) and adds >> > DITrivialType that extends from DIType (unspecified parameter will be >> > DITrivialType). >> > If you have opinions against it, please let me know, >> >> We haven't bothered using typed arrays in DebugInfo yet (as you say, >> we just have DIArray) so I have two thoughts >> >> 1) why does this one case need fixing/changing? Is it because we have >> things that aren't DIDescriptors inside the DIArray? (the strings that >> refer to types). Given how loosely typed DIDescriptor is (it doesn't >> check that it's a valid DIDescriptor) I assume this doesn't actually >> cause a problem, though it's certainly not nice. So we could just >> leave it as-is, pass DIArray's element to "resolve" (it'd implicitly >> convert the DIDescriptor back to a raw MDNode*), then perhaps we'd >> need to make DITypeRef's ctor public so it could be used here. Not >> suggesting that's ideal, though. > > > I should have provided an example to help understanding the issue :) > > When processing the following type node, we throw an assertion failure > assert(Ty == resolve(Ty.getRef())) > !{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 ] > > There are two type nodes with the same identifier: > !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 ] > !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 ] > > The only difference between these two is the file node > !474 = metadata !{metadata > !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71/SPU/SPUPacket.h", > metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71"} > !6696 = metadata !{metadata > !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71/SPU/../SPU/SPUPacket.h", > metadata !"/Users/manmanren/swb/rdar_17628609/AppleSPUFirmware-71"} > > We have direct access to 473 via 580's type array. > !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 ] > !581 = metadata !{metadata !124, metadata !575, metadata !473, metadata > !582, metadata !212, metadata !128, metadata !584} > > MDNode 473 will be resolved to MDNode 6695 and the assertion "assert(Ty => resolve(Ty.getRef()))" will fire. > > ------------------------------------------------- > To fix the problem, we need to remove the direct access to MDNode 473 by > replacing MDNode 581 from > metadata !{metadata !124, metadata !575, metadata !473, metadata !582, > metadata !212, metadata !128, metadata !584} > to > metadata !{metadata !124, metadata !575, metadata !"_ZTS13SpuPacketType", > metadata !582, metadata !212, metadata !128, metadata !584} > > And treat the field {metadata !"_ZTS13SpuPacketType"} as DITypeRef. > > ------------------------------------------------- > If we have DIDescriptorRef and all elements currently inside DIArray are > DIDescirptors, we can make DIArray an array of DIDescriptorRef. > 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. > > 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. > > 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.Why can't they?> We can fix that by extending DICompositeType to DISubroutineType and only > DISubroutineType::getTypeArray() will return DITypeArray. > Even for SubroutineType, elements of the type array can be unspecified > parameters which can't be DITypeRefs.Again - I'm just missing why this is the case. DITypeRefs can be direct references to types (such as file-internal C++ user defined types) so there's always a safe fallback, isn't there?> That is why I was thinking about > making unspecified parameters trivial DITypes. > > Thanks a lot, > Manman > >> >> >> 2) If we're going to fix DIArray apparent type safety (it's not safe - >> just convenient), perhaps we could just template it? (to avoid churn, >> we could leave DIArray as a typedef of DITypedArray<DIDescriptor> for >> example, and then have DITypedArray<DITypeRef> which is your >> DITypeArray (again, provided via typedef)). It's so small though, that >> I'm not too fussed if we write it out again as you've proposed. >> >> - David >> >> > >> > Thanks, >> > Manman >> > >> >> >> >> What are your thoughts? Suggestions are welcome. >> >> >> >> Is it a good idea to canonicalize file names (i.e dA/B.h should be >> >> equivalent to dA/../dA/B.h)? This will reduce the chance of having two >> >> DITypes that should be equivalent with equivalent file names. >> >> >> >> Thanks, >> >> Manman >> > >> > > >