| Message ID | 20240625080332.1517736-2-j@lambda.is |
|---|---|
| State | New |
| Headers | show |
| Series | Condition coverage docs, bugfix | expand |
> The value vec objects are destroyed on exit, but release still needs to > be called explicitly. > > gcc/ChangeLog: > > * tree-profile.cc (find_conditions): Release vectors before > return. I wonder if you turn hash_map<int_hash<unsigned, 0>, vec<basic_block>> exprs; to hash_map<int_hash<unsigned, 0>, auto_vec<basic_block>> exprs; Won't hash_map destructor take care of this by itself? Honza > --- > gcc/tree-profile.cc | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc > index e4bb689cef5..18f48e8d04e 100644 > --- a/gcc/tree-profile.cc > +++ b/gcc/tree-profile.cc > @@ -919,6 +919,9 @@ find_conditions (struct function *fn) > if (!have_post_dom) > free_dominance_info (fn, CDI_POST_DOMINATORS); > > + for (auto expr : exprs) > + expr.second.release (); > + > cov->m_masks.safe_grow_cleared (2 * cov->m_index.last ()); > const size_t length = cov_length (cov); > for (size_t i = 0; i != length; i++) > -- > 2.39.2 >
On 6/25/24 12:23, Jan Hubicka wrote: >> The value vec objects are destroyed on exit, but release still needs to >> be called explicitly. >> >> gcc/ChangeLog: >> >> * tree-profile.cc (find_conditions): Release vectors before >> return. > I wonder if you turn > hash_map<int_hash<unsigned, 0>, vec<basic_block>> exprs; > to > hash_map<int_hash<unsigned, 0>, auto_vec<basic_block>> exprs; > Won't hash_map destructor take care of this by itself? It does, actually - I think I tried something to that effect at one point and the auto_vec's non-copy semantics got in the way. Apparently I either misremember trying, or I did something differently at the time. auto_vec is much nicer, of course, I'll update the patch. Thanks! > > Honza >> --- >> gcc/tree-profile.cc | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc >> index e4bb689cef5..18f48e8d04e 100644 >> --- a/gcc/tree-profile.cc >> +++ b/gcc/tree-profile.cc >> @@ -919,6 +919,9 @@ find_conditions (struct function *fn) >> if (!have_post_dom) >> free_dominance_info (fn, CDI_POST_DOMINATORS); >> >> + for (auto expr : exprs) >> + expr.second.release (); >> + >> cov->m_masks.safe_grow_cleared (2 * cov->m_index.last ()); >> const size_t length = cov_length (cov); >> for (size_t i = 0; i != length; i++) >> -- >> 2.39.2 >>
On 6/25/24 12:23, Jan Hubicka wrote: >> The value vec objects are destroyed on exit, but release still needs to >> be called explicitly. >> >> gcc/ChangeLog: >> >> * tree-profile.cc (find_conditions): Release vectors before >> return. > I wonder if you turn > hash_map<int_hash<unsigned, 0>, vec<basic_block>> exprs; > to > hash_map<int_hash<unsigned, 0>, auto_vec<basic_block>> exprs; > Won't hash_map destructor take care of this by itself? > > Honza I updated this to use auto_vec and pushed it. Thanks, Jørgen >> --- >> gcc/tree-profile.cc | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc >> index e4bb689cef5..18f48e8d04e 100644 >> --- a/gcc/tree-profile.cc >> +++ b/gcc/tree-profile.cc >> @@ -919,6 +919,9 @@ find_conditions (struct function *fn) >> if (!have_post_dom) >> free_dominance_info (fn, CDI_POST_DOMINATORS); >> >> + for (auto expr : exprs) >> + expr.second.release (); >> + >> cov->m_masks.safe_grow_cleared (2 * cov->m_index.last ()); >> const size_t length = cov_length (cov); >> for (size_t i = 0; i != length; i++) >> -- >> 2.39.2 >>
I think we need to revert this. I got this email from linaro/gcc-regressions: [Linaro-TCWG-CI] gcc-15-1649-g19f630e6ae8d: FAIL: 2 regressions on aarch64 regressions.sum: === gcc tests === Running gcc:gcc.misc-tests/gcov.exp ... FAIL: gcc.misc-tests/gcov-23.c (internal compiler error: in operator[], at vec.h:910) FAIL: gcc.misc-tests/gcov-23.c (test for excess errors) This did not reproduce on my machine, but I took a quick look at the hash-map implementation. hash_map.put calls hash_table.find_slot_with_hash, which calls hash_table.expand, which does move+destroy. auto_vec is not really move-aware which leads to a double-free. The fix is either to make auto_vec move-aware (and more like C++'s std::vector) or revert this patch and apply the original version with an explicit release. OK? Thanks, Jørgen On 6/25/24 12:23, Jan Hubicka wrote: >> The value vec objects are destroyed on exit, but release still needs to >> be called explicitly. >> >> gcc/ChangeLog: >> >> * tree-profile.cc (find_conditions): Release vectors before >> return. > I wonder if you turn > hash_map<int_hash<unsigned, 0>, vec<basic_block>> exprs; > to > hash_map<int_hash<unsigned, 0>, auto_vec<basic_block>> exprs; > Won't hash_map destructor take care of this by itself? > > Honza >> --- >> gcc/tree-profile.cc | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc >> index e4bb689cef5..18f48e8d04e 100644 >> --- a/gcc/tree-profile.cc >> +++ b/gcc/tree-profile.cc >> @@ -919,6 +919,9 @@ find_conditions (struct function *fn) >> if (!have_post_dom) >> free_dominance_info (fn, CDI_POST_DOMINATORS); >> >> + for (auto expr : exprs) >> + expr.second.release (); >> + >> cov->m_masks.safe_grow_cleared (2 * cov->m_index.last ()); >> const size_t length = cov_length (cov); >> for (size_t i = 0; i != length; i++) >> -- >> 2.39.2 >>
diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc index e4bb689cef5..18f48e8d04e 100644 --- a/gcc/tree-profile.cc +++ b/gcc/tree-profile.cc @@ -919,6 +919,9 @@ find_conditions (struct function *fn) if (!have_post_dom) free_dominance_info (fn, CDI_POST_DOMINATORS); + for (auto expr : exprs) + expr.second.release (); + cov->m_masks.safe_grow_cleared (2 * cov->m_index.last ()); const size_t length = cov_length (cov); for (size_t i = 0; i != length; i++)