<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 27, 2013 at 12:41 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=""><div class="h5">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>
</div></div>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></blockquote><div>The verifier currently accepts both null and a StringRef, so there is no verification error with the existing</div><div>llvm testing cases. Yes, we are now producing "" instead of null. To make sure the existing llvm testing</div>
<div>cases follow what DIBuilder emits, we can update the testing cases or use a null value when the</div><div>string is empty as suggested below.</div><div><br></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">

<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>
<div class="im"><br>
><br>
><br>
> Attaching patches to implement the above item:<br>
> One patch on llvm side to update DIBuilder interface:<br>
> createClassType, createStructType, createUnionType, createEnumerationType,<br>
> and createForwardDecl will take an optional StringRef for clang to pass in<br>
> the unique<br>
> identifier.<br>
><br>
> One patch on clang side to actually generate the unique identifier and 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>
</div>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></blockquote><div><br></div><div>That is another possibility, but we have to update DIBuilder::AllRetainTypes to be VH instead of</div><div>plain Value *, since the types created can later on be modified. The patch updates CGDebugInfo::RetainedTypes and</div>
<div>uses TypeCache to find the corresponding DIType in finalize(), there it calls DIBuilder.retainType to add to DIBuilder.AllRetainTypes.</div><div> </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 class="im"><br>
> 3> Input for getOrCreateRecordFwdDecl is modified from RecordDecl to<br>
> RecordType to enable<br>
> CXX mangler, same applies for CreateEnumType.<br>
<br>
</div>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></blockquote><div>Sure, I can pull out the part separately.</div><div> </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 class="im"><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>
</div>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></blockquote><div>I will look deeper at this and figure out why.</div><div><br></div><div><div>Thanks for the quick review.</div><div><br></div><div>Manman </div></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 class=""><div class="h5"><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 CXX<br>
> mangler<br>
> to generate the unique identifier: CodeGenCXX/debug-info-template.cpp fails<br>
> with error:<br>
> cannot yet mangle expression type CXXUuidoofExpr.<span style="color:rgb(34,34,34)"> </span></div></div></blockquote><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=""><div class="h5">
> We can definitely fix the above error in ItaniumMangle.cpp, but I want to<br>
> see whether the approach I am<br>
> taking is on the right track.<br>
><br>
> Thanks,<br>
> Manman<br>
</div></div></blockquote></div><br></div></div>