[PATCH] First set of patches for type uniquing

[Manman Ren]

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.


> 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.


>
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130827/0096e81e/attachment.html>