[RFA] monitor.c fix for rom68k target boards

[RFA] monitor.c fix for rom68k target boards

[Jeff Holcomb]

This patch allows gdb to work with rom68k target boards once again (it's 
been broken for a year and a half apparently).

I don't think the routine needs to care about checking for termination of 
the string.  In the case of this monitor, the SR is returned as 
"2700-tfSm.111...xnzvc" for example.  Before Jason's change to this 
routine, it used strtoul which ended the matching at the first non 
digit.  Jason changed it to work with values larger than 'long' so it's 
now a hand-coded loop to gather the digits.

2001-03-29  Jeff Holcomb  <jeffh@redhat.com>

        * monitor.c (monitor_supply_register): Don't check for trailing
        junk since the rom68k monitor includes trailing junk in the
        status register.

Index: monitor.c
===================================================================
RCS file: /cvs/src/src/gdb/monitor.c,v
retrieving revision 1.22
diff -u -p -r1.22 monitor.c
--- monitor.c   2001/03/28 21:42:31     1.22
+++ monitor.c   2001/03/30 05:51:28
@@ -906,9 +906,8 @@ monitor_supply_register (int regno, char
     }
   monitor_debug ("Supplying Register %d %s\n", regno, valstr);

-  if (*p != '\0')
-    error ("monitor_supply_register (%d):  bad value from monitor: %s.",
-          regno, valstr);
+  /* Don't check for trailing junk since the 68k SR includes trailing
+     junk.  */

   /* supply register stores in target byte order, so swap here */



--
Jeff Holcomb
jeffh@redhat.com
GDB Engineering
Red Hat, Inc.

Re: [RFA] monitor.c fix for rom68k target boards

[Andrew Cagney]

Jeff Holcomb wrote:
> 
> This patch allows gdb to work with rom68k target boards once again (it's
> been broken for a year and a half apparently).
> 
> I don't think the routine needs to care about checking for termination of
> the string.  In the case of this monitor, the SR is returned as
> "2700-tfSm.111...xnzvc" for example.  Before Jason's change to this
> routine, it used strtoul which ended the matching at the first non
> digit.  Jason changed it to work with values larger than 'long' so it's
> now a hand-coded loop to gather the digits.

So the old code would just return with ``p'' pointing to one char beyond
the hex digit?

	Andrew

Re: [RFA] monitor.c fix for rom68k target boards

[Jeff Holcomb]

On Fri, 30 Mar 2001, Andrew Cagney wrote:

> So the old code would just return with ``p'' pointing to one char beyond
> the hex digit?

Yes, though p is a local variable in monitor_supply_register.  After the 
strtoul call p points to the location following the last valid digit.  The 
old code looked like this:

  val = strtoul (valstr, &p, 16);
  RDEBUG (("Supplying Register %d %s\n", regno, valstr));
 
  if (val == 0 && valstr == p)
    error ("monitor_supply_register (%d):  bad value from monitor: %s.",
           regno, valstr);

This seems like it was a valid check.  If the strtoul totally fails (no 
value was found and p hasn't changed), then it reports the error.

The new code looks like this:

  p = valstr;
  while (p && *p != '\0')
    {
      if (*p == '\r' || *p == '\n')
        {
          while (*p != '\0') 
              p++;
          break;
        }
      if (isspace (*p))
        {
          p++;
          continue;
        }
      if (!isxdigit (*p) && *p != 'x')
        {
          break;
        }
 
      val <<= 4;
      val += fromhex (*p++);
    }
  monitor_debug ("Supplying Register %d %s\n", regno, valstr);
 
  if (*p != '\0')
    error ("monitor_supply_register (%d):  bad value from monitor: %s.",
           regno, valstr);

This is testing something totally different.  If the next character is 
not the end of the string, then it reports the error.  The rom68k 
monitor outputs a text representation of the status register flags after 
the value.

A possibility would be to change the test back to the original version 
rather than removing the test, like:

  if (val == 0 && valstr == p)
     error ("monitor_supply_register (%d):  bad value from monitor: %s.",
           regno, valstr);

I'm willing to go either way, I just don't know what the intent was in 
changing the if test.

--
Jeff Holcomb
jeffh@redhat.com
GDB Engineering
Red Hat, Inc.

Re: [RFA] monitor.c fix for rom68k target boards

[Andrew Cagney]

> A possibility would be to change the test back to the original version
> rather than removing the test, like:
> 
>   if (val == 0 && valstr == p)
>      error ("monitor_supply_register (%d):  bad value from monitor: %s.",
>            regno, valstr);

Yes, that sounds better.  I think the function should still fail if it
does nothing - that would tend to suggest some sort of real problem with
the monitor.

> I'm willing to go either way, I just don't know what the intent was in
> changing the if test.

I don't think it was intentional.  Any way, assume your patch is
approved with the tweeked test.

enjoy,
	Andrew

[patch] monitor.c fix for rom68k target boards

[Jeff Holcomb]

On Wed, 4 Apr 2001, Andrew Cagney wrote:

> I don't think it was intentional.  Any way, assume your patch is
> approved with the tweeked test.

Here's the patch I checked in:

2001-04-05  Jeff Holcomb  <jeffh@redhat.com>
 
        * monitor.c (monitor_supply_register): Only report an error if we
        don't get a valid value.

Index: monitor.c
===================================================================
RCS file: /cvs/src/src/gdb/monitor.c,v
retrieving revision 1.22
diff -u -p -r1.22 monitor.c
--- monitor.c   2001/03/28 21:42:31     1.22
+++ monitor.c   2001/04/05 17:42:19
@@ -906,7 +906,7 @@ monitor_supply_register (int regno, char
     }
   monitor_debug ("Supplying Register %d %s\n", regno, valstr);
 
-  if (*p != '\0')
+  if (val == 0 && valstr == p)
     error ("monitor_supply_register (%d):  bad value from monitor: %s.",
           regno, valstr);