On 06/29/2017 04:34 PM, Jan Hubicka wrote:
The warning in the original code could have been suppressed (by
casting the pointer to char*), but it was valid so I opted not
to. I'd expect it to be possible to work around the bug but
I don't have easy access to GCC 4.2 to reproduce it or verify
the fix.
FWIW, after looking at the function again, I wondered if zeroing
out the elements (either way) was the right thing to do and if
they shouldn't be cleared by calling Descriptor::mark_empty()
instead, like in alloc_entries(), but making that change broke
a bunch of ipa/ipa-pta-*.c tests. It's not really clear to me
what this code is supposed to do.
Well, it is a standard hash table.
What I mean is that while the true branch of the if (nsize != size)
statement in hash_table::empty_slow () frees the table and then
allocates a new one which marks each element in the newly allocated
storage as empty by calling mark_empty() (i.e., Descriptor::
mark_empty()), while the false branch bypasses mark_empty() and
calls memset(entries, 0, ...) instead. The two are equivalent
only when Descriptor::mark_empty(value_type &e) sets e to all
zeros. They are not equivalent when Descriptor::mark_empty()
does something else. When value_type is a pointer they should
be the equivalent (so long as a null pointer is all zeros), but
it's not obvious to me what value_type is in your case (tree?)
In any case, calling memset bypasses the (undocumented)
customization point (Descriptor::mark_empty). Calling it
conditionally depending on the size of the hash table seems
suspicious (as does setting *entry = value_type();). If there
is a hash table instance where mark_empty is not equivalent to
memset(entries, 0, ...) it could mean a bug.
Looking at some other definitions of mark_empty I see int_hash
defines it this way:
template <typename Type, Type Empty, Type Deleted>
inline void
int_hash <Type, Empty, Deleted>::mark_empty (Type &x)
{
x = Empty;
}
So for hash_tables with int value_types the memset is only
equivalent to calling mark_empty when Empty is zero. There
are instances of hash tables in GCC like alias_set_hash and
uid_hash where Empty is non-zero. I wonder what happens when
hash_table::empty_slow() is called on one of these instances
with a small number of elements and that instance is used
again.
The problem I hit was
lookup_with_hash walking infinitly around the hash table because
all elements seemed used.
is_empty is defined as:
template <typename Type>
inline bool
pointer_hash <Type>::is_empty (Type *e)
{
return e == NULL;
}
and mark_empty as
template <typename Type>
inline void
pointer_hash <Type>::mark_empty (Type *&e)
{
e = NULL;
}
It sounds like value_type is a pointer in your case and
the assignment '*entry = value_type();' isn't clearing the *entry.
I vaguely remember coming across a bug like that years ago but I
don't know if that was GCC or some other compiler. I tried
compiling a small test case with a few GCC revisions close to
4.2 (r117926 and some others) but couldn't reproduce anything
unexpected.
I guess they are supposed to be definable to other implementations
but then the former memset code would break.
Right, that's my concern.
Martin
PS the below is just a shot in the dark. I'd be surprised if it
actually did something for you, but with little else to go on
it's worth a try.
Martin
PS Does this help at all?
@@ -804,8 +804,8 @@ hash_table<Descriptor, Allocator>::empty_slow ()
}
else
{
- for ( ; size; ++entries, --size)
- *entries = value_type ();
+ for (size_t i = 0; i != size; ++i)
+ entries[i] = value_type ();
I can give it a try tomorrow. Still wonder what goes wrong with ctors with 4.2
(and 4.3 as well apparently)
Honza
}
m_n_deleted = 0;
m_n_elements = 0;
