[LLVMdev] LTO type uniquing: ODR assertion failure

[Manman Ren]

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

> 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


>
> 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)
> >
> >>> > 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/20140721/d078aed5/attachment.html>