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 >> > >> > > >
On Mon, Jul 21, 2014 at 3:41 PM, David Blaikie <dblaikie at gmail.com> wrote:> 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? >For ArrayType, we create it like this: SmallVector<llvm::Value *, 8> Subscripts; ... Subscripts.push_back(DBuilder.getOrCreateSubrange(0, Count)); ... llvm::DIArray SubscriptArray = DBuilder.getOrCreateArray(Subscripts); The elements of getTypeArray() are DISubranges, even though the function is called getTypeArray :)> > > 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? >If a SubroutineType's getTypeArray() contains unspecified parameter (which is a DIDescriptor, not a DIType), we can't say DISubroutineType::getTypeArray() will return DITypeArray, since we assume DITypeArray (or DITypedArray<DITypeRef>) have all elements being DITypeRefs. Thanks, Manman> > > 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/48a35527/attachment.html>
On Mon, Jul 21, 2014 at 3:48 PM, Manman Ren <manman.ren at gmail.com> wrote:> > > > On Mon, Jul 21, 2014 at 3:41 PM, David Blaikie <dblaikie at gmail.com> wrote: >> >> 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? > > > For ArrayType, we create it like this: > SmallVector<llvm::Value *, 8> Subscripts; > ... > Subscripts.push_back(DBuilder.getOrCreateSubrange(0, Count)); > ... > llvm::DIArray SubscriptArray = DBuilder.getOrCreateArray(Subscripts); > > The elements of getTypeArray() are DISubranges, even though the function is > called getTypeArray :)Yeah, that seems pretty bogus. They could use a separate type with its own array handling, perhaps.> >> >> >> > We can fix that by extending DICompositeType to DISubroutineType and >> > only >> > DISubroutineType::getTypeArray() will return DITypeArray.Should it be a composite type at all? (I haven't looked/thought about this much, but figure it's worth asking - since DISubroutineType would still be a DICompositeType and DICompositeType's getTypeArray would still return bogus data, if we did that inheritance - but it's not the worst thing we've got, just still not very nice)>> > 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? > > > If a SubroutineType's getTypeArray() contains unspecified parameter (which > is a DIDescriptor, not a DIType), we can't say > DISubroutineType::getTypeArray() will return DITypeArray, > since we assume DITypeArray (or DITypedArray<DITypeRef>) have all elements > being DITypeRefs.Yeah, I agree with you there - we should just build unspecified parameters as some trivial DIType, most likely. Thanks for all the work/explanations, - David> > Thanks, > Manman >> >> >> > 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 >> >> > >> >> > >> > >> > > >