[LLVMdev] LTO type uniquing: ODR assertion failure

[Manman Ren]

The first patch is in r214111.

Thanks,
Manman


On Mon, Jul 21, 2014 at 4:59 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Mon, Jul 21, 2014 at 4:55 PM, Manman Ren <manman.ren at gmail.com> wrote:
> >
> >
> >
> > On Mon, Jul 21, 2014 at 3:54 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>
> >> 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)
> >
> >
> > 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
>
> >
> > Thanks,
> > Manman
> >
> >>
> >> >> > 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
> >> >> >> >
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140728/e7b89d7a/attachment.html>