Re: [PATCH] cex: fix leaks

[Akim Demaille]

Hi Vincent,

> Le 17 mai 2020 à 16:45, Akim Demaille <address@hidden> a écrit :
> 
>> Le 17 mai 2020 à 16:31, Akim Demaille <address@hidden> a écrit :
>> 
>> Your patch improves things: we go from 
>> 
>> SUMMARY: AddressSanitizer: 262960 byte(s) leaked in 1874 allocation(s).
>> 
>> to
>> 
>> SUMMARY: AddressSanitizer: 187064 byte(s) leaked in 779 allocation(s).
>> 
>> The following commit reduces it further to
>> 
>> SUMMARY: AddressSanitizer: 184968 byte(s) leaked in 648 allocation(s).
> 
> The following one reaches
> 
> SUMMARY: AddressSanitizer: 11352 byte(s) leaked in 263 allocation(s).
> 
> after addressing a very simple mistake.

Damnit...  The last one was right under my nose, yet it took me a long
time to spot it:

@@ -261,7 +262,7 @@ init_prods (void)
        = hash_initialize (nsyms - ntokens, NULL,
                           (Hash_hasher) hash_pair_hasher,
                           (Hash_comparator) hash_pair_comparator,
-                           NULL);
+                           (Hash_data_freer) hash_pair_free);

      // Add the nitems of state to skip to the production portion
      // of that state's state_items

Then we reach the 0 leak when there are no conflicts.
(Except for the libtextstyle issue).

BTW, why are you using a hash table here?  Aren't these guys just
regular integer-indexed arrays?  I suppose the API would be much
simpler and more natural.  And probably faster.  In fact, bitsetv
might do the trick.

Unfortunately we still have plenty of leaks when there are conflicts.
See the attachment for an example.

Cheers!

testsuite.log
Description: Binary data

Some of them seem to be simple, but I believe you are in the best
position to track these guys down.

commit 14ce40ed766e7bb3eada3e8a08288b288a365886
Author: Akim Demaille <address@hidden>
Date:   Sun May 17 17:49:52 2020 +0200

   cex: fix memory leaks when there are conflicts

   * src/counterexample.c (production_step, reduction_step): Release
   memory of temporary objects.

diff --git a/src/counterexample.c b/src/counterexample.c
index d1a9ef5b..d3acf47a 100644
--- a/src/counterexample.c
+++ b/src/counterexample.c
@@ -728,6 +728,7 @@ production_step (search_state *ss, int parser_state)
      copy->complexity = complexity;
      ssb_append (copy);
    }
+  gl_list_free (prods);
}

static inline int
@@ -776,6 +777,7 @@ reduction_step (search_state *ss, const item_number 
*conflict_item,
      copy->complexity += r_cost + PRODUCTION_COST + 2 * SHIFT_COST;
      gl_list_add_last (result, copy);
    }
+  gl_list_free (reduced);
  return result;
}




Cheers!



commit 5ccbaa5380e74309526c6daff0b2a6df4d390749
Author: Akim Demaille <address@hidden>
Date:   Sun May 17 17:22:29 2020 +0200

   cex: be sure to always reclaim memory put in hashes

   One call to hash_initialize did not provide a function to free memory.

   * src/state-item.c (hash_pair_table_create): New.
   Use it.

diff --git a/src/state-item.c b/src/state-item.c
index 76c0b4f2..d3d48ec2 100644
--- a/src/state-item.c
+++ b/src/state-item.c
@@ -65,6 +65,16 @@ hash_pair_free (hash_pair *hp)
  free (hp);
}

+Hash_table *
+hash_pair_table_create (int size)
+{
+  return hash_xinitialize (size,
+                           NULL,
+                           (Hash_hasher) hash_pair_hasher,
+                           (Hash_comparator) hash_pair_comparator,
+                           (Hash_data_freer) hash_pair_free);
+}
+
static bitset
hash_pair_lookup (Hash_table *tab, int key)
{
@@ -81,6 +91,7 @@ hash_pair_insert (Hash_table *tab, int key, bitset val)
  hp->key = key;
  hp->l = val;
  hash_pair *res = hash_xinsert (tab, hp);
+  // This must be the first insertion.
  assert (res == hp);
}

@@ -206,8 +217,8 @@ init_trans (void)
      state *s = states[i];
      transitions *t = s->transitions;
      Hash_table *transition_set
-        = hash_initialize (t->num, NULL, (Hash_hasher) state_sym_hasher,
-                           (Hash_comparator) state_sym_comparator, NULL);
+        = hash_xinitialize (t->num, NULL, (Hash_hasher) state_sym_hasher,
+                            (Hash_comparator) state_sym_comparator, NULL);
      for (int j = 0; j < t->num; ++j)
        if (!TRANSITION_IS_DISABLED (t, j))
          hash_xinsert (transition_set, t->states[j]);
@@ -247,21 +258,13 @@ si_prods_lookup (state_item_number si)
static void
init_prods (void)
{
-  si_prods = hash_initialize (nstate_items,
-                              NULL,
-                              (Hash_hasher) hash_pair_hasher,
-                              (Hash_comparator) hash_pair_comparator,
-                              (Hash_data_freer) hash_pair_free);
+  si_prods = hash_pair_table_create (nstate_items);
  for (int i = 0; i < nstates; ++i)
    {
      state *s = states[i];
      // closure_map is a hash map from nonterminals to a set
      // of the items that produce those nonterminals
-      Hash_table *closure_map
-        = hash_initialize (nsyms - ntokens, NULL,
-                           (Hash_hasher) hash_pair_hasher,
-                           (Hash_comparator) hash_pair_comparator,
-                           NULL);
+      Hash_table *closure_map = hash_pair_table_create (nsyms - ntokens);

      // Add the nitems of state to skip to the production portion
      // of that state's state_items
@@ -377,8 +380,7 @@ init_firsts (void)
          symbol_number lhs = r->lhs->number;
          bitset f_lhs = FIRSTS (lhs);
          for (item_number *n = r->rhs;
-               item_number_is_symbol_number (*n) &&
-                 ISVAR (*n);
+               item_number_is_symbol_number (*n) && ISVAR (*n);
               ++n)
            {
              bitset f = FIRSTS (*n);