LTO type uniquing: ODR assertion failure

Project Infrastructure LLVM Dev List Archives
#1

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.

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

#2

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,

Thanks,
Manman

#3

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,

I'm still not sure what problem you're trying to solve here. Can you
be more clear?

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.

Probably.

-eric

#4

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

#5

I should have provided an example to help understanding the issue :slight_smile:

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.

#6

>
>
>
>>
>>
>> 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 :slight_smile:

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?

#7

>
>
>
>>
>> >
>> >
>> >
>> >>
>> >>
>> >> 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 :slight_smile:
>
> 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 :slight_smile:

> 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

#8

>
>
>
>>
>> >
>> >
>> >
>> >>
>> >>
>> >> 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 :slight_smile:
>
> 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 :slight_smile:

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

#9

Oh, and you can probably do some of those parts separately - changing
unspecified parameters to a DIType should be a standalone refactoring.

As for ArrayType/VectorType/SubroutineType - rather than specializing
SubroutineType, it sounds like ArrayType/VectorType are the
odd-ones-out, having their composite type arrays not really be types.
Perhaps they should be the ones that end up as their own (possibly not
even composite? but I'm not sure) types. (again, whichever way that
goes, it could be a separate refactoring patch)

#10

Oh, and you can probably do some of those parts separately - changing
unspecified parameters to a DIType should be a standalone refactoring.

Yes.

As for ArrayType/VectorType/SubroutineType - rather than specializing
SubroutineType, it sounds like ArrayType/VectorType are the
odd-ones-out, having their composite type arrays not really be types.

Class/Enum/Struct/Union store DIObjcProperty and DISubprogram in the type
array. DIObjecProperty and DISubprogram are not DITypes.
So out of the 7 possible CompositeTypes, only SubroutineType can have all
elements of the array being DITypes.

So separating DISubroutineType should be better (easier).

Perhaps they should be the ones that end up as their own (possibly not
even composite? but I'm not sure) types. (again, whichever way that
goes, it could be a separate refactoring patch)

Yes, I will do it incrementally as separate patches.

Thanks,
Manman

#11

>
>
>
>>
>> >
>> >
>> >
>> >>
>> >> >
>> >> >
>> >> >
>> >> >>
>> >> >>
>> >> >> 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 :slight_smile:
>> >
>> > 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 :slight_smile:

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)

CompositeTypes have TypeArray, RunTimeLang, ContainingType and
TemplateParams, if SubroutineType uses these,
extending from CompositeType may be better.

I agree that it is weird to have both DICompositeType::getTypeArray()
returning DIArray and DISubroutineType::getTypeArray() returning
DITypeArray.
Maybe change DICompositeType::getTypeArray() to DICompositeType::getArray()?
If you have a strong preference, let me know,

Thanks,
Manman

#12

>
>
>
>>
>> >
>> >
>> >
>> >>
>> >> >
>> >> >
>> >> >
>> >> >>
>> >> >>
>> >> >> 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 :slight_smile:
>> >
>> > 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 :slight_smile:

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)

CompositeTypes have TypeArray, RunTimeLang, ContainingType and
TemplateParams, if SubroutineType uses these,
extending from CompositeType may be better.

I agree that it is weird to have both DICompositeType::getTypeArray()
returning DIArray and DISubroutineType::getTypeArray() returning
DITypeArray.
Maybe change DICompositeType::getTypeArray() to DICompositeType::getArray()?

Yep - if, for most composites, this contains things other than types,
it seems appropriate to call it something else. Maybe "getMembers"
(getArray seems vague, though the array is repurposed for fairly
disparate things in the various composite types, as you've
mentioned... so maybe getArray is OK too).

If you have a strong preference, let me know,

No terribly strong preferences, given the vagaries of the data structure.

- David

#13

The first patch is in r214111.

Thanks,
Manman