Re: 7.4->7.5 Regression gdb.base/pending.exp with gdbserver [Re: [PATCH] Dynamic printf for a target agent]

[Yao Qi <yao@codesourcery.com>]

On Wednesday, July 18, 2012 09:17:41 PM Jan Kratochvil wrote:
> 2885fb01788703023acfeb2bade84c20d8723853 is the first bad commit
> commit 2885fb01788703023acfeb2bade84c20d8723853
> Author: Stan Shebs <shebs@apple.com>
> Date:   Mon Jul 2 15:29:29 2012 +0000
>         Add target-side support for dynamic printf.
> 
> Running ./gdb.base/pending.exp ...
> FAIL: gdb.base/pending.exp: continue to resolved breakpoint 2
> FAIL: gdb.base/pending.exp: continue to resolved breakpoint 1
> FAIL: gdb.base/pending.exp: continue to resolved breakpoint 3 (the program
> exited)
> 
> There is somehow missed a breakpoint, IIUC.

Hi,
This regression is caused by the following change,

On Monday, July 02, 2012 11:18:34 AM Stan Shebs wrote:
> @@ -2916,22 +2918,33 @@ process_point_options (CORE_ADDR point_a
>  
>    while (*dataptr)
>      {
> -      switch (*dataptr)
> +      if (*dataptr == ';')
> +       ++dataptr;
> +
> +      if (*dataptr == 'X')
>         {
> -         case 'X':
> -           /* Conditional expression.  */
> -           if (remote_debug)
> -             fprintf (stderr, "Found breakpoint condition.\n");
> -           add_breakpoint_condition (point_addr, &dataptr);
> -           break;
> -         default:
> -           /* Unrecognized token, just skip it.  */
> -           fprintf (stderr, "Unknown token %c, ignoring.\n",
> -                    *dataptr);
> +         /* Conditional expression.  */
> +         fprintf (stderr, "Found breakpoint condition.\n");
> +         add_breakpoint_condition (point_addr, &dataptr);
> +       }
> +      else if (strncmp (dataptr, "cmds:", strlen ("cmds:")) == 0)
> +       {
> +         dataptr += strlen ("cmds:");
> +         if (debug_threads)
> +           fprintf (stderr, "Found breakpoint commands %s.\n", dataptr);
> +         persist = (*dataptr == '1');
> +         dataptr += 2;
> +         add_breakpoint_commands (point_addr, &dataptr, persist);
> +       }
> +      else
> +       {
> +         /* Unrecognized token, just skip it.  */
> +         fprintf (stderr, "Unknown token %c, ignoring.\n",
> +                  *dataptr);
>         }
>  
>        /* Skip tokens until we find one that we recognize.  */
> -      while (*dataptr && *dataptr != 'X' && *dataptr != ';')
> +      while (*dataptr && *dataptr != ';')

It seems incorrect to remove "*dataptr != 'X'" out of this condition checking.

With the breakpoint commands added, the Z packet becomes

  "Z0xxxxXxxxx,Xxxxx;cmds:xxxxx"

When parsing this packet, in current (wrong) GDBserver, only the first
condition ('Xxxxx') in packet is added, and the rest of conditions are skipped,
which is a mistake.

The fix is just to revert this change.  Regression tested on x86_64/gdbserver,
and these fails are fixed.

-FAIL: gdb.base/pending.exp: continue to resolved breakpoint 2
-FAIL: gdb.base/pending.exp: continue to resolved breakpoint 1
+PASS: gdb.base/pending.exp: continue to resolved breakpoint 2
+PASS: gdb.base/pending.exp: continue to resolved breakpoint 1
-FAIL: gdb.base/pending.exp: continue to resolved breakpoint 3 (the program exited)
+PASS: gdb.base/pending.exp: continue to resolved breakpoint 3

Regression tested on x86_64/native, no changes.

-- 
Yao (齐尧)

gdb/gdbserver:

2012-07-20  Yao Qi  <yao@codesourcery.com>

	* server.c (process_point_options): Stop at 'X' when parsing.

---
 gdb/gdbserver/server.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 963d575..4e15b3c 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2944,7 +2944,7 @@ process_point_options (CORE_ADDR point_addr, char **packet)
 	}
 
       /* Skip tokens until we find one that we recognize.  */
-      while (*dataptr && *dataptr != ';')
+      while (*dataptr && *dataptr != 'X' && *dataptr != ';')
 	dataptr++;
     }
   *packet = dataptr;
-- 
1.7.7.6