<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Sep 4, 2013 at 4: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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div class="im">On Tue, Sep 3, 2013 at 1:54 PM, Manman Ren <span dir="ltr"><<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@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 dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div>On Tue, Sep 3, 2013 at 1:24 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">Thanks - few follow-on or outstanding issues:<br>
<br>
Does getFieldAs<DYTypeRef> require an explicit specialization? I<br>
speculated about this on IRC, but it was only a speculation/guess, I<br>
didn't mean to claim I knew it was necessary. Looking at the code, the<br>
implementation of that specialization looks the same as the original<br>
template so can probably be omitted.<br></blockquote></div><div>getFieldAs used to handle subclass of DIDescriptor, it treats the field as MDNode and constructs a DIDescriptor from it.</div></div></div></div></blockquote>
<div><br></div></div><div>Sure, but it wouldn't need to - getDescriptorField could be replaced by the getField utility function in DebugInfo.cpp... except it returns Value* and only DITypeRef can be constructed from a Value*, all the other DI* need a MDNode*. OK, so I see now why we need a specialization here.<br>
</div><div class="im"><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 dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div>For DITypeRef, the field can be either a MDNode or a MDString, that is why getField instead of getDescriptorField is used</div>
<div>for DITypeRef.</div><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">
<br>
But if we do need the specialization, could we befriend it (or even<br>
just befriend the whole template) and make the DITypeRef ctor private?<br></blockquote></div><div>Can we actually befriend a specialization where class DITypeRef is not a template?</div></div></div></div></blockquote><div>
<br></div></div><div>Yep, seemed to work for me. Just put this:<br><br> friend DITypeRef DIDescriptor::getFieldAs<DITypeRef>(unsigned Elt) const;<br><br>in DITypeRef, instead of the template friend you have there.</div>
</div></div></div></blockquote><div>Yes, updated in the attached patch.</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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im"><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>If we befriend the whole template, we also need to make DITypeRef a friend of DIDescriptor so DITypeRef</div>
<div>can access the protected member "DIDescriptor::getFieldAs".</div><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">
<br>
Remove the "protected" section from DITypeRef - the member should be<br>
private & that's the default in a class anyway.<br></blockquote></div><div>Will do. </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">
<br>
The documentation comment for DITypeRef discusses implementation<br>
details that aren't really important/relevant to the contract of the<br>
class. I'd probably just say something like "abstracts over direct and<br>
identifier-based metadata type references" or the like.<br></blockquote></div><div>Are you referring to the comments above the class definition?</div><div>+ /// A thin wrapper to represent reference to a DIType. It wraps around Value*,</div>
<div>+ /// which can be either a MDNode or a MDString, in the latter, MDString</div><div>+ /// specifies the type identifier.</div><div>+ class DITypeRef { </div></div></div></div></blockquote><div><br></div></div><div>
Yes, I was referring to those comments.</div></div></div></div></blockquote><div>Updated in the attached patch. </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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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>
fieldIsMDStringOrMDNode's name is non-descript in that it describes<br>
the implementation, not the purpose. "isTypeRef" might be<br>
simpler/better (this could even be re-used for the assert in<br>
DITypeRef's ctor if the function can be placed somewhere sufficiently<br>
general - maybe even a static member function of DITypeRef)<br></blockquote></div><div>I thought about that, the only difference is one handles a field of a MDNode,</div><div>the other handles the actual Value.</div><div>
Should we introduce fieldIsTypeRef and isTypeRef?</div></div></div></div></blockquote><div><br></div></div><div>fieldIsTypeRef looks good. As you say, since this deals with looking up a particular field in an mdnode, there's not an immediate reuse with the DITypeRef ctor. I don't think it's so critical then. I see you've changed it already - that's fine.</div>
<div class="im">
<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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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">
<br>
Rename "lookupType" to "resolve" for consistency, perhaps?<br>
(problematic to have different verbs/names for the same concept)<br></blockquote></div><div>Will do.</div><div><br></div><div>Thanks again for the quick review.</div></div></div></div></blockquote><div><br></div></div><div>
The comment added "getClassType()" can probably be omitted. It documents DITypeRef's semantics (& implementation details I mentioned weren't really public documentation for the type, let alone functions that return objects of that type) which are out of place here. We won't want to add such a documentation comment to every function that returns or handles a DITypeRef - we know what they are or we can look at the implementation (& comments, doc or otherwise) to find out, rather than redundantly documenting it at every use.<br>
</div></div></div></div></blockquote><div><br></div><div>I forgot to update the comments after wrapping Value* in DITypeRef. Updated in the attached patch. </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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<br>I'm still thinking that "generateDITypeIdentifierMap" should return a DITypeIdentifierMap, rather than take one by const ref. It'll model the semantics more clearly (this doesn't append to the map, or shouldn't - it does all the work to build the map across all the CUs & should do just that/not be affected by the prior state of the map, etc). DenseMap has a move assignment operator that should keep the performance characteristics fairly similar to the ref-passing implementation.<br>
</div></div></div></div></blockquote><div>Done in attached patch. </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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<br>DITypeRef::resolve has a few "return DIType(...);" that can be written as "return ...;" instead (due to the implicit DIType(const MDNode*) ctor that you're relying on in "return NULL;" anyway)<br>
</div></div></div></div></blockquote><div>Done in attached patch. </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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<br>Is there any reason to build the map of all the types in /all/ the CUs? rather than just building up a map for each CU as we visit it? (in the scope of the for loop a few lines below the "generateDITypeIdentifierMap" call)</div>
</div></div></div></blockquote><div> I don't see a for loop below the "generateDITypeIdentifierMap" call. Are you referring to the implementation of generateDITypeIdentifierMap :)</div><div>When I search for "for (", they are all in the body of generateDITypeIdentifierMap.</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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>
<br>Why does DIDescriptor befriend DITypeRef?</div></div></div></div></blockquote><div>If we don't, DITypeRef can't befriend the protected member function of DIDescriptor (DIDescriptor::getFieldAs).</div><div><br>
</div><div>Thanks,</div><div>Manman </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 dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div><span class=""><font color="#888888"><br><br>- David<br><br><br></font></span></div><div><div class="h5"><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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div>Manman</div></font></span><div><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><div><br>
<br>
<br>
<br>
On Tue, Sep 3, 2013 at 12:54 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>> wrote:<br>
> Attaching updated patch that should handle the review requests.<br>
><br>
> Thanks,<br>
> Manman<br>
><br>
><br>
> On Fri, Aug 30, 2013 at 3:13 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> On Fri, Aug 30, 2013 at 3:10 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>> wrote:<br>
>> > Once again, thanks for the quick review :)<br>
>> ><br>
>> ><br>
>> > On Fri, Aug 30, 2013 at 11:19 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Looks generally reasonable.<br>
>> >><br>
>> >> I take it you're using pointer_to_member's base type as the first<br>
>> >> field example & we'll see similar, smaller patches (not adding any<br>
>> >> infrastructure, just porting another field over) for each field from<br>
>> >> here on out? It seems like a nice, small example - thanks for that.<br>
>> ><br>
>> > Yes, that is the plan.<br>
>> >><br>
>> >><br>
>> >> lib/IR/DebugInfo.cpp<br>
>> >> - DIDerivedType::Verify<br>
>> >> * we should have a single function that does<br>
>> >> "fieldIsMDNode/fieldIsMDString" because we're going to use that test<br>
>> >> for every type field<br>
>> ><br>
>> > Will do.<br>
>> >><br>
>> >> - generateDITypeIdentifierMap<br>
>> >> * any reason this wouldn't be a ctor of DITypeIdentifierMap (or at<br>
>> >> least a member function)?<br>
>> ><br>
>> > The current patch does not wrap around DITypeIdentifierMap, that is why<br>
>> > generateDITypeIdentifierMap<br>
>> > is a helper function. If we use a class for DITypeIdentifierMap, the<br>
>> > only<br>
>> > member will be a DenseMap and<br>
>> > it can have member functions for generating the map and looking up<br>
>> > values.<br>
>><br>
>> Ah, right - yeah, it's probably not quite worth a separate type, then.<br>
>> Optionally - we could return a DITypeIdentifierMap by value rather<br>
>> than passing one in to populate, if the initialization happens at the<br>
>> point of construction to take advantage of NRVO (or if DenseMap has<br>
>> move-related overloads in C++11 mode, that'd suffice too).<br>
>><br>
>> ><br>
>> >> * "DICompositeType Ty = DICompositeType(x);" can be rewritten as<br>
>> >> "DICompositeType Ty(x);"<br>
>> ><br>
>> > Yes, will make the change.<br>
>> ><br>
>> >><br>
>> >> * map lookup and insertion should be combined, probably simply like<br>
>> >> this:<br>
>> >> pair<iterator, bool> p = Map.insert(make_pair(TypeId, Ty)); //<br>
>> >> try to insert<br>
>> >> if (!p.second && !Ty.isForwardDecl()) // if we didn't insert<br>
>> >> because the element was already there<br>
>> >> p.first->second = Ty; // replace whatever we had (declaration<br>
>> >> or definition) with a definition<br>
>> >> (pseudocode, comments for illustration only)<br>
>> ><br>
>> > Good deal, one search of the map instead of two.<br>
>> >><br>
>> >><br>
>> >> DwarfDebug.cpp:<br>
>> >> - lookupType<br>
>> >> * assert should have an explanatory string in it<br>
>> ><br>
>> > Yes.<br>
>> >><br>
>> >><br>
>> >> 'getClassType' - this is going to become pervasive in every DI*<br>
>> >> function that returns DIType, right? I think it might be helpful to<br>
>> >> add an opaque type here so we're not just referring to any Value.<br>
>> >> Either something totally opaque, a class that befriends the<br>
>> >> DIDescriptor class (for construction) and the map (for lookup) &<br>
>> >> otherwise has no public members except simple copying, etc. Or perhaps<br>
>> >> a PointerUnion of MDNode/MDString - this is unnecessary, of course,<br>
>> >> since the Value hierarchy already knows which object it is, so there's<br>
>> >> no need to discriminate the pointer in actuality...<br>
>> ><br>
>> > I will think about this more and chat with you on IRC.<br>
>> ><br>
>> > Thanks,<br>
>> > Manman<br>
>> ><br>
>> >><br>
>> >><br>
>> >> On Fri, Aug 30, 2013 at 10:40 AM, Manman Ren <<a href="mailto:manman.ren@gmail.com" target="_blank">manman.ren@gmail.com</a>><br>
>> >> wrote:<br>
>> >> > Also add a typedef for DITypeIdentifierMap and a helper<br>
>> >> > generateDITypeIdentifierMap in DebugInfo.h. In DwarfDebug.cpp, we<br>
>> >> > keep<br>
>> >> > a DITypeIdentifierMap and call generateDITypeIdentifierMap to<br>
>> >> > actually<br>
>> >> > populate the map. DwarfDebug also has a helper function lookupType<br>
>> >> > that<br>
>> >> > finds the type MDNode for a given Value (either an identifier in<br>
>> >> > MDString<br>
>> >> > or the actual MDNode).<br>
>> >> ><br>
>> >> > Verifier is updated accordingly.<br>
>> >> ><br>
>> >> > 3 patches are attached:<br>
>> >> > The first one is a simple refactoring to add getField to DIDescriptor<br>
>> >> > so<br>
>> >> > we<br>
>> >> > can use it to return Value* for getClassType.<br>
>> >> ><br>
>> >> > The second one is the actual change to DIBuilder and DwarfDebug etc<br>
>> >> > with<br>
>> >> > an<br>
>> >> > added testing case.<br>
>> >> ><br>
>> >> > The last one is to update testing cases in clang.<br>
>> >> ><br>
>> >> > Thanks,<br>
>> >> > Manman<br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>