[PATCH] First set of patches for type uniquing

[David Blaikie]

On Tue, Aug 27, 2013 at 11:23 AM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
>
> On Mon, Aug 26, 2013 at 4:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Mon, Aug 26, 2013 at 4:18 PM, Manman Ren <manman.ren at gmail.com> wrote:
>> >
>> >
>> >
>> > On Mon, Aug 26, 2013 at 4:00 PM, David Blaikie <dblaikie at gmail.com>
>> > wrote:
>> >>
>> >> On Mon, Aug 26, 2013 at 3:46 PM, Manman Ren <manman.ren at gmail.com>
>> >> wrote:
>> >> > In r189282 and r189283.
>> >> >
>> >> > I will work on clang front-end to generate the unique identifier for
>> >> > DICompositeType.
>> >>
>> >> For each field that uses a DIType that may refer to a DICompositeType,
>> >> we'll see two patches - one for the backend support (to do the
>> >> necessary indirect access through a map lookup) and frontend support
>> >> to emit a name.
>> >
>> >
>> > By frontend support, are you referring to the change to DIBuilder?
>>
>> I was thinking of the actual bit where Clang passes in an identifier,
>> but yes - that'll be fairly mechanical (lots of test cases to update)
>> & a no-op until DIBuilder actually propagates that to type references.
>>
>> > DIBuilder
>> > will
>> > inspect the DIType reference, and if it is a DICompositeType and the
>> > unique
>> > identifier is non-null, DIBuilder will use the identifier instead of the
>> > MDNode itself.
>> >
>> > Per our discussion via IRC, I thought clang will first generate unique
>> > identifier for
>> > external C++ DICompositeTypes. The next step is to update the usage of a
>> > DIType
>> > that may refer to a DICompositeType (with one use as a start).
>>
>> You're right, this could happen in either order & either will be
>> testable but have no ultimate effect until both frontend and backend
>> have been modified.
>>
>> So I suppose to get my "one simple case" working, we'd be talking
>> about having DIBuilder do the "is this a DICompositeType, if so use
>> the identifier instead of the MDNode" check on exactly one field &
>> implement the backend support for that too.
>>
>> >
>> >>
>> >> The backend patch can be committed prior to the
>> >> frontend patch (though the frontend patch would be a nice way to
>> >> generate the backend test to ensure it's what we intend to
>> >> support/works end-to-end).
>> >>
>> >> It'd be good to see exactly one (a small one, where possible) use of
>> >> this first so we can checkin the map creation & lookup plumbing with a
>> >> single simple field to motivate/demonstrate/test it, then
>> >> incrementally commit support for each field after that.
>> >
>> > Agreed.
>>
>> OK, so:
>>
>> 1) add identifier from Clang (DIBuilder support, plus passing in the
>> identifier from Clang), updating lots of Clang test cases

I assume you're going to have to update a lot of test cases if you
make the change as you've written it - as we'll start producing empty
strings ("") for the identifier field instead of null. Is that
correct?

I'm always on the fence about the issues around null versus empty in
strings that can't represent both (such as StringRef), but not having
to update the test cases again might be sufficient motivation to use
null instead of "" for these cases (adding a conditional operator, or
a trivial function, to produce a null value when the string is empty &
otherwise use MDString::get).

With test case updates or the change to use null values for empty
strings, please go ahead and commit this patch.

>
>
> Attaching patches to implement the above item:
> One patch on llvm side to update DIBuilder interface:
> createClassType, createStructType, createUnionType, createEnumerationType,
> and createForwardDecl will take an optional StringRef for clang to pass in
> the unique
> identifier.
>
> One patch on clang side to actually generate the unique identifier and pass
> it
> to DIBuilder:
> 1> add getUniqueTagTypeName to use CXX mangler to generate a unique
> identifier for
> external CXX TagType.
> 2> call getUniqueTagTypeName and add the type to RetainedTypes in a few
> places, before
> creating a ClassType, StructType, UnionType, EnumerationType, or
> ForwardDecl.

Could we move type retention into DIBuilder so it could manage adding
any type with a unique identifier to it automatically? (or at least
allow the list of retained types handled by Clang to be appended to a
list that DIBuilder mantains?)

> 3> Input for getOrCreateRecordFwdDecl is modified from RecordDecl to
> RecordType to enable
> CXX mangler, same applies for CreateEnumType.

It's easy to get a RecordType from a RecordDecl (getTypeForDecl) &
only slightly harder to go the other way - but if this actually
simplifies code I'm OK with it, but would probably want this patch
pulled out separately for ease of review.

> 4> It is possible that the same Type is pushed to RetainedTypes multiple
> times, so a SmallPtrSet
> is used locally in finalize() to remove duplication.

When does this happen? That sounds like it might be a bug in Clang, or
at least something we could tidy up there to make this less
complicated.

>
> The 3rd patch to update testing cases on clang side:
>
> There is one remaining issue with the current implementation of using CXX
> mangler
> to generate the unique identifier: CodeGenCXX/debug-info-template.cpp fails
> with error:
> cannot yet mangle expression type CXXUuidoofExpr.
> We can definitely fix the above error in ItaniumMangle.cpp, but I want to
> see whether the approach I am
> taking is on the right track.
>
> Thanks,
> Manman
>
>
>>
>> 2) add LLVM support for mapping one particular type field, if it's
>> string instead of an MDNode, add explicit test cases, possibly
>> generated by Clang with (2.1)
>> 2.1) (can be separate from (2), probably should be for reviewability -
>> that does leave (2) semi-untested - a manual test case or test case
>> generated with (2.1) but committed with (2) might be OK) change
>> DIBuilder to do the string-instead-of-mdnode thing for that one field.
>>
>> (2.1 can't come before 2 or Clang would be broken in the interim -
>> generating identifier references when LLVM was incapable of doing the
>> mapping)
>>
>> Repeat 2 and 2.1 for one field at a time until done.
>
>