<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 27, 2013 at 1:04 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On Tue, Aug 27, 2013 at 12:57 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>

><br>
><br>
><br>
> On Tue, Aug 27, 2013 at 12:41 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Tue, Aug 27, 2013 at 11:23 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
>><br>
>> >> 1) add identifier from Clang (DIBuilder support, plus passing in the<br>
>> >> identifier from Clang), updating lots of Clang test cases<br>
>><br>
>> I assume you're going to have to update a lot of test cases if you<br>
>> make the change as you've written it - as we'll start producing empty<br>
>> strings ("") for the identifier field instead of null. Is that<br>
>> correct?<br>
><br>
> The verifier currently accepts both null and a StringRef, so there is no<br>
> verification error with the existing<br>
> llvm testing cases. Yes, we are now producing "" instead of null. To make<br>
> sure the existing llvm testing<br>
> cases follow what DIBuilder emits, we can update the testing cases or use a<br>
> null value when the<br>
> string is empty as suggested below.<br>
<br>
</div>It's the Clang test cases I'm concerned about - if you just commit<br>
this change, the Clang build will break, won't it?<br></blockquote><div><br></div><div>I attached a patch for updating testing cases earlier:</div><div>it basically changed from null to the actual mangled name.</div>
<div>One example:</div><div><p style="margin:0px;font-size:16px;font-family:Monaco;color:rgb(213,59,211);background-color:rgb(0,0,0)">-// CHECK: [[C:![0-9]*]] = {{.*}} metadata [[C_MEM:![0-9]*]], i32 0, metadata [[<span style="color:rgb(229,229,229);background-color:rgb(229,33,0)">C]], null, null} ; [ DW_TAG_structure_type ] [C] {{.*}} [def]</span></p>

<p style="margin:0px;font-size:16px;font-family:Monaco;color:rgb(52,187,199);background-color:rgb(0,0,0)">+// CHECK: [[C:![0-9]*]] = {{.*}} metadata [[C_MEM:![0-9]*]], i32 0, metadata [[<span style="color:rgb(229,229,229);background-color:rgb(229,33,0)">C]], null, metadata !"_ZTS1C"} ; [ DW_TAG_structure_type ] [C] {{.*}} [def]</span></p>
</div><div><br></div><div>By applying the 3 patches (clang, llvm, clang-test), I still see one failure<span style="font-family:arial,sans-serif;font-size:13px">: CodeGenCXX/debug-info-</span><span style="font-family:arial,sans-serif;font-size:13px">template.cpp fails with error: </span><span style="font-family:arial,sans-serif;font-size:13px">cannot yet mangle expression type CXXUuidoofExpr.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><font face="arial, sans-serif">This can be fixed by modifying ItaniumMangler (CXXNameMangler::mangleExpression), it currently has</font></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">FIXME: invent manglings for all these.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Thanks,</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">Manman</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<div><div class="h5"><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 pass<br>
>> > in<br>
>> > the unique<br>
>> > identifier.<br>
>> ><br>
>> > One patch on clang side to actually generate the unique identifier 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 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 DIBuilder::AllRetainTypes<br>
> to be VH instead of<br>
> plain Value *, since the types created can later on be modified. The 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>
</div></div>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>
<div class=""><div class="h5"><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 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 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>
</div></div></blockquote></div><br></div></div>