[PATCH] First set of patches for type uniquing

[Manman Ren]

On Tue, Aug 27, 2013 at 1:04 PM, David Blaikie <dblaikie at gmail.com> wrote:

> 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 attached a patch for updating testing cases earlier:
it basically changed from null to the actual mangled name.
One example:

-// CHECK: [[C:![0-9]*]] = {{.*}} metadata [[C_MEM:![0-9]*]], i32 0,
metadata [[C]], null, null} ; [ DW_TAG_structure_type ] [C] {{.*}} [def]

+// CHECK: [[C:![0-9]*]] = {{.*}} metadata [[C_MEM:![0-9]*]], i32 0,
metadata [[C]], null, metadata !"_ZTS1C"} ; [ DW_TAG_structure_type ] [C]
{{.*}} [def]

By applying the 3 patches (clang, llvm, clang-test), I still see one failure:
CodeGenCXX/debug-info-template.cpp fails with error: cannot yet mangle
expression type CXXUuidoofExpr.

This can be fixed by modifying ItaniumMangler
(CXXNameMangler::mangleExpression), it currently has
FIXME: invent manglings for all these.

Thanks,
Manman


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