From 2275962abd72acd1636d438d1cb176bbabdcef06 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge 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 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(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::reserve(unsigned int, bool) (vec.h:1858) - by 0xA8B277: vec::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(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::reserve(unsigned int, bool) (vec.h:1858) - by 0xA8B277: vec::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*, 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(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::reserve(unsigned int, bool) (vec.h:1858) - by 0xA8B277: vec::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(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::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(vec*&, unsigned int, bool) (vec.h:290) - by 0xA901ED: bool vec_safe_reserve(vec*&, unsigned int, bool) (vec.h:697) - by 0xA8F161: void vec_alloc(vec*&, unsigned int) (vec.h:718) - by 0xA8D18D: vec::copy() const (vec.h:979) - by 0xA8B0C3: vec::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(vec*&, unsigned int, bool) (vec.h:290) - by 0xA901ED: bool vec_safe_reserve(vec*&, unsigned int, bool) (vec.h:697) - by 0xA8F161: void vec_alloc(vec*&, unsigned int) (vec.h:718) - by 0xA8D18D: vec::copy() const (vec.h:979) - by 0xA8B0C3: vec::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(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::reserve(unsigned int, bool) (vec.h:1858) - by 0xA8B277: vec::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(vec*&, unsigned int, bool) (vec.h:290) - by 0xA901ED: bool vec_safe_reserve(vec*&, unsigned int, bool) (vec.h:697) - by 0xA8F161: void vec_alloc(vec*&, unsigned int) (vec.h:718) - by 0xA8D18D: vec::copy() const (vec.h:979) - by 0xA8B0C3: vec::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, class_decl_loc_t> >::hash_entry, false, xcallocator>::expand() (hash-table.h:839) - by 0xA8D4B3: hash_table, 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, 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::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(-) diff --git a/gcc/hash-map-tests.c b/gcc/hash-map-tests.c index 6acc0d4337e..511d4342087 100644 --- a/gcc/hash-map-tests.c +++ b/gcc/hash-map-tests.c @@ -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 IntHash; hash_map > x; for (int i = 1; i != 32; ++i) x.put (i, i); ASSERT_EQ (x.get (0), NULL); diff --git a/gcc/hash-table.h b/gcc/hash-table.h index a6e0ac8eea9..8c2a0589fbd 100644 --- a/gcc/hash-table.h +++ b/gcc/hash-table.h @@ -808,30 +808,34 @@ hash_table::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 ::data_free (oentries); else ggc_free (oentries); } /* Implements empty() in cases where it isn't a no-op. */ template