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

Ralf Wildenhues <[email protected]>
Newsgroups gmane.comp.gnu.m4.patches
Organization Department of Numerical Simulation, University of Bonn
Message-ID <[email protected]>
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);
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.