[LLVMdev] LTO type uniquing: ODR assertion failure

[David Blaikie]

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