[PATCH] First set of patches for type uniquing

[David Blaikie]

On Tue, Aug 27, 2013 at 1:12 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>
>
> 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]

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?

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.

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.

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