From 404d5b985c24d2a15d1908229456e6a8663b1ff3 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 | 9 +++++---- gcc/hash-table.h | 4 ++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/gcc/hash-map-tests.c b/gcc/hash-map-tests.c index 3c79a13c1a8..d49eafa0bb9 100644 --- a/gcc/hash-map-tests.c +++ b/gcc/hash-map-tests.c @@ -304,31 +304,31 @@ 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 'hash_table::expand'. */ +/* Verify that 'hash_table::expand' doesn't leak Value objects. */ static void test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline) { typedef hash_map Map; /* Note that we are starting with a fresh 'Map'. Even if an existing one has been cleared out completely, there remain 'deleted' elements, and these would disturb the following logic, where we don't have access to the actual 'm_n_deleted' value. */ size_t m_n_deleted = 0; const int N_elem = 1234; void *a[N_elem]; for (int i = 0; i < N_elem; ++i) @@ -368,68 +368,69 @@ 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', but does influence any following via 'm_n_deleted'. */ 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); } } 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