<div dir="ltr">Hi David,<div><br></div><div>Thanks for the fast reviewing.<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 26, 2013 at 12:58 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Generally we include test changes in patches that require them, rather<br>
than splitting them out. I assume in this instance you split the<br>
patches for ease of reviewing, but intend to commit them together (so<br>
as not to break the buildbots)<br></blockquote><div>Yes, I separated to make reviewing easier. The three patches will be submitted together. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
It looks like we're constraining the DICompositeType schema a little<br>
further than it was before - as you can see from your patch, some<br>
DICompositeTypes had 13 fields (no template argument field) and some<br>
have 14. Now they all have 15. I think this is OK, but just want to<br>
highlight it as a change in schema. </blockquote><div>Yes, all DICompositeTypes will have 15 fields, for types where template argument field was omitted, I</div><div>added an extra null. I will expand my commit message to emphasize the schema change.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(personally I don't like the vague<br>
optionality of trailing fields like that - if we're going to try to<br>
reduce the size of these records (there is a lot of redundancy in them<br>
- some type descriptions don't need nearly so many fields) we should<br>
sit down & do that deliberately rather than with little hacks, so<br>
making things more consistent is good as it'll make it easier to<br>
decide where to try to remove such redundancy)<br>
<br>
While you're here, you could change those lines that use<br>
"Constant::getNullValue" to just use NULL as we do in other places (I<br>
assume the "getNullValue" is how we end up with i32 0 as 'null' values<br>
sometimes? </blockquote><div>Yes, that is how we get "i32 0". </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">when we really want null values anyway, and it's shorter to<br>
write) such as in createSubroutineType, createEnumerationType,<br>
createArrayType - just on the lines you're already touching (because<br>
you have to add a ',') anyway. </blockquote><div>I will make the change.</div><div><br></div><div>Thanks,</div><div>Manman </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
But otherwise this looks correct - Please commit. (I haven't tested<br>
this, so leaving it up to you to provide assurances that your test<br>
changes are necessary & sufficient - I glanced at a few, but didn't<br>
rigorously investigate them all).<br>
<div class="HOEnZb"><div class="h5"><br>
On Mon, Aug 26, 2013 at 12:50 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
><br>
> Eric, David and I chatted on IRC and agreed to submit smaller patches.<br>
> We will add an identifier field to DICompositeType. For DICompositeTypes<br>
> with a non-null identifier field, its reference can<br>
> be replaced with the identifier to remove cycles and to help type uniquing.<br>
><br>
> As a first step, I am going to add a field to DICompositeType, the attached<br>
> patches implement that:<br>
> DICompositeType will have an identifier field at position 14. For now, the<br>
> field is set to null by DIBuilder.<br>
> Update verifier to check that DICompositeType has 15 fields and the last<br>
> field is null or a MDString.<br>
> Update testing cases to include an extra field for DICompositeType.<br>
> The identifier field will be used by type uniquing so a front end can<br>
> generate a DICompositeType with a unique identifier.<br>
><br>
> 3 patches are attached, two of them update testing cases.<br>
><br>
> Thanks,<br>
> Manman<br>
><br>
><br>
><br>
> On Fri, Aug 23, 2013 at 10:49 AM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>
> wrote:<br>
>><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>
>> ><br>
>> ><br>
>> ><br>
>> > I know dragonegg calls those type creation functions, and there are<br>
>> > maybe<br>
>> > other out-of-tree usages.<br>
>> > This patch provides default value for the unique name (an empty string)<br>
>> > to<br>
>> > the type creation functions, so<br>
>> > it is not going to break dragonegg or out-of-tree projects. Inside the<br>
>> > type<br>
>> > creation functions, if the passed-in<br>
>> > unique name is empty, DIBuilder will generate a unique name. I am not<br>
>> > sure<br>
>> > if it is necessary for dragonegg<br>
>> > to have its own implementation of generating the unique name, and how<br>
>> > hard<br>
>> > it is.<br>
>><br>
>> 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>
>><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>
>> + 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>
>><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>
>> >> +<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<br>
>> > one.<br>
>><br>
>> 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>
>><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>
>> 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>
>><br>
>> -eric<br>
>><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>><br>
>> >> 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<br>
>> >> > 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<br>
>> >> > 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<br>
>> >> > 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<br>
>> >> > corresponding<br>
>> >> > DIType given a type name.<br>
>> >> > 5> how to generate the unique name?<br>
>> >> > The clang frontend has two helper functions:<br>
>> >> > getUniqueTagTypeName<br>
>> >> > and<br>
>> >> > getUniqueTypeName<br>
>> >> > The first one handles TagType and it uses ODR (One<br>
>> >> > Definition<br>
>> >> > Rule) for external c++ types. It is currently called for struct,<br>
>> >> > class,<br>
>> >> > union and enum.<br>
>> >> > The second one can be used for general types, it uses<br>
>> >> > mangled<br>
>> >> > name<br>
>> >> > appended with CU's directory and file name to provide uniqueness<br>
>> >> > 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<br>
>> >> > 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<br>
>> >> > is<br>
>> >> > needed for dragonegg, before it provides a unique name (not sure<br>
>> >> > 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>
><br>
><br>
</div></div></blockquote></div><br></div></div></div>