<div dir="ltr">llvm DIBuilder change is in r189410.<div>DIBuilder will use NULL when the identifier is empty.<br><div><br></div><div>Thanks,</div><div>Manman</div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Tue, Aug 27, 2013 at 3:19 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Tue, Aug 27, 2013 at 3:14 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
><br>
>> > I attached a patch for updating testing cases earlier:<br>
>> > it basically changed from null to the actual mangled name.<br>
>> > One example:<br>
>> ><br>
>> > -// CHECK: [[C:![0-9]*]] = {{.*}} metadata [[C_MEM:![0-9]*]], i32 0,<br>
>> > metadata [[C]], null, null} ; [ DW_TAG_structure_type ] [C] {{.*}} [def]<br>
>> ><br>
>> > +// CHECK: [[C:![0-9]*]] = {{.*}} metadata [[C_MEM:![0-9]*]], i32 0,<br>
>> > metadata [[C]], null, metadata !"_ZTS1C"} ; [ DW_TAG_structure_type ]<br>
>> > [C]<br>
>> > {{.*}} [def]<br>
>><br>
>> OK, so you're planning on committing dependent patches - the change to<br>
>> LLVM to /allow/ passing in names will be revision-locked to the change<br>
>> to Clang to pass in (all?) names & update the test cases to reflect<br>
>> them?<br>
><br>
> That was my plan :)<br>
>><br>
>><br>
>> I'm not a huge fan of rev-locked changes like that, but I realize<br>
>> they're necessary in the current architecture (even if we did the<br>
>> extra step I'm describing - where we had the ability to pass in the<br>
>> parameters, but we didn't pass them in - but still changed to<br>
>> zero-length strings rather than null, we'd have to do a batch test<br>
>> update)<br>
>><br>
>> This does make me lean a bit more towards having DIBuilder insert null<br>
>> rather than zero-length strings, then you can commit the DIBuilder<br>
>> change right now without any other changes depending on it & without<br>
>> breaking Clang.<br>
><br>
><br>
> Yeah, it is a nice way to break into small patches. I will do that and<br>
> commit the<br>
> DIBuilder change first.<br>
><br>
>><br>
>><br>
>> Then you can slowly add support for passing names from Clang at each<br>
>> of the "createXXX" DIBuilder calls, one at a time, updating only those<br>
>> test cases it affects while also adding backend functionality<br>
>> incrementally along with the paired DIBuilder changes for the<br>
>> referring fields.<br>
>><br>
>> This means we'll be in an intermediate state for a while where a type<br>
>> is referenced both by real metadata references and via the<br>
>> identifiers, depending on which field used to reference it, until<br>
>> we've migrated all the references over & we will magically start<br>
>> seeing type nodes being shared across Compile Units at that point<br>
>> because we will have broken the cycles.<br>
>><br>
>> ><br>
>> ><br>
>> > By applying the 3 patches (clang, llvm, clang-test), I still see one<br>
>> > failure: CodeGenCXX/debug-info-template.cpp fails with error: cannot yet<br>
>> > mangle expression type CXXUuidoofExpr.<br>
>> ><br>
>> > This can be fixed by modifying ItaniumMangler<br>
>> > (CXXNameMangler::mangleExpression), it currently has<br>
>> > FIXME: invent manglings for all these.<br>
>><br>
>> Presumably we can just avoid trying to mangle these & not provide<br>
>> unique names for them for now. I'm not sure how special such a special<br>
>> case would be.<br>
><br>
><br>
> The mangler does not have an API of saying whether it can be mangled. It<br>
> will<br>
> just throw an error. Are you referring to a helper function in CGDebugInfo<br>
> to check<br>
> whether it can be mangled?<br>
<br>
</div></div>I haven't looked at your patch in detail, but presumably we have logic<br>
to detect whether something's publicly mangleable & only try to mangle<br>
then. So maybe that would catch the UUID case too, if we do it right<br>
(or maybe not, I don't know).<br>
<br>
In addition/alternatively, I wonder why that test case wasn't already<br>
failing to mangle the UUID, I would assume the mangled name of the<br>
non-type template parameters is needed for the template to be<br>
compiled. So what does it do with the UUID, I wonder...<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> Thanks,<br>
> Manman<br>
><br>
>><br>
>><br>
>> ><br>
>> > Thanks,<br>
>> > Manman<br>
>> ><br>
>> >><br>
>> >> ><br>
>> >> >><br>
>> >> >> I'm always on the fence about the issues around null versus empty in<br>
>> >> >> strings that can't represent both (such as StringRef), but not<br>
>> >> >> having<br>
>> >> >> to update the test cases again might be sufficient motivation to use<br>
>> >> >> null instead of "" for these cases (adding a conditional operator,<br>
>> >> >> or<br>
>> >> >> a trivial function, to produce a null value when the string is empty<br>
>> >> >> &<br>
>> >> >> otherwise use MDString::get).<br>
>> >> >><br>
>> >> >> With test case updates or the change to use null values for empty<br>
>> >> >> strings, please go ahead and commit this patch.<br>
>> >> >><br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > Attaching patches to implement the above item:<br>
>> >> >> > One patch on llvm side to update DIBuilder interface:<br>
>> >> >> > createClassType, createStructType, createUnionType,<br>
>> >> >> > createEnumerationType,<br>
>> >> >> > and createForwardDecl will take an optional StringRef for clang to<br>
>> >> >> > pass<br>
>> >> >> > in<br>
>> >> >> > the unique<br>
>> >> >> > identifier.<br>
>> >> >> ><br>
>> >> >> > One patch on clang side to actually generate the unique identifier<br>
>> >> >> > and<br>
>> >> >> > pass<br>
>> >> >> > it<br>
>> >> >> > to DIBuilder:<br>
>> >> >> > 1> add getUniqueTagTypeName to use CXX mangler to generate a<br>
>> >> >> > unique<br>
>> >> >> > identifier for<br>
>> >> >> > external CXX TagType.<br>
>> >> >> > 2> call getUniqueTagTypeName and add the type to RetainedTypes in<br>
>> >> >> > a<br>
>> >> >> > few<br>
>> >> >> > places, before<br>
>> >> >> > creating a ClassType, StructType, UnionType, EnumerationType, or<br>
>> >> >> > ForwardDecl.<br>
>> >> >><br>
>> >> >> Could we move type retention into DIBuilder so it could manage<br>
>> >> >> adding<br>
>> >> >> any type with a unique identifier to it automatically? (or at least<br>
>> >> >> allow the list of retained types handled by Clang to be appended to<br>
>> >> >> a<br>
>> >> >> list that DIBuilder mantains?)<br>
>> >> ><br>
>> >> ><br>
>> >> > That is another possibility, but we have to update<br>
>> >> > DIBuilder::AllRetainTypes<br>
>> >> > to be VH instead of<br>
>> >> > plain Value *, since the types created can later on be modified. The<br>
>> >> > patch<br>
>> >> > updates CGDebugInfo::RetainedTypes and<br>
>> >> > uses TypeCache to find the corresponding DIType in finalize(), there<br>
>> >> > it<br>
>> >> > calls DIBuilder.retainType to add to DIBuilder.AllRetainTypes.<br>
>> >><br>
>> >> That sounds reasonable. A test case in Clang (since that's essentially<br>
>> >> how DIBuilder is tested, unfortunately) that would fail if this was<br>
>> >> implemented the way I suggested (using retainType directly from within<br>
>> >> createStructureType, etc, whenever the Unique Identifier is provided -<br>
>> >> but without making the Value*/VH change you mentioned is needed).<br>
>> >><br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> > 3> Input for getOrCreateRecordFwdDecl is modified from RecordDecl<br>
>> >> >> > to<br>
>> >> >> > RecordType to enable<br>
>> >> >> > CXX mangler, same applies for CreateEnumType.<br>
>> >> >><br>
>> >> >> It's easy to get a RecordType from a RecordDecl (getTypeForDecl) &<br>
>> >> >> only slightly harder to go the other way - but if this actually<br>
>> >> >> simplifies code I'm OK with it, but would probably want this patch<br>
>> >> >> pulled out separately for ease of review.<br>
>> >> ><br>
>> >> > Sure, I can pull out the part separately.<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> > 4> It is possible that the same Type is pushed to RetainedTypes<br>
>> >> >> > multiple<br>
>> >> >> > times, so a SmallPtrSet<br>
>> >> >> > is used locally in finalize() to remove duplication.<br>
>> >> >><br>
>> >> >> When does this happen? That sounds like it might be a bug in Clang,<br>
>> >> >> or<br>
>> >> >> at least something we could tidy up there to make this less<br>
>> >> >> complicated.<br>
>> >> ><br>
>> >> > I will look deeper at this and figure out why.<br>
>> >> ><br>
>> >> > Thanks for the quick review.<br>
>> >> ><br>
>> >> > Manman<br>
>> >> >><br>
>> >> >><br>
>> >> >> ><br>
>> >> >> > The 3rd patch to update testing cases on clang side:<br>
>> >> >> ><br>
>> >> >> > There is one remaining issue with the current implementation of<br>
>> >> >> > using<br>
>> >> >> > CXX<br>
>> >> >> > mangler<br>
>> >> >> > to generate the unique identifier:<br>
>> >> >> > CodeGenCXX/debug-info-template.cpp<br>
>> >> >> > fails<br>
>> >> >> > with error:<br>
>> >> >> > cannot yet mangle expression type CXXUuidoofExpr.<br>
>> >> >><br>
>> >> >> > We can definitely fix the above error in ItaniumMangle.cpp, but I<br>
>> >> >> > want<br>
>> >> >> > to<br>
>> >> >> > see whether the approach I am<br>
>> >> >> > taking is on the right track.<br>
>> >> >> ><br>
>> >> >> > Thanks,<br>
>> >> >> > Manman<br>
>> >> ><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>