On Thu, 2011-01-27 at 18:38 -0800, Roland McGrath wrote:
Ultimately dwarf_output::debug_info_entry should probably not
recompute the
local hash at all. That object only comes into existence when a
pending_entry is ready to be finalized in the collector. So when a new one
is being created in the collector, it can just store the local hash value
already computed in the pending_entry.
Yes. I just had a bit of an issue figuring out how stuff was passed to
the dwarf_output::debug_info_entry constructor, and it isn't really,
because it gets constructed through this () operator conversion step.
But having all hashes (local and global) in pending_entry would be
beneficial. Currently however we try to drop the pending_entry as soon
as possible whenever we finalize an entry. So maybe we should move them
both into entry. Also because entry already has the parent which is used
in the tracker for the matching algorithm.
So I see you decided to go with computing a hash value for
attributes_type
and a hash value for children_type, so the debug_info_entry hash value is
just the hash_combine of those with the tag. That is fine, but the
write-up of the hashing algorithm should be precise about that. It's not
exactly the same (arithmetically) as combining the tag, the hashes of each
attribute, and the hashes of each child, which is how you described it. As
I said before, I don't think it matters a lot to the quality of the hash
which way it's done. But we're confused enough already that having every
detail of the canonical write-up match what the code actually does is a
good idea. (Perhaps that write-up should live on the wiki rather than just
in the list archive.)
Yes, again I admit that the way this evolved was mainly because of the
way I was most easily able to change the code. Ideally I would have
added the local hash code to the attributes themselves, but it seemed
dwarf_data::value_reference, but I really couldn't figure out how things
got initialized correctly in that case. And it wasn't clear to me
whether the concept of local hashes really made much sense to any other
subclass/template instance of that type. I'll make sure the final
algorithm used for the hash calculation will be documented in the code.
As I did things originally, the dwarf_output::attr_value::reference
method
(really dwarf_data::attr_value::reference) is not meant to be used on
something that isn't really final and placed.
I have to admit that I find this slightly confusing, since it seems to
depend on which template instance is being used whether or not
reference() works or not. Although I now see why that would be in the
implementation. It kind of breaks the interface abstraction.
The way you got to
std::logic_error ("can't happen!") was because you used this method on an
attr_value whose _m_value pointer was a copier::entry rather than a
value_reference. If you look at pending_dwarf::attr_value::reference,
you'll see that it checks for this case specially:
const entry *ref = dynamic_cast<const entry *> (_m_value);
This returns NULL if _m_value is not a copier::entry. In that case, it
really is a finalized reference attribute, or else it's a special
circular_reference object.
if (ref == NULL)
// Either really a final reference, or a circular reference.
return typename debug_info_entry::const_pointer
(dynamic_cast<const value::value_reference *> (_m_value));
That's pending_dwarf::debug_info_entry::const_pointer, which is another
name for pending_dwarf::debug_info_entry::children_type::const_iterator.
This has overloaded constructor methods, one of which takes a
value_reference pointer. That's the one that calls init_from_ref.
Yeah, I actually had wanted to do a trick like that in the
attributes_type do_hash () function, but I didn't know how to get at the
_m_value of the attr_value there. Which is also why I ended up using
what_space to detect whether the attribute was a reference value or not.
When in fact it is a copier::entry, we instead get here:
/* This is an attribute comparison inside the attrs_match
comparator. The attribute sets passed to attrs_match
directly don't hit this--they've already been finalized.
But following those references we got to another
pending_entry and its attributes that are not yet
finalized. If attrs_match winds up returning true, these
will never be finalized because they are duplicates. */
return typename debug_info_entry::const_pointer
(const_cast<entry *> (ref));
That calls the different overload for that constructor, the one that takes
an entry pointer.
Either way, we get a pending_dwarf::debug_info_entry::const_pointer object.
This is not the same thing as a dwarf_output::debug_info_entry::const_pointer.
I admit that my c++ typing knowledge fails me again in this example.
How can it be that this template variant of reference () returns ...
while the dwarf_data::attr_value::reference method is defined as
inline const die_ptr &reference () const with typedef typename
impl::debug_info_entry::pointer die_ptr; Since impl is dwarf_output, it
seems somewhat curious that this method is allowed to change the type of
the return value. Again this kind of breaks my vision of the abstraction
interface.
[...]
So I don't think that really answered the question of how it should be now,
but that is the pieces of how it is now that I think you were missing.
No it didn't directly answer my question of "how the hell do I trick c++
into doing what I want in this case", but it does show me I should
concentrate on moving all (local) hashing logic into the (pending) entry
which can rely on the input die tree and stay as far away from anything
that could become part of pending_dwarf as I can for now :)
Thanks,
Mark