This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)


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;


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]