RFA: fix macro expansion bug

RFA: fix macro expansion bug

[Tom Tromey]

This patch fixes a macro expansion bug pointed out by Pedro.

The bug is that get_pp_number incorrectly handles pp-number tokens
starting with '.'.  According to the C standard, it ought to only
accept '.' followed by a digit at the beginning of a pp-number.
However, it unconditionally accepts a leading '.'.

Built and regtested on x86-64 (compile farm).
Test case included.

Please review.

Tom

2008-12-11  Tom Tromey  <tromey@redhat.com>

	* macroexp.c (get_pp_number): Require digit after leading ".".

2008-12-11  Tom Tromey  <tromey@redhat.com>

	* gdb.base/macscp.exp: New regression test.

diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index 7fb23ce..982df4d 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -278,7 +278,9 @@ get_pp_number (struct macro_buffer *tok, char *p, char *end)
 {
   if (p < end
       && (macro_is_digit (*p)
-          || *p == '.'))
+          || (*p == '.'
+	      && p + 2 <= end
+	      && macro_is_digit (p[1]))))
     {
       char *tok_start = p;
 
diff --git a/gdb/testsuite/gdb.base/macscp.exp b/gdb/testsuite/gdb.base/macscp.exp
index 9cb9ef5..40021b9 100644
--- a/gdb/testsuite/gdb.base/macscp.exp
+++ b/gdb/testsuite/gdb.base/macscp.exp
@@ -652,3 +652,11 @@ gdb_test "print str(maude)" \
 gdb_test "print xstr(maude)" \
   " = \"5\"" \
   "stringify with substitution"
+
+# Regression test for pp-number bug.
+gdb_test "macro define si_addr fields.fault.si_addr" \
+  "" \
+  "define si_addr macro"
+gdb_test "macro expand siginfo.si_addr" \
+  "expands to: siginfo. fields.fault.si_addr" \
+  "macro expand siginfo.si_addr"

Re: RFA: fix macro expansion bug

[Pedro Alves]

On Thursday 11 December 2008 21:36:24, Tom Tromey wrote:
> This patch fixes a macro expansion bug pointed out by Pedro.
> 
> The bug is that get_pp_number incorrectly handles pp-number tokens
> starting with '.'.  According to the C standard, it ought to only
> accept '.' followed by a digit at the beginning of a pp-number.

Indeed it does.  Clearly a bug.

> However, it unconditionally accepts a leading '.'.
> 
> Built and regtested on x86-64 (compile farm).
> Test case included.
> 
> Please review.
> 

Looks good, this is OK.

Though, I'm having a bit of trouble convincing myself that the logic to
handle 'pp-number e|E|p|P|. sign' below is 100% sane.

static int
get_pp_number (struct macro_buffer *tok, char *p, char *end)
{
...
      while (p < end)
        {
          if (macro_is_digit (*p)
              || macro_is_identifier_nondigit (*p)
              || *p == '.')
            p++;
          else if (p + 2 <= end
                   && strchr ("eEpP.", *p)
                   && (p[1] == '+' || p[1] == '-'))
            p += 2;
          else
            break;
        }

It seems macro_is_identifier_nondigit will always eat any of "eEpP",
thus, say, when parsing "1e-" only "1e" will be identified as a pp
number, leaving "+" in the stream.  Is this right?

> +
> +# Regression test for pp-number bug.
> +gdb_test "macro define si_addr fields.fault.si_addr" \
> +  "" \
> +  "define si_addr macro"
> +gdb_test "macro expand siginfo.si_addr" \
> +  "expands to: siginfo. fields.fault.si_addr" \
                          ^
Just curious, as it's just a visual annoyance: do you know where
this space comes from?  Do we store the definition with the space for
some reason?  We don't get that extra space if the define came
from the code, instead of from a 'macro define'.

-- 
Pedro Alves

Re: RFA: fix macro expansion bug

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Though, I'm having a bit of trouble convincing myself that the logic to
Pedro> handle 'pp-number e|E|p|P|. sign' below is 100% sane.
[...]
Pedro> It seems macro_is_identifier_nondigit will always eat any of "eEpP",
Pedro> thus, say, when parsing "1e-" only "1e" will be identified as a pp
Pedro> number, leaving "+" in the stream.  Is this right?

Yeah.  Also, "." should not appear in the strchr argument.

Here's a new patch.  I'll regression-test it.  I don't expect
problems.  Ok if it passes?

>> +  "expands to: siginfo. fields.fault.si_addr" \

Pedro> Just curious, as it's just a visual annoyance: do you know where
Pedro> this space comes from?  Do we store the definition with the space for
Pedro> some reason?  We don't get that extra space if the define came
Pedro> from the code, instead of from a 'macro define'.

Yes, it is a bug in "macro define".
I'll fix shortly.

Tom


2008-12-11  Tom Tromey  <tromey@redhat.com>

	* macroexp.c (get_pp_number): Require digit after leading ".".
	Correctly handle suffixes.

2008-12-11  Tom Tromey  <tromey@redhat.com>

	* gdb.base/macscp.exp: New regression test.

diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index 7fb23ce..dda3592 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -278,20 +278,22 @@ get_pp_number (struct macro_buffer *tok, char *p, char *end)
 {
   if (p < end
       && (macro_is_digit (*p)
-          || *p == '.'))
+          || (*p == '.'
+	      && p + 2 <= end
+	      && macro_is_digit (p[1]))))
     {
       char *tok_start = p;
 
       while (p < end)
         {
-          if (macro_is_digit (*p)
-              || macro_is_identifier_nondigit (*p)
-              || *p == '.')
-            p++;
-          else if (p + 2 <= end
-                   && strchr ("eEpP.", *p)
-                   && (p[1] == '+' || p[1] == '-'))
+	  if (p + 2 <= end
+	      && strchr ("eEpP", *p)
+	      && (p[1] == '+' || p[1] == '-'))
             p += 2;
+          else if (macro_is_digit (*p)
+		   || macro_is_identifier_nondigit (*p)
+		   || *p == '.')
+            p++;
           else
             break;
         }
diff --git a/gdb/testsuite/gdb.base/macscp.exp b/gdb/testsuite/gdb.base/macscp.exp
index 9cb9ef5..40021b9 100644
--- a/gdb/testsuite/gdb.base/macscp.exp
+++ b/gdb/testsuite/gdb.base/macscp.exp
@@ -652,3 +652,11 @@ gdb_test "print str(maude)" \
 gdb_test "print xstr(maude)" \
   " = \"5\"" \
   "stringify with substitution"
+
+# Regression test for pp-number bug.
+gdb_test "macro define si_addr fields.fault.si_addr" \
+  "" \
+  "define si_addr macro"
+gdb_test "macro expand siginfo.si_addr" \
+  "expands to: siginfo. fields.fault.si_addr" \
+  "macro expand siginfo.si_addr"

Re: RFA: fix macro expansion bug

[Andreas Schwab]

Pedro Alves <pedro@codesourcery.com> writes:

> Though, I'm having a bit of trouble convincing myself that the logic to
> handle 'pp-number e|E|p|P|. sign' below is 100% sane.
>
> static int
> get_pp_number (struct macro_buffer *tok, char *p, char *end)
> {
> ...
>       while (p < end)
>         {
>           if (macro_is_digit (*p)
>               || macro_is_identifier_nondigit (*p)
>               || *p == '.')
>             p++;
>           else if (p + 2 <= end
>                    && strchr ("eEpP.", *p)
>                    && (p[1] == '+' || p[1] == '-'))
>             p += 2;
>           else
>             break;
>         }
>
> It seems macro_is_identifier_nondigit will always eat any of "eEpP",
> thus, say, when parsing "1e-" only "1e" will be identified as a pp
> number, leaving "+" in the stream.  Is this right?

Also, a sign can only follow [eEpP], but not [.].

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: RFA: fix macro expansion bug

[Pedro Alves]

On Thursday 11 December 2008 23:25:48, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> Though, I'm having a bit of trouble convincing myself that the logic to
> Pedro> handle 'pp-number e|E|p|P|. sign' below is 100% sane.
> [...]
> Pedro> It seems macro_is_identifier_nondigit will always eat any of "eEpP",
> Pedro> thus, say, when parsing "1e-" only "1e" will be identified as a pp
> Pedro> number, leaving "+" in the stream.  Is this right?
> 
> Yeah.  Also, "." should not appear in the strchr argument.
> 
> Here's a new patch.  I'll regression-test it.  I don't expect
> problems.  Ok if it passes?

Certainly.  Thanks.

> >> +  "expands to: siginfo. fields.fault.si_addr" \
> 
> Pedro> Just curious, as it's just a visual annoyance: do you know where
> Pedro> this space comes from?  Do we store the definition with the space for
> Pedro> some reason?  We don't get that extra space if the define came
> Pedro> from the code, instead of from a 'macro define'.
> 
> Yes, it is a bug in "macro define".
> I'll fix shortly.
> 

Thanks!

-- 
Pedro Alves

Re: RFA: fix macro expansion bug

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

>> +gdb_test "macro expand siginfo.si_addr" \
>> +  "expands to: siginfo. fields.fault.si_addr" \

Pedro> Just curious, as it's just a visual annoyance: do you know where
Pedro> this space comes from?  Do we store the definition with the space for
Pedro> some reason?  We don't get that extra space if the define came
Pedro> from the code, instead of from a 'macro define'.

Here's a patch to fix it.

This requires a minor tweak to the previous patch's test case, but
otherwise nothing.

As with the previous, I'm sending this through the regression tester,
but I don't expect problems.  Ok if it passes?

Tom

2008-12-11  Tom Tromey  <tromey@redhat.com>

	* macrocmd.c (macro_define_command): Skip whitespace after
	macro name.
	(print_one_macro): Print space after macro name.

diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
index 56e9a48..fa639d1 100644
--- a/gdb/macrocmd.c
+++ b/gdb/macrocmd.c
@@ -315,13 +315,17 @@ macro_define_command (char *exp, int from_tty)
 	}
       /* Skip the closing paren.  */
       ++exp;
+      skip_ws (&exp);
 
       macro_define_function (macro_main (macro_user_macros), -1, name,
 			     new_macro.argc, (const char **) new_macro.argv,
 			     exp);
     }
   else
-    macro_define_object (macro_main (macro_user_macros), -1, name, exp);
+    {
+      skip_ws (&exp);
+      macro_define_object (macro_main (macro_user_macros), -1, name, exp);
+    }
 
   do_cleanups (cleanup_chain);
 }
@@ -358,9 +362,7 @@ print_one_macro (const char *name, const struct macro_definition *macro,
 			  macro->argv[i]);
       fprintf_filtered (gdb_stdout, ")");
     }
-  /* Note that we don't need a leading space here -- "macro define"
-     provided it.  */
-  fprintf_filtered (gdb_stdout, "%s\n", macro->replacement);
+  fprintf_filtered (gdb_stdout, " %s\n", macro->replacement);
 }
 
 

Re: RFA: fix macro expansion bug

[Pedro Alves]

On Thursday 11 December 2008 23:38:19, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> >> +gdb_test "macro expand siginfo.si_addr" \
> >> +  "expands to: siginfo. fields.fault.si_addr" \
> 
> Pedro> Just curious, as it's just a visual annoyance: do you know where
> Pedro> this space comes from?  Do we store the definition with the space for
> Pedro> some reason?  We don't get that extra space if the define came
> Pedro> from the code, instead of from a 'macro define'.
> 
> Here's a patch to fix it.
> 
> This requires a minor tweak to the previous patch's test case, but
> otherwise nothing.
> 
> As with the previous, I'm sending this through the regression tester,
> but I don't expect problems.  Ok if it passes?

Looks obvious enough.  Ok if it passes.


Got curious and confirmed that somehow the trailing end
whitespace is stripped already.

(gdb) macro define foo      bar      
                      ^^^^^^   ^^^^^^
(spaces)

(gdb) macro expand a->foo->b
expands to: a->      bar->b
(gdb) 

> 
> Tom
> 
> 2008-12-11  Tom Tromey  <tromey@redhat.com>
> 
> 	* macrocmd.c (macro_define_command): Skip whitespace after
> 	macro name.
> 	(print_one_macro): Print space after macro name.
> 
> diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
> index 56e9a48..fa639d1 100644
> --- a/gdb/macrocmd.c
> +++ b/gdb/macrocmd.c
> @@ -315,13 +315,17 @@ macro_define_command (char *exp, int from_tty)
>  	}
>        /* Skip the closing paren.  */
>        ++exp;
> +      skip_ws (&exp);
>  
>        macro_define_function (macro_main (macro_user_macros), -1, name,
>  			     new_macro.argc, (const char **) new_macro.argv,
>  			     exp);
>      }
>    else
> -    macro_define_object (macro_main (macro_user_macros), -1, name, exp);
> +    {
> +      skip_ws (&exp);
> +      macro_define_object (macro_main (macro_user_macros), -1, name, exp);
> +    }
>  
>    do_cleanups (cleanup_chain);
>  }
> @@ -358,9 +362,7 @@ print_one_macro (const char *name, const struct macro_definition *macro,
>  			  macro->argv[i]);
>        fprintf_filtered (gdb_stdout, ")");
>      }
> -  /* Note that we don't need a leading space here -- "macro define"
> -     provided it.  */
> -  fprintf_filtered (gdb_stdout, "%s\n", macro->replacement);
> +  fprintf_filtered (gdb_stdout, " %s\n", macro->replacement);
>  }
>  
>  
> 



-- 
Pedro Alves

Re: RFA: fix macro expansion bug

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Got curious and confirmed that somehow the trailing end
Pedro> whitespace is stripped already.

Pedro> (gdb) macro define foo      bar      
Pedro>                       ^^^^^^   ^^^^^^

I believe trailing whitespace is stripped by gdb command processing.

Tom

Re: RFA: fix macro expansion bug

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

[ macro define's extra space ]

>> As with the previous, I'm sending this through the regression tester,
>> but I don't expect problems.  Ok if it passes?

Pedro> Looks obvious enough.  Ok if it passes.

Committed.

Tom

Re: RFA: fix macro expansion bug

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

>> Here's a new patch.  I'll regression-test it.  I don't expect
>> problems.  Ok if it passes?

Pedro> Certainly.  Thanks.

I checked this in, with the obvious update to the test suite to delete
the extra space.

Tom