[PATCH] First set of patches for type uniquing

[Manman Ren]

> > 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]
>
> OK, so you're planning on committing dependent patches - the change to
> LLVM to /allow/ passing in names will be revision-locked to the change
> to Clang to pass in (all?) names & update the test cases to reflect
> them?
>
That was my plan :)

>
> I'm not a huge fan of rev-locked changes like that, but I realize
> they're necessary in the current architecture (even if we did the
> extra step I'm describing - where we had the ability to pass in the
> parameters, but we didn't pass them in - but still changed to
> zero-length strings rather than null, we'd have to do a batch test
> update)
>
> This does make me lean a bit more towards having DIBuilder insert null
> rather than zero-length strings, then you can commit the DIBuilder
> change right now without any other changes depending on it & without
> breaking Clang.
>

Yeah, it is a nice way to break into small patches. I will do that and
commit the
DIBuilder change first.


>
> Then you can slowly add support for passing names from Clang at each
> of the "createXXX" DIBuilder calls, one at a time, updating only those
> test cases it affects while also adding backend functionality
> incrementally along with the paired DIBuilder changes for the
> referring fields.
>
> This means we'll be in an intermediate state for a while where a type
> is referenced both by real metadata references and via the
> identifiers, depending on which field used to reference it, until
> we've migrated all the references over & we will magically start
> seeing type nodes being shared across Compile Units at that point
> because we will have broken the cycles.
>
> >
> >
> > 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.
>
> Presumably we can just avoid trying to mangle these & not provide
> unique names for them for now. I'm not sure how special such a special
> case would be.
>

The mangler does not have an API of saying whether it can be mangled. It
will
just throw an error. Are you referring to a helper function in CGDebugInfo
to check
whether it can be mangled?

Thanks,
Manman


>
> >
> > 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/d951bec1/attachment.html>