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

[Eric Christopher]

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

> On May 11, 2015, at 3:32 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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
>
>
> I was thinking in the context of the loop that emits the standalone
> versions of all retained types:
>     for (auto *Ty : CUNode->getRetainedTypes())
>       CU.getOrCreateTypeDIE(cast<DIType>(resolve(Ty->getRef())));
>
> (Not that it would be hard to ignore signature/UID-only types here.)
>
>
>
>>
>>
>>
>>> 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.
>
>
> That would be something we’d only do on systems that don’t support type
> units. (i.e. Darwin: partially because Mach-O doesn’t do comdat sections,
> partially because other dwarf consumers like Instruments and
>

Just as a "For the Record", you don't need comdat to implement type units.
You do need things to understand them, but you don't need comdat to
implement them. :)

-eric


> dtrace also don’t support them.) Which is not to say that it will always
> be different. It would be straightforward to have dsymutil take
> bag-of-dwarf-style debug info and produce compact output that is easy for
> these tools to digest.
>
>
> 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.
>
>
> That’s fair.
>
>
>
>> 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.
>
>
> thanks,
> adrian
>
>
> - 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/20150521/0b57c483/attachment.html>