<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> 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 ] [C]<br>
> {{.*}} [def]<br>
<br>
</div></div>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></blockquote><div>That was my plan :) </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote><div><br></div><div>Yeah, it is a nice way to break into small patches. I will do that and commit the</div><div>DIBuilder change first.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<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>
<div class="im"><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>
</div>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></blockquote><div><br></div><div>The mangler does not have an API of saying whether it can be mangled. It will</div><div>just throw an error. Are you referring to a helper function in CGDebugInfo to check</div>
<div>whether it can be mangled?</div><div><br></div><div>Thanks,</div><div>Manman</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><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 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, or<br>
>> >> a trivial function, to produce a null value when the string is empty &<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 unique<br>
>> >> > identifier for<br>
>> >> > external CXX TagType.<br>
>> >> > 2> call getUniqueTagTypeName and add the type to RetainedTypes in 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 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 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 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 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, 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 using<br>
>> >> > CXX<br>
>> >> > mangler<br>
>> >> > to generate the unique identifier: 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>
</div></div></blockquote></div><br></div></div>