[PATCH] Add a DIExternalTypeRef debug metadata node to the IR.

[David Blaikie]

On Mon, May 11, 2015 at 3:04 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015 May 11, at 14:51, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Mon, May 11, 2015 at 2:42 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >
> > > On 2015 May 11, at 09:43, Adrian Prantl <aprantl at apple.com> wrote:
> > >
> > >>
> > >> On May 9, 2015, at 6:55 AM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > >>
> > >>
> > >>> On 2015 May 8, at 17:38, Adrian Prantl <aprantl at apple.com> wrote:
> > >>>
> > >>>>
> > >>>> On May 8, 2015, at 1:00 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > >>>>
> > >>>>
> > >>>>> On 2015 May 8, at 15:32, David Blaikie <dblaikie at gmail.com> wrote:
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On Fri, May 8, 2015 at 11:18 AM, Adrian Prantl <aprantl at apple.com>
> wrote:
> > >>>>> Hi dexonsmith, dblaikie, echristo,
> > >>>>>
> > >>>>> This adds a DIExternalTypeRef debug metadata node to the IR that
> is meant to be emitted by Clang when referring to a type that is defined in
> a .pcm file. DIExternalTypeRef is a child of DICompositeTypeBase because it
> shares common traits such as the unique identifier.
> > >>>>>
> > >>>>> Could this be an attribute to the DIClassType/StructureType? (to
> emit the currently needed type unit references, we still should know the
> correct class/structure type tag) LLVM already knows to hash the ref
> identifier (mangled name) for the type unit identifier.
> > >>>
> > >>> Note that DIExternalTypeRef also knows the tag.
> > >>>>
> > >>>> I think they should be separate for now.  When we have an
> > >>>> `identifier:` field in `DICompositeType` we use the
> > >>>> string-based `DITypeRef`s instead of direct pointers.  I don't
> > >>>> imagine there's any value in adding that indirection for these.
> > >>>
> > >>> It would make lookups slower, but otherwise it would be harmless.
> > >>>
> > >>>> (Although, this makes me wonder about an LTO scenario.  Imagine you
> > >>>> have two translation units, and they both use a type identified by
> > >>>> (e.g.) "my-type".  One of them pulled the type in from a module
> > >>>> and has a `DIExternalTypeRef`, while the other translation unit
> > >>>> didn't use modules and has a `DICompositeType`.  What should the
> > >>>> debug info look like?  Is it okay if we end up with both?  If not,
> > >>>> which one should win?
> > >>>
> > >>> This is a scenario that can only happen in C++. For maximum space
> efficiency you would want the external type ref to win or you will end up
> with two copies of the type (which isn’t otherwise harmful). If you are are
> using dsymutil, it will still unique the two copies.
> > >>>
> > >>>> Will these new nodes show up in the
> > >>>> `retainedType:` list?)
> > >>>
> > >>> They will not show up in the retained types list.
> > >>
> > >> That's a show stopper for sharing with `DICompositeType`, at least
> > >> until we get rid of type refs.
> > >>
> > >> There's logic scattered around the compiler that assumes that if a
> > >> node is `DICompositeType` and it has a valid identifier field, then
> > >> it should be referenced by string.  If something is referenced by
> > >> a string, then it needs to be in the retained type list, or you
> > >> can't convert the string back to a `DICompositeType`.
> > >>
> > >> I think it would be better to keep these classes distinct.  If it
> > >> makes sense to, we can always merge them later (once we no longer
> > >> have type refs).
> > >
> > > There is no hard reason for why the external type references couldn’t
> be added to the list of retained types. We could add code to the DWARF
> backend to not emit the “definitions” of external retained types.
> >
> > Okay, sure, but that seems like a hack.  To me it looks like all the
> > IR-level and backend code that cares that something is a
> > DICompositeType will need special handling based on whether it's an
> > external type ref.
> >
> > So far as I recall very little code cares about DICompositeType
> specifically - it should just be one place, essentially, that fills out the
> attributes/child DIEs of the type DIE. We'd have a special case there that
> would be about as costly/shoehorned as the case for emitting a declaration.
> (it seems like it's just a different kind of declaration - one with a
> DW_AT_signature as well as the DW_AT_declaration and DW_AT_name)
> >
> > Is there some complexity I'm missing here?
>
> Hmm.  I just did a `git grep DICompositeType`, and there's less
> in the backend than I thought.
>
> I guess the only real problem I have is with the type refs.  It
> doesn't make sense to me that we'd add these to the retained
> type list.
>

That's fair, I'm not sure that's particularly costly, is it?

Given the need for backwards compatibility/hardcoded debug info schema, I'm
a bit hesitant to add new constructs we know are a stopgap/insufficient for
the general case we'd like to support in the end.


>
> >
> > It seems like it's just being shoehorned in.
> >
> > @David, if you still feel like they should be the same class, can you
> > explain why?
> >
> > > I’m fine with either variant, with a preference to introducing a
> separate DIExternalTypeRef over having a very special DICompositeType that
> is distinguished by a flag.
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150511/642d25ab/attachment.html>