[RFC] A patch for parse_number (c-exp.y) to recognize 1.25f

[RFC] A patch for parse_number (c-exp.y) to recognize 1.25f

[Wu Zhou]

Hello all,

I just found a problem in gdb that its c-parser take 1.25f as invalid 
number.  After taking some look at the source code, I found that the error 
lies in the following code:

  if (parsed_float)
    {
      /* It's a float since it contains a point or an exponent.  */
      char c;
      int num = 0;      /* number of tokens scanned by scanf */
      char saved_char = p[len];

      p[len] = 0;       /* null-terminate the token */
      if (sizeof (putithere->typed_val_float.dval) <= sizeof (float))
        num = sscanf (p, "%g%c", (float *) &putithere->typed_val_float.dval,&c);
......
      p[len] = saved_char;      /* restore the input stream */
      if (num != 1)             /* check scanf found ONLY a float ... */
        return ERROR;

So I code a simple patch to let it recognize 1.25f or 1.25l correctly:

Index: c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.29
diff -c -3 -p -r1.29 c-exp.y
*** c-exp.y	8 Mar 2005 14:35:17 -0000	1.29
--- c-exp.y	5 Sep 2005 01:51:10 -0000
*************** parse_number (p, len, parsed_float, puti
*** 1097,1103 ****
  #endif
  	}
        p[len] = saved_char;	/* restore the input stream */
!       if (num != 1) 		/* check scanf found ONLY a float ... */
  	return ERROR;
        /* See if it has `f' or `l' suffix (float or long double).  */
  
--- 1097,1103 ----
  #endif
  	}
        p[len] = saved_char;	/* restore the input stream */
!       if (num != 1 && num != 2) 	/* check scanf found ONLY a float ... */
  	return ERROR;
        /* See if it has `f' or `l' suffix (float or long double).  */
  

This is the least intrusive way for GDB to recognize 1.25f as I think.  
But it still have a problem: it might handle 1.25df (or 1.25ddf and so on) 
as also a valid number and transfer it to 1.25.  So do we need to 
eliminate this kind of false-positive? 

To fix this kind of false-negative and not to introduce false-positive, I 
am thinking of changing the second format in sscanf to "%s" (string).  
Thus the parser could get the trailing characters following the float. 
Then we can add some code to parse them to eliminate false-positive.  Any 
points on this idea?  Is it worthwhile to do this?  Please comment.  TIA.

Regards
- Wu Zhou 

Re: [RFC] A patch for parse_number (c-exp.y) to recognize 1.25f

[Wu Zhou]

> To fix this kind of false-negative and not to introduce false-positive, I 
> am thinking of changing the second format in sscanf to "%s" (string).  
> Thus the parser could get the trailing characters following the float. 
> Then we can add some code to parse them to eliminate false-positive.  Any 
> points on this idea?  Is it worthwhile to do this?  Please comment.  TIA.

Following the above point, I coded another patch.  It can fix the problem 
and don't introduce any false-positive mentioned above.  I had also coded 
a testcase for that.  Had tested on i386.  With the patched GDB, it 
reported 18 PASS; with the original GDB, it report 9 PASS and 9 FAIL (all 
of these failure is due to the fact that GDB can't handle the suffix 
following the float constants.)

IMHO, there are yet another advantage for this patch: it is easy to be
extended to handle other suffixes, such "df" for _Decimal32, "dd" for 
_Decimal64 and "dl" for _Decimal128, all of which are described in IEEE 
754R for decimal floating point.

Here goes the patch and the testcase.

2005-09-15  Wu Zhou  <woodzltc@cn.ibm.com>

	* c-exp.y (parse-number): Modify the float parsing logic to let it 
	recognize the suffix.

Index: c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.29
diff -c -3 -p -r1.29 c-exp.y
*** c-exp.y	8 Mar 2005 14:35:17 -0000	1.29
--- c-exp.y	15 Sep 2005 08:26:36 -0000
*************** parse_number (p, len, parsed_float, puti
*** 1074,1116 ****
    if (parsed_float)
      {
        /* It's a float since it contains a point or an exponent.  */
!       char c;
        int num = 0;	/* number of tokens scanned by scanf */
        char saved_char = p[len];
  
        p[len] = 0;	/* null-terminate the token */
        if (sizeof (putithere->typed_val_float.dval) <= sizeof (float))
! 	num = sscanf (p, "%g%c", (float *) &putithere->typed_val_float.dval,&c);
        else if (sizeof (putithere->typed_val_float.dval) <= sizeof (double))
! 	num = sscanf (p, "%lg%c", (double *) &putithere->typed_val_float.dval,&c);
        else
  	{
  #ifdef SCANF_HAS_LONG_DOUBLE
! 	  num = sscanf (p, "%Lg%c", &putithere->typed_val_float.dval,&c);
  #else
  	  /* Scan it into a double, then assign it to the long double.
  	     This at least wins with values representable in the range
  	     of doubles. */
  	  double temp;
! 	  num = sscanf (p, "%lg%c", &temp,&c);
  	  putithere->typed_val_float.dval = temp;
  #endif
  	}
        p[len] = saved_char;	/* restore the input stream */
!       if (num != 1) 		/* check scanf found ONLY a float ... */
! 	return ERROR;
!       /* See if it has `f' or `l' suffix (float or long double).  */
! 
!       c = tolower (p[len - 1]);
! 
!       if (c == 'f')
! 	putithere->typed_val_float.type = builtin_type (current_gdbarch)->builtin_float;
!       else if (c == 'l')
! 	putithere->typed_val_float.type = builtin_type (current_gdbarch)->builtin_long_double;
!       else if (isdigit (c) || c == '.')
! 	putithere->typed_val_float.type = builtin_type (current_gdbarch)->builtin_double;
!       else
! 	return ERROR;
  
        return FLOAT;
      }
--- 1074,1121 ----
    if (parsed_float)
      {
        /* It's a float since it contains a point or an exponent.  */
!       char *s = malloc (len);
        int num = 0;	/* number of tokens scanned by scanf */
        char saved_char = p[len];
  
        p[len] = 0;	/* null-terminate the token */
+ 
        if (sizeof (putithere->typed_val_float.dval) <= sizeof (float))
! 	num = sscanf (p, "%g%s", (float *) &putithere->typed_val_float.dval,s);
        else if (sizeof (putithere->typed_val_float.dval) <= sizeof (double))
! 	num = sscanf (p, "%lg%s", (double *) &putithere->typed_val_float.dval,s);
        else
  	{
  #ifdef SCANF_HAS_LONG_DOUBLE
! 	  num = sscanf (p, "%Lg%s", &putithere->typed_val_float.dval,s);
  #else
  	  /* Scan it into a double, then assign it to the long double.
  	     This at least wins with values representable in the range
  	     of doubles. */
  	  double temp;
! 	  num = sscanf (p, "%lg%s", &temp,s);
  	  putithere->typed_val_float.dval = temp;
  #endif
  	}
        p[len] = saved_char;	/* restore the input stream */
! 
!       if (num == 1)
! 	putithere->typed_val_float.type = builtin_type (current_gdbarch)
! 					    ->builtin_double;
! 
!       if (num == 2 )
! 	{
! 	  /* See if it has any float suffix: 'f' for float, 'l' for long 
! 	     double.  */
! 	  if (!strcasecmp (s, "f"))
! 	    putithere->typed_val_float.type = builtin_type (current_gdbarch)
! 						->builtin_float;
! 	  else if (!strcasecmp (s, "l"))
! 	    putithere->typed_val_float.type = builtin_type (current_gdbarch)
! 						->builtin_long_double;
! 	  else
! 	    return ERROR;
! 	}
  
        return FLOAT;
      }

2005-09-15  Wu Zhou  <woodzltc@cn.ibm.com>

	* bfp-test.c: New file.
	* bfp-test.exp: New testcase.

Index: gdb.base/bfp-test.c
===================================================================
RCS file: gdb.base/bfp-test.c
diff -N gdb.base/bfp-test.c
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- gdb.base/bfp-test.c	15 Sep 2005 06:49:51 -0000
***************
*** 0 ****
--- 1,15 ----
+ #include <stdio.h>
+ #include <stdlib.h>
+ 
+ float b32;
+ double b64;
+ long double b128;
+ 
+ int main()
+ {
+   b32 = 1.5f;
+   b64 = 2.25;
+   b128 = 3.375l;
+  
+   return 0;
+ }
Index: gdb.base/bfp-test.exp
===================================================================
RCS file: gdb.base/bfp-test.exp
diff -N gdb.base/bfp-test.exp
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- gdb.base/bfp-test.exp	15 Sep 2005 06:49:51 -0000
***************
*** 0 ****
--- 1,109 ----
+ # Copyright 2005 Free Software Foundation, Inc.
+ 
+ # This program is free software; you can redistribute it and/or modify
+ # it under the terms of the GNU General Public License as published by
+ # the Free Software Foundation; either version 2 of the License, or
+ # (at your option) any later version.
+ #
+ # This program is distributed in the hope that it will be useful,
+ # but WITHOUT ANY WARRANTY; without even the implied warranty of
+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ # GNU General Public License for more details.
+ #
+ # You should have received a copy of the GNU General Public License
+ # along with this program; if not, write to the Free Software
+ # Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+ 
+ # This file is part of the gdb testsuite.  It is intended to test that
+ # gdb could correctly handle floating point constant with a suffix.
+ 
+ if $tracelevel {
+     strace $tracelevel
+ }
+ 
+ set testfile "bfp-test"
+ set srcfile ${testfile}.c
+ set binfile ${objdir}/${subdir}/${testfile}
+ if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested "Couldn't compile ${srcfile}"
+     return -1
+ }
+ 
+ gdb_exit
+ gdb_start
+ gdb_reinitialize_dir $srcdir/$subdir
+ gdb_load ${binfile}
+ 
+ if ![runto_main] then {
+     perror "couldn't run to breakpoint"
+     continue
+ }
+ 
+ # Run to the breakpoint at return.
+ set bp_location [gdb_get_line_number "return"]
+ gdb_test "break $bp_location" \
+     "Breakpoint.*at.* file .*$srcfile, line $bp_location\\." \
+     "breakpoint at return"
+ gdb_test "continue" \
+     "Continuing\\..*Breakpoint.*" \
+     "continue to breakpoint"
+ 
+ # Print the original value of b32, b64 and b128.
+ gdb_test "print b32" ".*1 = 1\.5.*" "The original value of b32 is 1.5"
+ gdb_test "print b64" ".*2 = 2\.25.*" "The original value of b64 is 2.25"
+ gdb_test "print b128" ".*3 = 3\.375.*" "The original value of b128 is 3.375"
+ 
+ # Test that gdb could correctly recognize float constant expression with a suffix. 
+ gdb_test "print b32=-1.5f" ".*4 = -1\.5.*" "Try to change b32 to -1.5 with 'print b32=-1.5f'"
+ gdb_test "print b64=-2.25f" ".*5 = -2\.25.*" "Try to change b64 to -2.25 with 'print b64=-2.25f'"
+ gdb_test "print b128=-3.375l" ".*6 = -3\.375.*" "Try to change b128 to -3.375 with 'print b128=-3.375l'"
+ 
+ # Test that gdb could handle the above correctly with "set var" command.
+ send_gdb "set var b32=10.5f\n"
+ gdb_expect {
+     -re "$gdb_prompt $" { pass "set var b32=10.5f" }
+     -re "Invalid number" { fail "do not recognize 10.5f" }
+     timeout     {fail "set var b32=10.5f" }
+   }
+ send_gdb "set var b64=20.25f\n"
+ gdb_expect {
+     -re "$gdb_prompt $" { pass "set var b64=20.25f" }
+     -re "Invalid number" { fail "do not recognize 20.25f" }
+     timeout     {fail "set var b64=20.25f" }
+   }
+ send_gdb "set var b128=30.375f\n"
+ gdb_expect {
+     -re "$gdb_prompt $" { pass "set var b128=30.375f" }
+     -re "Invalid number" { fail "do not recognize 30.375f" }
+     timeout     {fail "set var b128=30.375f" }
+   }
+ gdb_test "print b32" ".*7 = 10\.5.*" "The value of b32 is changed to 10.5"
+ gdb_test "print b64" ".*8 = 20\.25.*" "The value of b64 is changed to 20.25"
+ gdb_test "print b128" ".*9 = 30\.375.*" "The value of b128 is changed to 30.375"
+ 
+ # Test that gdb could handle invalid suffix correctly.
+ send_gdb "set var b32=100.5a\n"
+ gdb_expect {
+     -re "$gdb_prompt $" { fail "don't report error on 100.5a" }
+     -re "Invalid number" { pass "100.5a is invalid number" }
+     timeout     {fail "set var b32=100.5a" }
+   }
+ send_gdb "set var b64=200.25x\n"
+ gdb_expect {
+     -re "$gdb_prompt $" { fail "don't report error on 200.25x" }
+     -re "Invalid number" { pass "200.25x is invalid number" }
+     timeout     {fail "set var b64=200.25x" }
+   }
+ send_gdb "set var b128=300.375fl\n"
+ gdb_expect {
+     -re "$gdb_prompt $" { fail "don't report error on 300.375fl" }
+     -re "Invalid number" { pass "300.375fl is invalid number" }
+     timeout     {fail "set var b128=300.375fl" }
+   }
+ send_gdb "set var b32=300.375fff\n"
+ gdb_expect {
+     -re "$gdb_prompt $" { fail "don't report error on 300.375fff" }
+     -re "Invalid number" { pass "300.375fff is invalid number" }
+     timeout     {fail "set var b128=300.375fff" }
+   }
+  

Regards
- Wu Zhou

Re: [RFC] A patch for parse_number (c-exp.y) to recognize 1.25f

[Daniel Jacobowitz]

On Thu, Sep 15, 2005 at 04:05:48PM +0800, Wu Zhou wrote:
> 
> > To fix this kind of false-negative and not to introduce false-positive, I 
> > am thinking of changing the second format in sscanf to "%s" (string).  
> > Thus the parser could get the trailing characters following the float. 
> > Then we can add some code to parse them to eliminate false-positive.  Any 
> > points on this idea?  Is it worthwhile to do this?  Please comment.  TIA.
> 
> Following the above point, I coded another patch.  It can fix the problem 
> and don't introduce any false-positive mentioned above.  I had also coded 
> a testcase for that.  Had tested on i386.  With the patched GDB, it 
> reported 18 PASS; with the original GDB, it report 9 PASS and 9 FAIL (all 
> of these failure is due to the fact that GDB can't handle the suffix 
> following the float constants.)
> 
> IMHO, there are yet another advantage for this patch: it is easy to be
> extended to handle other suffixes, such "df" for _Decimal32, "dd" for 
> _Decimal64 and "dl" for _Decimal128, all of which are described in IEEE 
> 754R for decimal floating point.
> 
> Here goes the patch and the testcase.

This looks good.  I have some comments on formatting and testing, but
that's it.

> 2005-09-15  Wu Zhou  <woodzltc@cn.ibm.com>
> 
> 	* c-exp.y (parse-number): Modify the float parsing logic to let it 
> 	recognize the suffix.

"a suffix" rather than "the suffix", please.

> !       if (num == 1)
> ! 	putithere->typed_val_float.type = builtin_type (current_gdbarch)
> ! 					    ->builtin_double;

Instead of splitting this line (and the two below) in the middle of a
single reference, you can split it at the assignment.  Like this - two
spaces in the continued line:

	putithere->typed_val_float.type
	  = builtin_type (current_gdbarch)->builtin_double;

Emacs'll butcher your version, I'm afraid.

> Index: gdb.base/bfp-test.c
> ===================================================================
> RCS file: gdb.base/bfp-test.c
> diff -N gdb.base/bfp-test.c
> *** /dev/null	1 Jan 1970 00:00:00 -0000
> --- gdb.base/bfp-test.c	15 Sep 2005 06:49:51 -0000
> ***************
> *** 0 ****
> --- 1,15 ----

It's a pretty trivial test case; but please add a copyright notice
anyway.

> + # Run to the breakpoint at return.
> + set bp_location [gdb_get_line_number "return"]
> + gdb_test "break $bp_location" \
> +     "Breakpoint.*at.* file .*$srcfile, line $bp_location\\." \
> +     "breakpoint at return"
> + gdb_test "continue" \
> +     "Continuing\\..*Breakpoint.*" \
> +     "continue to breakpoint"

This is fine, but you can also use gdb_breakpoint and
gdb_continue_to_brekpoint or gdb_continue.

> + # Test that gdb could handle the above correctly with "set var" command.
> + send_gdb "set var b32=10.5f\n"
> + gdb_expect {
> +     -re "$gdb_prompt $" { pass "set var b32=10.5f" }
> +     -re "Invalid number" { fail "do not recognize 10.5f" }
> +     timeout     {fail "set var b32=10.5f" }
> +   }

Please don't use "send_gdb" and "gdb_expect" directly.  Also, the
passing and failing tests should have the same message - an optional
string at the end in parentheses explaining the failure is OK, but
anything else is bad for automated testing.

gdb_test_multiple should work OK everywhere you used send_gdb.

> + # Test that gdb could handle invalid suffix correctly.
> + send_gdb "set var b32=100.5a\n"
> + gdb_expect {
> +     -re "$gdb_prompt $" { fail "don't report error on 100.5a" }
> +     -re "Invalid number" { pass "100.5a is invalid number" }
> +     timeout     {fail "set var b32=100.5a" }
> +   }

... although all of the invalid tests could just use gdb_test.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC] A patch for parse_number (c-exp.y) to recognize 1.25f

[Wu Zhou]

Hi Daniel,

> This looks good.  I have some comments on formatting and testing, but
> that's it.

Thanks for your comment. They are very helpful and informative indeed.

> > 
> > 	* c-exp.y (parse-number): Modify the float parsing logic to let it 
> > 	recognize the suffix.
> 
> "a suffix" rather than "the suffix", please.

Changed.

> 
> > !       if (num == 1)
> > ! 	putithere->typed_val_float.type = builtin_type (current_gdbarch)
> > ! 					    ->builtin_double;
> 
> Instead of splitting this line (and the two below) in the middle of a
> single reference, you can split it at the assignment.  Like this - two
> spaces in the continued line:
> 
> 	putithere->typed_val_float.type
> 	  = builtin_type (current_gdbarch)->builtin_double;
> 
> Emacs'll butcher your version, I'm afraid.

I am not familar with Emacs.  But I believe that you are right.  Fixed.

[snip]
> 
> It's a pretty trivial test case; but please add a copyright notice
> anyway.

Added.

> 
> > + # Run to the breakpoint at return.
> > + set bp_location [gdb_get_line_number "return"]
> > + gdb_test "break $bp_location" \
> > +     "Breakpoint.*at.* file .*$srcfile, line $bp_location\\." \
> > +     "breakpoint at return"
> > + gdb_test "continue" \
> > +     "Continuing\\..*Breakpoint.*" \
> > +     "continue to breakpoint"
> 
> This is fine, but you can also use gdb_breakpoint and
> gdb_continue_to_brekpoint or gdb_continue.

Switched to gdb_breakpoint and gdb_continue_to_brekpoint.

> 
> > + # Test that gdb could handle the above correctly with "set var" command.
> > + send_gdb "set var b32=10.5f\n"
> > + gdb_expect {
> > +     -re "$gdb_prompt $" { pass "set var b32=10.5f" }
> > +     -re "Invalid number" { fail "do not recognize 10.5f" }
> > +     timeout     {fail "set var b32=10.5f" }
> > +   }
> 
> Please don't use "send_gdb" and "gdb_expect" directly.  Also, the
> passing and failing tests should have the same message - an optional
> string at the end in parentheses explaining the failure is OK, but
> anything else is bad for automated testing.

Good guideline.  I am now using gdb_test_multiple.

I had run the modified testcase against patched and un-patched GDB. It 
report expected test result.  Both the patch and testcases are checked in.  

Thanks.

Regards
- Wu Zhou