[PATCH] First set of patches for type uniquing

[David Blaikie]

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

I haven't looked at your patch in detail, but presumably we have logic
to detect whether something's publicly mangleable & only try to mangle
then. So maybe that would catch the UUID case too, if we do it right
(or maybe not, I don't know).

In addition/alternatively, I wonder why that test case wasn't already
failing to mangle the UUID, I would assume the mangled name of the
non-type template parameters is needed for the template to be
compiled. So what does it do with the UUID, I wonder...

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