[PATCH] First set of patches for type uniquing

[David Blaikie]

On Tue, Aug 27, 2013 at 12:57 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
> On Tue, Aug 27, 2013 at 12:41 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Tue, Aug 27, 2013 at 11:23 AM, Manman Ren <manman.ren at gmail.com> wrote:
>>
>> >> 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?
>
> The verifier currently accepts both null and a StringRef, so there is no
> verification error with the existing
> llvm testing cases. Yes, we are now producing "" instead of null. To make
> sure the existing llvm testing
> cases follow what DIBuilder emits, we can update the testing cases or use a
> null value when the
> string is empty as suggested below.

It's the Clang test cases I'm concerned about - if you just commit
this change, the Clang build will break, won't it?

>
>>
>> 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?)
>
>
> That is another possibility, but we have to update DIBuilder::AllRetainTypes
> to be VH instead of
> plain Value *, since the types created can later on be modified. The patch
> updates CGDebugInfo::RetainedTypes and
> uses TypeCache to find the corresponding DIType in finalize(), there it
> calls DIBuilder.retainType to add to DIBuilder.AllRetainTypes.

That sounds reasonable. A test case in Clang (since that's essentially
how DIBuilder is tested, unfortunately) that would fail if this was
implemented the way I suggested (using retainType directly from within
createStructureType, etc, whenever the Unique Identifier is provided -
but without making the Value*/VH change you mentioned is needed).

>
>>
>>
>> > 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.
>
> Sure, I can pull out the part separately.
>
>>
>>
>> > 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.
>
> I will look deeper at this and figure out why.
>
> Thanks for the quick review.
>
> Manman
>>
>>
>> >
>> > 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
>
>