<div dir="ltr">Hi Eric,<div><br></div><div>Thanks for reviewing the patch.</div><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 22, 2013 at 12:48 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@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">Hi Manman,<br>
<br>
I'm still looking at the patch but one comment that will affect a lot<br>
of the text of the patch is that I think the unique name should be<br>
passed into the type creation functions, not created inside the<br>
backend. This means that each front end can use the definition of<br>
"unique" that most appeals to them.<br></blockquote><div><br></div><div><br></div><div>I know dragonegg calls those type creation functions, and there are maybe other out-of-tree usages.</div><div>This patch provides default value for the unique name (an empty string) to the type creation functions, so</div>
<div>it is not going to break dragonegg or out-of-tree projects. Inside the type creation functions, if the passed-in</div><div>unique name is empty, DIBuilder will generate a unique name. I am not sure if it is necessary for dragonegg</div>
<div>to have its own implementation of generating the unique name, and how hard it is.</div><div><br></div><div>The patch uses mangled name for C++ in clang, and it falls back to DIBuilder to provide a unique name for</div>
<div>other languages. I am not sure whether front end can provide a better name than DIBuilder, so instead of</div><div>requiring each front end to have an implementation for generating unique name, I choose to have a centralized</div>
<div>implementation in DIBuilder. That decision of course is debatable.</div><div> <br></div><div>In summary, the patch does the following in terms of name generation:</div><div><span style="color:rgb(80,0,80)"><br></span></div>
<div><span style="color:rgb(80,0,80)">></span><span style="color:rgb(80,0,80)">The clang</span><span style="color:rgb(80,0,80)"> frontend has two helper functions: getUniqueTagTypeName and</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> getUniqueTypeName</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> The first one handles TagType and it uses ODR (One Definition</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> Rule) for external c++ types. It is currently called for struct, class,</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> union and enum.</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> The second one can be used for general C++ types, it uses mangled name</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> appended with CU's directory and file name to provide uniqueness across CUs.</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> It is currently used to generate unique name for subroutine type. </span></div><div><span style="color:rgb(80,0,80)">> For other </span><span style="color:rgb(80,0,80)">types, DIBuilder will provide a unique name. </span><span style="color:rgb(80,0,80)">DIBuilder also has a helper</span></div>
<div><span style="color:rgb(80,0,80)">> function getUniqueTypeName, this is called when clients of DIBuilder do not provide a unique name.</span></div><div><span style="color:rgb(80,0,80)">> Note that Clang can handle more types in</span></div>
<div><span style="color:rgb(80,0,80)">> follow-on patches if it can provide a better name than DIBuilder.</span><br></div><div><span style="color:rgb(80,0,80)"><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">
<br>
Also I'm not sure why you've got an actual hashing algorithm in there.<br>
Can you explain?<br></blockquote><div><br></div><div>Which hashing algorithm are you referring to?</div><div>I don't recall a hashing algorithm in the 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">
<br>
Couple other things I noticed:<br>
<br>
+ // If the consumer of DIBuilder does not provide a unique name, generate one<br>
+ // here.<br>
+ MDString *UniqueStr = UniqueName.empty() ?<br>
+ getUniqueTypeName(dwarf::DW_TAG_structure_type, Name) :<br>
+ MDString::get(VMContext, UniqueName);<br>
<br>
I don't think we should do this. Have an example where we'd want to?<br></blockquote><div><br></div><div>The patch has a default value of an empty string for the type creation function, so if the caller of the type</div>
<div>creation function does not provide a name, DIBuilder will come up with one.</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>
+ Map.insert(std::make_pair(<br>
+ std::make_pair(Ty.getUniqueName(), Ty.isForwardDecl()),<br>
+ Ty));<br>
<br>
A pair of a pair is a bit icky here as well.<br></blockquote><div>Any suggestion here?</div><div><br></div><div>Thanks,</div><div>Manman</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>
This should be enough to start discussion/revision of the patch.<br>
<span class=""><font color="#888888"><br>
-eric<br>
</font></span><div class="im"><br>
On Wed, Jul 31, 2013 at 4:37 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
><br>
</div><div class=""><div class="h5">> After cleaning up usage of the verify function and all the testing cases,<br>
> the first set of patches are ready for review :)<br>
><br>
> 3 patches are included: one for clang (tu_clang_07312013.patch), one for<br>
> code change of llvm (tu_07312013.patch), and one for updating all the<br>
> testing cases (tu_test_07312013.patch).<br>
><br>
> The following are implemented:<br>
> 1> DIType will have a unique name at field 9.<br>
> For DIBasicType, DICompositeType, DIDerivedType, the fields after will<br>
> be shifted by 1.<br>
> 2> I am going to update usage of DIType in phases:<br>
> At the first batch, getContainingType of DICompositeType and<br>
> DISubprogram will be replaced with a MDString, as well as getClassType of<br>
> DIDerivedType.<br>
> 3> The verifier is updated according to 1 and 2.<br>
> 4> DwarfDebug has a TypeMap.<br>
> It also has a helper function lookupType to find the corresponding<br>
> DIType given a type name.<br>
> 5> how to generate the unique name?<br>
> The clang frontend has two helper functions: getUniqueTagTypeName and<br>
> getUniqueTypeName<br>
> The first one handles TagType and it uses ODR (One Definition<br>
> Rule) for external c++ types. It is currently called for struct, class,<br>
> union and enum.<br>
> The second one can be used for general types, it uses mangled name<br>
> appended with CU's directory and file name to provide uniqueness across CUs.<br>
> It is currently used to generate unique name for subroutine type. For other<br>
> types, DIBuilder will provide a unique name. Clang can handle more types in<br>
> follow-on patches if it can provide a better name than DIBuilder.<br>
> DIBuilder also has a helper function getUniqueTypeName, this is called<br>
> when consumer of DIBuilder does not provide a unique type name. This is<br>
> needed for dragonegg, before it provides a unique name (not sure whether it<br>
> is needed).<br>
> 6> All DITypes created by DIBuilder will be added to RetainTypes.<br>
><br>
> Comments and feedback are welcome.<br>
><br>
> Manman<br>
><br>
</div></div></blockquote></div><br></div></div>