<div dir="ltr">In r189282 and r189283.<div><br></div><div>I will work on clang front-end to generate the unique identifier for DICompositeType.</div><div><br></div><div>Thanks,</div><div>Manman </div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Mon, Aug 26, 2013 at 1:40 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">
<div class="im">On Mon, Aug 26, 2013 at 1:20 PM, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
> Hi David,<br>
><br>
> Thanks for the fast reviewing.<br>
<br>
</div>Sure thing - thanks for making it easy to review quickly!<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> On Mon, Aug 26, 2013 at 12:58 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> 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>
><br>
> Yes, I separated to make reviewing easier. The three patches will be<br>
> submitted together.<br>
>><br>
>><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.<br>
><br>
> Yes, all DICompositeTypes will have 15 fields, for types where template<br>
> argument field was omitted, I<br>
> added an extra null. I will expand my commit message to emphasize the schema<br>
> change.<br>
><br>
>><br>
>> (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?<br>
><br>
> Yes, that is how we get "i32 0".<br>
>><br>
>> 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.<br>
><br>
> I will make the change.<br>
><br>
> Thanks,<br>
> Manman<br>
>><br>
>><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>
>><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<br>
>> > uniquing.<br>
>> ><br>
>> > As a first step, I am going to add a field to DICompositeType, the<br>
>> > attached<br>
>> > patches implement that:<br>
>> > DICompositeType will have an identifier field at position 14. For now,<br>
>> > 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<br>
>> >> >> 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<br>
>> >> > string)<br>
>> >> > to<br>
>> >> > the type creation functions, so<br>
>> >> > it is not going to break dragonegg or out-of-tree projects. Inside<br>
>> >> > 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<br>
>> >> >> 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<br>
>> >> > creation<br>
>> >> > function, so if the caller of the type<br>
>> >> > creation function does not provide a name, DIBuilder will come up<br>
>> >> > 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),<br>
>> >> >> > one<br>
>> >> >> > for<br>
>> >> >> > code change of llvm (tu_07312013.patch), and one for updating all<br>
>> >> >> > 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.<br>
>> >> >> > 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<br>
>> >> >> > is<br>
>> >> >> > called<br>
>> >> >> > when consumer of DIBuilder does not provide a unique type name.<br>
>> >> >> > 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>
><br>
><br>
</div></div></blockquote></div><br></div>