Re: [0/18] new argv_ref branch for m4 speedup

[Ralf Wildenhues]

With 16000 arguments in the m4_join example, valgrind gives me these
errors, the first of which is unique to large N, but the latter two
happen with small N as well:

==22831== 1 errors in context 1 of 3:
==22831== Invalid read of size 1
==22831==    at 0x4135D5: arg_adjust_refcount (macro.c:804)
==22831==    by 0x412FA6: expand_macro (macro.c:716)
==22831==    by 0x41142F: expand_token (macro.c:312)
==22831==    by 0x411037: expand_input (macro.c:219)
==22831==    by 0x402CAD: process_file (m4.c:307)
==22831==    by 0x4031DE: main (m4.c:568)
==22831==  Address 0x56622fc is 128,132 bytes inside a block of size 129,688 
free'd
==22831==    at 0x4A1AA65: free (vg_replace_malloc.c:320)
==22831==    by 0x4B8F57A: obstack_free (in /lib/libc-2.3.6.so)
==22831==    by 0x41342F: adjust_refcount (macro.c:750)
==22831==    by 0x4135C8: arg_adjust_refcount (macro.c:803)
==22831==    by 0x412FA6: expand_macro (macro.c:716)
==22831==    by 0x41142F: expand_token (macro.c:312)
==22831==    by 0x411037: expand_input (macro.c:219)
==22831==    by 0x402CAD: process_file (m4.c:307)
==22831==    by 0x4031DE: main (m4.c:568)
==22831==
==22831== 12047 errors in context 2 of 3:
==22831== Conditional jump or move depends on uninitialised value(s)
==22831==    at 0x40E984: append_quote_token (input.c:997)
==22831==    by 0x410AB3: next_token (input.c:1639)
==22831==    by 0x411558: expand_argument (macro.c:368)
==22831==    by 0x41204B: collect_arguments (macro.c:533)
==22831==    by 0x412D7C: expand_macro (macro.c:682)
==22831==    by 0x41142F: expand_token (macro.c:312)
==22831==    by 0x411037: expand_input (macro.c:219)
==22831==    by 0x402CAD: process_file (m4.c:307)
==22831==    by 0x4031DE: main (m4.c:568)
==22831==
==22831== 52628 errors in context 3 of 3:
==22831== Conditional jump or move depends on uninitialised value(s)
==22831==    at 0x40D989: pop_input (input.c:575)
==22831==    by 0x40C23F: push_string_init (input.c:272)
==22831==    by 0x412ECB: expand_macro (macro.c:694)
==22831==    by 0x41142F: expand_token (macro.c:312)
==22831==    by 0x411037: expand_input (macro.c:219)
==22831==    by 0x402CAD: process_file (m4.c:307)
==22831==    by 0x4031DE: main (m4.c:568)

They seem to be fixed by the patches below.

I optimistically assumed the argv would point to storage freed just
before inside adjust_refcount. and I'm not sure about the third patch
but it makes the warning go away.

Incidentally, these three changes also fix the SEGV for 32000 items.
:-)

Cheers,
Ralf

        * src/macro.c (arg_adjust_refcount): Avoid reading argv memory
        after freeing it in adjust_refcount.

diff --git a/src/macro.c b/src/macro.c
index 012fdd2..de28d41 100644
--- a/src/macro.c
+++ b/src/macro.c
@@ -770,6 +770,7 @@ arg_adjust_refcount (macro_arguments *argv, bool increase)
   size_t i;
   token_chain *chain;
   bool result = false;
+  bool inuse;
 
   if (!argv->has_ref)
     result = true;
@@ -800,8 +801,10 @@ arg_adjust_refcount (macro_arguments *argv, bool increase)
        else
          result = true;
     }
+
+  inuse = argv->inuse;
   adjust_refcount (argv->level, increase);
-  return result && !argv->inuse;
+  return result && !inuse;
 }
 
 

        Avoid referencing uninitialized part of union.
        * src/input.c (append_quote_token): Reorder logic to avoid
        reading string part of token_chain if argv_ref.

diff --git a/src/input.c b/src/input.c
index 6950d5b..3082a06 100644
--- a/src/input.c
+++ b/src/input.c
@@ -994,7 +994,8 @@ append_quote_token (struct obstack *obs, token_data *td)
   /* Speed consideration - for short enough tokens, the speed and
      memory overhead of parsing another INPUT_CHAIN link outweighs the
      time to inline the token text.  */
-  if (src_chain->u.u_s.len <= INPUT_INLINE_THRESHOLD && !src_chain->argv_ref)
+  if (!src_chain->argv_ref
+      && src_chain->u.u_s.len <= INPUT_INLINE_THRESHOLD)
     {
       assert (src_chain->u.u_s.level >= 0);
       obstack_grow (obs, src_chain->u.u_s.str, src_chain->u.u_s.len);



        * src/input.c (pop_input): Avoid reading uninitialized data.

diff --git a/src/input.c b/src/input.c
index 3082a06..c912fdd 100644
--- a/src/input.c
+++ b/src/input.c
@@ -572,7 +572,7 @@ pop_input (bool cleanup)
        {
          if (!chain->argv_ref)
            {
-             if (*chain->u.u_s.str)
+             if (chain->u.u_s.str)
                return false;
              if (chain->u.u_s.level >= 0)
                adjust_refcount (chain->u.u_s.level, false);