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

[David Blaikie]

On Mon, Jul 29, 2013 at 2:18 PM, Eric Christopher <echristo at gmail.com> wrote:
> 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.

Seems like we'd still not want to skip over function scopes in type
unit hashes or we'd get collissions:

void f1() { struct foo { }; }
void f2() { struct foo { }; }

>>>>>> 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.

Right - I haven't seen the iterative version, so perhaps I'll
understand what you mean if/when I see it. You're right though, I can
at least picture that - build a stack of parents, then walk back down.
(weird - I wonder what artifact of GCC's implementation lead them to
hashing down rather than up - I guess they split out the type DIE &
its parents first and then simply walked the whole tree?)

>>>>> 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.

Sure - all I'm suggesting here is filing the bug & changing the
comment. When the bug is fixed isn't something I'm concerned with in
this review.

- Dave