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

[David Blaikie]

On Mon, May 11, 2015 at 3:25 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On May 11, 2015, at 3:14 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, May 11, 2015 at 3:09 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> > On 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.
>>
>> The fwddecl-style references (with full DeclContext + Name) that David
>> referred to actually make some sense in the retained types list.
>
>
> Not sure I follow how one (fwddecl style references) relate to the other
> (retained types list)?
>
>
> There is really nothing we could emit for an external type ref that is
> only a signature or UID.
>

Still missing some steps in this thread. Confused.

If all we have is a signature, then we emit DW_AT_type + DW_FORM_ref_sig8


>
>
>
>> As I recently noted in the thread about DIModule there are good reasons
>> to emit them even on Darwin (they tell us which module a type originated
>> from)
>
>
> Don't follow this either - what does the type reference style (DW_TAG_type
> + DW_FORM_ref_sig8 compared to DW_TAG_type as a reference to a declaration
> type DIE) relate to module origin and dsymutil's behavior?
>
>
> I was thinking more about DW_AT_type + DW_FORM_ref_sig8 (not sure if that
> was a typo)
>

Not a typo - that's one alternative way to reference a type in a type unit
if we don't emit a full declaration. (but it presents problems when we want
to emit the out-of-line definition of a member function that needs to
reference the declaration which we currently put inside the declaration -
GCC Produces 3 different ways of referencing type units, I just implemented
the most general (the one that allows member function declarations))


> versus a DW_AT_TYPE + ref to fwd decl that is a child of the DW_TAG_module
> or the skeleton CU.
>

I'd rather not reference things in the module or skeleton CU - I'm hoping
to keep this usable by a DWARF 5 debugger supporting fission and type units
rather than adding more custom/experimental features.

For anything outside fission+type units, I really want to talk about that
sort of thing in detail to understand the motivation, direction, benefits,
tradeoffs, alternatives, etc. That's why I like piggybacking on the
existing features - it requires far less scrutiny/design as the features
are already formalized/implemented/used in practice, etc.


> In the first case the consumer cannot know in which module to look for the
> type (other than by searching through all modules imported by the CU). In
> the second case it could use the decl context of the fwd decl (assuming
> that it would be the root of the DeclContext some way or another) to figure
> out the module. It also could load the type directly from the module using
> the info from the decl context.
>

Using the fully qualified name built from the context/name? If that suits
you guys, though I think we originally discussed having a type unit
hash->module info type identifier table somewhere, but that's for the other
review threads I think... which I'll eventually muster the courage to get
back to. Sorry for the delay.

- David


>
> -- adrian
>
>
>
>> and let dsymutil optimize them away.
>>
>> -- adrian
>>
>> >
>> >>
>> >> 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/e5a4508f/attachment.html>