<div dir="ltr"><div><br></div>Eric, David and I chatted on IRC and agreed to submit smaller patches.<div>We will add an identifier field to DICompositeType. For DICompositeTypes with a non-null identifier field, its reference can</div>
<div>be replaced with the identifier to remove cycles and to help type uniquing.</div><div><br></div><div>As a first step, I am going to add a field to DICompositeType, the attached patches implement that:</div><div>DICompositeType will have an identifier field at position 14. For now, the field is set to null by DIBuilder.</div>
<div>Update verifier to check that DICompositeType has 15 fields and the last field is null or a MDString.</div><div>Update testing cases to include an extra field for DICompositeType.</div><div>The identifier field will be used by type uniquing so a front end can generate a DICompositeType with a unique identifier.</div>
<div><br></div><div>3 patches are attached, two of them update testing cases.</div><div><br></div><div>Thanks,</div><div>Manman</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 23, 2013 at 10:49 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">>><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>
><br>
><br>
><br>
> I know dragonegg calls those type creation functions, and there are maybe<br>
> other out-of-tree usages.<br>
> This patch provides default value for the unique name (an empty string) to<br>
> the type creation functions, so<br>
> it is not going to break dragonegg or out-of-tree projects. Inside the type<br>
> creation functions, if the passed-in<br>
> unique name is empty, DIBuilder will generate a unique name. I am not sure<br>
> if it is necessary for dragonegg<br>
> to have its own implementation of generating the unique name, and how hard<br>
> it is.<br>
<br>
</div>It shouldn't be too hard, the question is what should go into the<br>
field. I don't know how comfortable I am with emitting a "unique" name<br>
out of the backend if one isn't provided. I think the best method here<br>
would be this:<br>
<br>
The type field can either be an MDString or an MDNode. The MDNode will<br>
be the referenced type as is currently used. The MDString would be the<br>
unique name for the type that will be used to disambiguate as we've<br>
discussed (also to remove cycles and references so we can unique).<br>
This will avoid the need to change clients except for those that wish<br>
to take advantage of this.<br>
<br>
Here's a very incremental way of doing the patch I think:<br>
<br>
1) Add a field to DIComposite type which is initially just a direct<br>
reference back to itself.<br>
2) Ensure that these end up in the retain types list.<br>
3) Then build the map based on that key, which is just a node<br>
reference at the moment.<br>
- Each field can thus be done incrementally and mapped one at a time.<br>
4) Then start passing in strings one at a time.<br>
<br>
This will make it easier to review etc.<br>
<br>
Then we can get to the declaration deduplication and<br>
declaration/defined type merging.<br>
<br>
What do you think?<br>
<div class="im"><br>
>> Also I'm not sure why you've got an actual hashing algorithm in there.<br>
>> Can you explain?<br>
><br>
><br>
> Which hashing algorithm are you referring to?<br>
> I don't recall a hashing algorithm in the patch.<br>
><br>
<br>
</div>+ static unsigned getHashValue(const Pair& PairVal) {<br>
+ uint64_t Key = (uint64_t)hash_value(PairVal.first) << 32<br>
+ | (uint64_t)PairVal.second;<br>
+ Key += ~(Key << 32);<br>
+ Key ^= (Key >> 22);<br>
+ Key += ~(Key << 13);<br>
+ Key ^= (Key >> 8);<br>
+ Key += (Key << 3);<br>
+ Key ^= (Key >> 15);<br>
+ Key += ~(Key << 27);<br>
+ Key ^= (Key >> 31);<br>
+ return (unsigned)Key;<br>
+ }<br>
<br>
Related to the many pairs that we talk about below.<br>
<div class="im"><br>
>> Couple other things I noticed:<br>
>><br>
>> + // If the consumer of DIBuilder does not provide a unique name,<br>
>> generate one<br>
>> + // here.<br>
>> + MDString *UniqueStr = UniqueName.empty() ?<br>
>> + getUniqueTypeName(dwarf::DW_TAG_structure_type,<br>
>> 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>
><br>
><br>
> The patch has a default value of an empty string for the type creation<br>
> function, so if the caller of the type<br>
> creation function does not provide a name, DIBuilder will come up with one.<br>
<br>
</div>Yeah, not such a fan. Either we should do this in such a way that we<br>
don't require outside clients to update, or decide that we don't care.<br>
<div class="im"><br>
>> + Map.insert(std::make_pair(<br>
>> + std::make_pair(Ty.getUniqueName(),<br>
>> Ty.isForwardDecl()),<br>
>> + Ty));<br>
>><br>
>> A pair of a pair is a bit icky here as well.<br>
><br>
> Any suggestion here?<br>
><br>
<br>
</div>I'm not sure what each field is being used for in the map so I can't<br>
provide suggestions. Perhaps you can explain what's going on? (The<br>
StringRef and the DIType are obvious, the bool not so much.)<br>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> Thanks,<br>
> Manman<br>
><br>
>><br>
>><br>
>> This should be enough to start discussion/revision of the patch.<br>
>><br>
>> -eric<br>
>><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>
>> > After cleaning up usage of the verify function and all the testing<br>
>> > 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<br>
>> > 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<br>
>> > 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<br>
>> > 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<br>
>> > name<br>
>> > appended with CU's directory and file name to provide uniqueness across<br>
>> > CUs.<br>
>> > It is currently used to generate unique name for subroutine type. For<br>
>> > other<br>
>> > types, DIBuilder will provide a unique name. Clang can handle more types<br>
>> > in<br>
>> > follow-on patches if it can provide a better name than DIBuilder.<br>
>> > DIBuilder also has a helper function getUniqueTypeName, this is<br>
>> > 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<br>
>> > 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>
><br>
><br>
</div></div></blockquote></div><br></div>