This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
- From: Martin Sebor <msebor at gmail dot com>
- To: Jan Hubicka <hubicka at ucw dot cz>
- Cc: Jason Merrill <jason at redhat dot com>, Pedro Alves <palves at redhat dot com>, Gcc Patch List <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 29 Jun 2017 18:16:23 -0600
- Subject: Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
- Authentication-results: sourceware.org; auth=none
- References: <email@example.com> <firstname.lastname@example.org> <CADzB+2=5ZD4YkENJ65bn-Q-d1RXgn3nGKoFQE+pgja_KPNO1xw@mail.gmail.com> <email@example.com> <20170629161506.GA25586@atrey.karlin.mff.cuni.cz> <firstname.lastname@example.org> <20170629223408.GC25586@atrey.karlin.mff.cuni.cz>
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
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>
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
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>
pointer_hash <Type>::is_empty (Type *e)
return e == NULL;
and mark_empty as
template <typename Type>
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
I guess they are supposed to be definable to other implementations
but then the former memset code would break.
Right, that's my concern.
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.
PS Does this help at all?
@@ -804,8 +804,8 @@ hash_table<Descriptor, Allocator>::empty_slow ()
- 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)
m_n_deleted = 0;
m_n_elements = 0;