From 2275962abd72acd1636d438d1cb176bbabdcef06 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Fri, 13 Aug 2021 18:03:38 +0200
Subject: [PATCH] Fix 'hash_table::expand' to destruct stale Value objects
Thus plugging potentional memory leaks if these have non-trivial
constructor/destructor.
See
<https://stackoverflow.com/questions/6730403/how-to-delete-object-constructed-via-placement-new-operator>
and others.
As one example, compilation of 'g++.dg/warn/Wmismatched-tags.C' per
'valgrind --leak-check=full' improves as follows:
[...]
-
-104 bytes in 1 blocks are definitely lost in loss record 399 of 519
- at 0x483DFAF: realloc (vg_replace_malloc.c:836)
- by 0x223B62C: xrealloc (xmalloc.c:179)
- by 0xA8D848: void va_heap::reserve<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
- by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1858)
- by 0xA8B277: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::safe_push(class_decl_loc_t::class_key_loc_t const&) (vec.h:1967)
- by 0xA57481: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32967)
- by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941)
- by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819)
- by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227)
- by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942)
- by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517)
- by 0xA43C71: cp_parser_parameter_declaration(cp_parser*, int, bool, bool*) (parser.c:24242)
-
-168 bytes in 3 blocks are definitely lost in loss record 422 of 519
- at 0x483DFAF: realloc (vg_replace_malloc.c:836)
- by 0x223B62C: xrealloc (xmalloc.c:179)
- by 0xA8D848: void va_heap::reserve<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
- by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1858)
- by 0xA8B277: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::safe_push(class_decl_loc_t::class_key_loc_t const&) (vec.h:1967)
- by 0xA57481: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32967)
- by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941)
- by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819)
- by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227)
- by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942)
- by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517)
- by 0xA53385: cp_parser_single_declaration(cp_parser*, vec<deferred_access_check, va_gc, vl_embed>*, bool, bool, bool*) (parser.c:31072)
-
-488 bytes in 7 blocks are definitely lost in loss record 449 of 519
- at 0x483DFAF: realloc (vg_replace_malloc.c:836)
- by 0x223B62C: xrealloc (xmalloc.c:179)
- by 0xA8D848: void va_heap::reserve<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
- by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1858)
- by 0xA8B277: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::safe_push(class_decl_loc_t::class_key_loc_t const&) (vec.h:1967)
- by 0xA57481: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32967)
- by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941)
- by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819)
- by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227)
- by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942)
- by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517)
- by 0xA49508: cp_parser_member_declaration(cp_parser*) (parser.c:26440)
-
-728 bytes in 7 blocks are definitely lost in loss record 455 of 519
- at 0x483B7F3: malloc (vg_replace_malloc.c:309)
- by 0x223B63F: xrealloc (xmalloc.c:177)
- by 0xA8D848: void va_heap::reserve<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
- by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1858)
- by 0xA57508: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32980)
- by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941)
- by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819)
- by 0xA48BC6: cp_parser_class_head(cp_parser*, bool*) (parser.c:26090)
- by 0xA4674B: cp_parser_class_specifier_1(cp_parser*) (parser.c:25302)
- by 0xA47D76: cp_parser_class_specifier(cp_parser*) (parser.c:25680)
- by 0xA37E27: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18912)
- by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517)
-
-832 bytes in 8 blocks are definitely lost in loss record 458 of 519
- at 0x483B7F3: malloc (vg_replace_malloc.c:309)
- by 0x223B63F: xrealloc (xmalloc.c:177)
- by 0xA8D848: void va_heap::reserve<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
- by 0xA901ED: bool vec_safe_reserve<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:697)
- by 0xA8F161: void vec_alloc<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int) (vec.h:718)
- by 0xA8D18D: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>::copy() const (vec.h:979)
- by 0xA8B0C3: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::copy() const (vec.h:1824)
- by 0xA896B1: class_decl_loc_t::operator=(class_decl_loc_t const&) (parser.c:32697)
- by 0xA571FD: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32899)
- by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819)
- by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227)
- by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942)
-
-1,144 bytes in 11 blocks are definitely lost in loss record 466 of 519
- at 0x483B7F3: malloc (vg_replace_malloc.c:309)
- by 0x223B63F: xrealloc (xmalloc.c:177)
- by 0xA8D848: void va_heap::reserve<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
- by 0xA901ED: bool vec_safe_reserve<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:697)
- by 0xA8F161: void vec_alloc<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int) (vec.h:718)
- by 0xA8D18D: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>::copy() const (vec.h:979)
- by 0xA8B0C3: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::copy() const (vec.h:1824)
- by 0xA896B1: class_decl_loc_t::operator=(class_decl_loc_t const&) (parser.c:32697)
- by 0xA571FD: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32899)
- by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819)
- by 0xA48BC6: cp_parser_class_head(cp_parser*, bool*) (parser.c:26090)
- by 0xA4674B: cp_parser_class_specifier_1(cp_parser*) (parser.c:25302)
-
-1,376 bytes in 10 blocks are definitely lost in loss record 467 of 519
- at 0x483DFAF: realloc (vg_replace_malloc.c:836)
- by 0x223B62C: xrealloc (xmalloc.c:179)
- by 0xA8D848: void va_heap::reserve<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
- by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1858)
- by 0xA8B277: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::safe_push(class_decl_loc_t::class_key_loc_t const&) (vec.h:1967)
- by 0xA57481: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32967)
- by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941)
- by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819)
- by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227)
- by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942)
- by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517)
- by 0xA301E0: cp_parser_simple_declaration(cp_parser*, bool, tree_node**) (parser.c:14772)
-
-3,552 bytes in 33 blocks are definitely lost in loss record 483 of 519
- at 0x483B7F3: malloc (vg_replace_malloc.c:309)
- by 0x223B63F: xrealloc (xmalloc.c:177)
- by 0xA8D848: void va_heap::reserve<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
- by 0xA901ED: bool vec_safe_reserve<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:697)
- by 0xA8F161: void vec_alloc<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int) (vec.h:718)
- by 0xA8D18D: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>::copy() const (vec.h:979)
- by 0xA8B0C3: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::copy() const (vec.h:1824)
- by 0xA8964A: class_decl_loc_t::class_decl_loc_t(class_decl_loc_t const&) (parser.c:32689)
- by 0xA8F515: hash_table<hash_map<tree_decl_hash, class_decl_loc_t, simple_hashmap_traits<default_hash_traits<tree_decl_hash>, class_decl_loc_t> >::hash_entry, false, xcallocator>::expand() (hash-table.h:839)
- by 0xA8D4B3: hash_table<hash_map<tree_decl_hash, class_decl_loc_t, simple_hashmap_traits<default_hash_traits<tree_decl_hash>, class_decl_loc_t> >::hash_entry, false, xcallocator>::find_slot_with_hash(tree_node* const&, unsigned int, insert_option) (hash-table.h:1008)
- by 0xA8B1DC: hash_map<tree_decl_hash, class_decl_loc_t, simple_hashmap_traits<default_hash_traits<tree_decl_hash>, class_decl_loc_t> >::get_or_insert(tree_node* const&, bool*) (hash-map.h:200)
- by 0xA57128: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32888)
[...]
LEAK SUMMARY:
- definitely lost: 8,440 bytes in 81 blocks
+ definitely lost: 48 bytes in 1 blocks
indirectly lost: 12,529 bytes in 329 blocks
possibly lost: 0 bytes in 0 blocks
still reachable: 1,644,376 bytes in 768 blocks
gcc/
* hash-table.h (hash_table<Descriptor, Lazy, Allocator>::expand):
Destruct stale Value objects.
* hash-map-tests.c (test_map_of_type_with_ctor_and_dtor_expand):
Update.
---
gcc/hash-map-tests.c | 10 ++++++----
gcc/hash-table.h | 4 ++++
2 files changed, 10 insertions(+), 4 deletions(-)
@@ -305,31 +305,32 @@ test_map_of_type_with_ctor_and_dtor ()
m.get_or_insert (a[i]);
ASSERT_EQ (val_t::ndefault, 1 + i);
ASSERT_EQ (val_t::ncopy, 0);
ASSERT_EQ (val_t::nassign, 0);
ASSERT_EQ (val_t::ndtor, i);
m.remove (a[i]);
ASSERT_EQ (val_t::ndefault, 1 + i);
ASSERT_EQ (val_t::ncopy, 0);
ASSERT_EQ (val_t::nassign, 0);
ASSERT_EQ (val_t::ndtor, 1 + i);
}
}
}
-/* Verify aspects of 'hash_table::expand'. */
+/* Verify aspects of 'hash_table::expand', in particular that it doesn't leak
+ Value objects. */
static void
test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline)
{
/* Configure, so that hash table expansion triggers a few times. */
const size_t N_init = 0;
const int N_elem = 70;
size_t expand_c_expected = 4;
size_t expand_c = 0;
/* For stability of this testing, we need all Key values 'k' to produce
unique hash values 'Traits::hash (k)', as otherwise the dynamic
insert/remove behavior may diverge across different architectures. This
is, for example, a problem when using the standard 'pointer_hash::hash',
which is simply doing a 'k >> 3' operation, which is fine on 64-bit
@@ -388,69 +389,70 @@ test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline)
unsigned int nindex = hash_table_higher_prime_index (elts * 2);
m_size = prime_tab[nindex].prime;
}
m_n_deleted = 0;
/* All non-deleted elements have been moved. */
n_expand_moved += i;
if (remove_some_inline)
n_expand_moved -= (i + 2) / 3;
}
ASSERT_EQ (val_t::ndefault, 1 + i);
ASSERT_EQ (val_t::ncopy, n_expand_moved);
ASSERT_EQ (val_t::nassign, 0);
if (remove_some_inline)
- ASSERT_EQ (val_t::ndtor, (i + 2) / 3);
+ ASSERT_EQ (val_t::ndtor, n_expand_moved + (i + 2) / 3);
else
- ASSERT_EQ (val_t::ndtor, 0);
+ ASSERT_EQ (val_t::ndtor, n_expand_moved);
/* Remove some inline. This never triggers an 'expand' here, but via
'm_n_deleted' does influence any following one. */
if (remove_some_inline
&& !(i % 3))
{
m.remove (a[i]);
/* Per 'hash_table::remove_elt_with_hash'. */
m_n_deleted++;
ASSERT_EQ (val_t::ndefault, 1 + i);
ASSERT_EQ (val_t::ncopy, n_expand_moved);
ASSERT_EQ (val_t::nassign, 0);
- ASSERT_EQ (val_t::ndtor, 1 + (i + 2) / 3);
+ ASSERT_EQ (val_t::ndtor, n_expand_moved + 1 + (i + 2) / 3);
}
}
ASSERT_EQ (expand_c, expand_c_expected);
int ndefault = val_t::ndefault;
int ncopy = val_t::ncopy;
int nassign = val_t::nassign;
int ndtor = val_t::ndtor;
for (int i = 0; i < N_elem; ++i)
{
if (remove_some_inline
&& !(i % 3))
continue;
m.remove (a[i]);
++ndtor;
ASSERT_EQ (val_t::ndefault, ndefault);
ASSERT_EQ (val_t::ncopy, ncopy);
ASSERT_EQ (val_t::nassign, nassign);
ASSERT_EQ (val_t::ndtor, ndtor);
}
+ ASSERT_EQ (val_t::ndefault + val_t::ncopy, val_t::ndtor);
}
/* Test calling empty on a hash_map that has a key type with non-zero
"empty" value. */
static void
test_nonzero_empty_key ()
{
typedef int_hash<int, INT_MIN, INT_MAX> IntHash;
hash_map<int, int, simple_hashmap_traits<IntHash, int> > x;
for (int i = 1; i != 32; ++i)
x.put (i, i);
ASSERT_EQ (x.get (0), NULL);
@@ -808,30 +808,34 @@ hash_table<Descriptor, Lazy, Allocator>::expand ()
m_entries = nentries;
m_size = nsize;
m_size_prime_index = nindex;
m_n_elements -= m_n_deleted;
m_n_deleted = 0;
value_type *p = oentries;
do
{
value_type &x = *p;
if (!is_empty (x) && !is_deleted (x))
{
value_type *q = find_empty_slot_for_expand (Descriptor::hash (x));
new ((void*) q) value_type (std::move (x));
+
+ /* Manually invoke destructor of original object, to counterbalance
+ object constructed via placement new. */
+ x.~value_type ();
}
p++;
}
while (p < olimit);
if (!m_ggc)
Allocator <value_type> ::data_free (oentries);
else
ggc_free (oentries);
}
/* Implements empty() in cases where it isn't a no-op. */
template<typename Descriptor, bool Lazy,
--
2.30.2