[llvm] r187213 - Add preliminary support for hashing DIEs and breaking them into

[Eric Christopher]

On Mon, Jul 29, 2013 at 10:37 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>> +  // For each surrounding type or namespace...
>>>>>
>>>>> What surrounding things can we expect that aren't namespaces and types?
>>>>
>>>> The compile unit? But as you ask later, this is actually a quote.
>>>
>>> For myself it wasn't obvious what the condition was avoiding - perhaps
>>> it'd be helpful. If it's only the compile_unit, that should always be
>>> the top level, so shouldn't we just be able to replace this condition
>>> and the next one with "if it's not a compile unit, recurse" (or loop,
>>> if we do it with a loop instead) - with an assert that it should be a
>>> namespace, class, or struct? That seems a bit more
>>> explicit/self-documenting.
>>
>> Oh, also functions. Missed that one in my description to you as well
>> which makes it more complicated. Sorry about that.
>
> Hmm - skipping past function scopes? That seems strange. I would
> expect that either we never ODR hash function-local types or, if we do
> so, we'd want/need to include the function in the hash.
>

We don't, but it's the same hashing for parent scopes that we use in
the type unit/full CU hash.

> Again, I think I'd find this more legible if it were a bit more
> explicit about what it's doing.
>

That's fine.

>>>>> I'd sort of rather a loop than a recursion, though I realize the
>>>>> recursion won't be terribly deep.
>>>>>
>>>>
>>>> Heh. I'd originally written it that way
>>>
>>> Any particular reason you chose to rewrite it recursively?
>>
>> Made it more compact - though not terribly. I can undo it if you'd like.
>
> I kind of would - while I'm not one to throw stones about writing
> overly compact code, it is a bit unobvious when glancing at the code
> that it's looping, just recursively.
>

*nod* I found this easier to reason about than the iterative merely
because I have to find the top of the stack and then go back down, but
it's not a trial either way.


> In the type unit/CU hashing standard, you mean? Yeah, as we've
> agreed/mentioned - the standard definition of hashing is essentially
> irrelevant. At best it's a "recommended implementation", but there
> doesn't seem to be any benefit to treating it as a necessary way to
> compute the hash of the DIE - I really don't see much reason not to
> diverge wherever we see fit.
>

Agreed.

>>>> Not really, and we're definitely going to be grabbing lots of
>>>> attributes with string representations as we hash whole DIEs - and
>>>> unfortunately it's a sorted list. :\
>>>
>>> I guess you meant it's not sorted?
>>
>> The ordering of DIEs to hash has a particular sort. The order that
>> they're added to the DIE is not sorted. We could, if we wanted, sort
>> things as they're added to the DIE.
>
> Well, once we've got the sorting in that we need for hashing, then
> we'll probably be able to do the name lookup for the ODR hash (which
> is "after full DIE construction" anyway - so will be at a similar
> point to type/CU hashing, I assume... oh, well, a step before (since
> you'll need to hash the ODR hash I guess)) we can rely on the sorting.
> But, yes, might be easier just to maintain a sorted invariant rather
> than a sorted and unsorted dynamic state.
>

This will require some reworking on the part of values for the DIEs,
but it's possible. I just have been avoiding doing that. The current
code I've got to deal with all of the attributes actually puts them
into the custom sorted list because then I just have to deal with it
once. I'm not actually going to sort the attributes in the data
structure.


>>>> It's in an anonymous namespace.
>>>
>>> Again, could be helpful to include that in the comment somehow. That
>>> almost seems bug-worthy - we have a global variable of that type yet
>>> no debug info for it? That seems strange.
>>
>> We may wish to emit debug info for w (and the type walrus), we
>> definitely don't want to emit a hash. I'm not sure which thing you
>> considered bug worthy. The former is, the latter isn't :)
>
> Well this test can't possibly say one way or the other on the latter -
> since there's no definition for the variable/type at all. So the only
> bug this could demonstrate is that we don't emit the type definition
> for 'walrus' (but, yes, it would help to include in the comment
> "if/when we do fix PRXXXX, we still shouldn't emit an ODR hash for
> it").
>

Again, no argument, just something else that I'll need to look at.

-eric