<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jul 18, 2013, at 9:58 PM, David Blaikie wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><p dir="ltr"><br>
On Jul 18, 2013 6:31 PM, "Eric Christopher" <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
><br>
> > Somehow I can't put inline comments.<br>
><br>
> Beta software? ;)<br>
><br>
> From David:<br>
><br>
> ><br>
> > Rather than having foo.getThing(map) we'd have something like<br>
> > lookup(foo.getThing()) - we'd need a return type from getThing that<br>
> > represent as the type identifier (the thing we keep calling a 'hash' but it<br>
> > isn't), we could just use stringref or something more specific.<br>
> ><br>
><br>
> From you:<br>
> > Is lookup here a global function like "llvm::isSubprogramContext" or a<br>
> > member function of the DIMap?<br>
> > If the former, then it should be lookup(StringRef TypeName, DIMap &), right?<br>
> > We also need to modify foo.getThing() to return a StringRef instead of a<br>
> > DIType, right?<br>
> > In terms of lines of change, two options should be similar.<br>
> ><br>
><br>
> I was thinking the former yes. </p><p dir="ltr">I was picturing just a helper member function of dwarfdebug so it could be written in that context as lookup(foo.getThing()), but short of that, if we actually have a dimap type, it probably makes sense as a member of that. If we don't then it should be fine as a free function.</p><p dir="ltr">> It should probably return a StringRef,<br>
> or an DITypeHandle, or something. Leave the actual choice there to<br>
> you.</p><p dir="ltr">A typedef might be healthy here at least, so we can change it later of we need to.</p><p dir="ltr">><br>
> > About the Verify functions, the patch modifies from Verify() to Verify(DIMap<br>
> > &). Is this interface change okay?<br>
><br>
> Again, I was hoping not to change the signature of these functions if possible.</p><p dir="ltr">Agreed. Let's drop anything from verify that needs type resolution, unless there's a good reason not to. We should move verification to a single pass at some point which could use resolution there,</p><p dir="ltr">><br>
> > What about isUnsignedDIType, getOriginalTypeSize, isBlockByrefVariable, and<br>
> > isSubprogramContext? Should they take an additional DIMap?<br>
> > - bool isUnsignedDIType();<br>
> > + bool isUnsignedDIType(const DITypeHashMap &Map);<br>
> ><br>
> > Are the changes okay for<br>
> > - bool isArtificial() const {<br>
> > + bool isArtificial(const DITypeHashMap &Map) const {<br>
> > and other member functions in DbgVariable?<br>
> ><br>
><br>
> From David:<br>
><br>
> > As we discussed before, we shouldn't need conditional lookup (so we<br>
> > shouldn't be in a state where 'getThing' returns something that might be a<br>
> > type identifier or might be the type itself). We should just migrate one<br>
> > field at a time (or a bunch or all of them, but not switching the consuming<br>
> > code before the production code, otherwise were just adding a bunch of<br>
> > dead/untested code)<br>
> ><br>
><br>
> From you:<br>
><br>
> > There are some cases where a thing can be either a StringRef or a<br>
> > DIDescriptor such as getContext() or getElement() of a DIArray.<br>
> > What type should getElement() or getContext() return? A Value*?<br>
> ><br>
> > Downside for what is implemented in the patch:<br>
> > What if someone just wants the StringRef? (the patch added<br>
> > getNameDerivedFrom, Value* getType)<br>
> ><br>
> > Downside for the suggested lookup:<br>
> > Verify takes a DIMap, getThing does not<br>
> > getThing returns StringRef, getContext, getElement returns a Value<br>
> > It requires updating the testing cases since the thing must be a StringRef.<br>
> ><br>
> > The first two items are not really downsides, I am just worried about the<br>
> > last item, see below.<br>
> ><br>
><br>
> I think your plan above there will work. It'll come down to which one<br>
> is cleaner ultimately. Our feeling was that it'd be cleaner not<br>
> passing the map everywhere and doing the lookup separately to keep the<br>
> concerns separate of "field" and "underlying value".<br>
><br>
> The below is thinking out loud:<br>
><br>
> As far as using Value * here that seems reasonable... or add a new<br>
> class of DIType that is a "reference to" type that just contains the<br>
> string and can be passed around like everything else, then lookup just<br>
> needs to happen when we want to change/emit it. How's that sound?</p><p dir="ltr">Gut reaction: value* is top general and deriving from ditype is incorrect, since the whole api would be invalid as the type couldnt be implicitly resolved on demand (adding an extra state to this special ditype, which seems unfortunate). I think a simple type with a 'resolve(map)' function (could be written as a non member, in which case the type would need members to differentiate and return the ditype or string/ditypehandle) that returns the ditype one way or another, internally if use a pointerunion of string value or mdnode, perhaps? (Yeah, just a value pointer would do, with dyn cast, I suppose, but at least it'd be hidden away in that type)</p><p dir="ltr">><br>
> > There are additional code changes required to migrate a single field:<br>
> > 1> update the backend to support one MDNode (DIE) used in two different CUs.<br>
> > 2> modify DIBuilder to use StringRef instead of DIType<br>
> > 3> to generate the unique String name in clang.<br>
><br>
> #3 is definitely a requirement for this. The "name" of a type<br>
> absolutely must be in the IR in place of any DIType fields. I'm not<br>
> quite sure what you mean by #1 and #2.</p><p dir="ltr">1 and 2 make sense to me, I think, I'll try to explain them to you in person, as I understand them.</p><p dir="ltr">But these changes can be made incrementally - the backend and frontend need to change the schema atomically (including updating test cases), but once per field not for all the fields together (yes, this means thrashing the test cases as I did in the past, by that's OK). But 2 can then be done separately for each of those changes - adding the merging support.</p><div><br></div></blockquote>Attaching patches for 1, 2 and 3.</div><div>1 is needed at least for performance, to avoid creating multiple DIEs for a single MDNode that is shared across CUs.</div><div>We used to have a map in DwarfCompileUnit which maps from a MDNode to a DIE.</div><div>I am not sure whether it is required for correctness.</div><div><br></div><div>2 can be incremental if we are going to update a batch of fields at a time.</div><div><br></div><div>I will get back to you on other issues soon.</div><div><br></div><div>Thanks</div><div>Manman</div><div></div></body></html>