[PATCH] Fix bug 15433 - GDB crashes when using agent dprintf, %s format, and an in-line string

[PATCH] Fix bug 15433 - GDB crashes when using agent dprintf, %s format, and an in-line string

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

Hi,

This issue is because GDB doesn't OP_STRING now.  So
parse_cmd_to_aexpr will got NULL if using agent dprintf, %s format,
and an in-line string.
And this NULL will be push to bl->target_info.tcommands.  Then it make
remote part crash.

So add a check in build_target_command_list for the return of
parse_cmd_to_aexpr.  And add a test for it.

Thanks,
Hui

2013-05-11  Hui Zhu  <hui@codesourcery.com>

	PR gdb/15433
	* breakpoint.c (build_target_command_list): Add check for
	the return of parse_cmd_to_aexpr.

2013-05-11  Hui Zhu  <hui@codesourcery.com>

	PR gdb/15433
	* gdb.base/dprintf.exp: Test unsupport commands on target.

[-- Attachment #2: dprintf-cmd-not-support.txt --]
[-- Type: text/plain, Size: 425 bytes --]

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2302,6 +2302,9 @@ build_target_command_list (struct bp_loc
 		 need to parse the command to bytecodes again.  */
 	      aexpr = parse_cmd_to_aexpr (bl->address,
 					  loc->owner->extra_string);
+	      if (aexpr == NULL)
+		error (_("Agent is not support commands of breakpoint %d."),
+		       bl->owner->number);
 	      loc->cmd_bytecode = aexpr;
 
 	      if (!aexpr)

[-- Attachment #3: dprintf-cmd-not-support-test.txt --]
[-- Type: text/plain, Size: 526 bytes --]

--- a/gdb/testsuite/gdb.base/dprintf.exp
+++ b/gdb/testsuite/gdb.base/dprintf.exp
@@ -131,6 +131,11 @@ if $target_can_dprintf {
 	"\[\r\n\]\tbreakpoint already hit 2 times"
 	"\[\r\n\]        agent-printf \"arg=%d, g=%d\\\\n\", arg, g"
     }
+
+    # Test unsupport commands.
+    gdb_test "dprintf $dp_location1,\"%s\\n\", \"test\"" "Dprintf .*"
+    gdb_test "continue" "Agent is not support commands of breakpoint .*" \
+      "3rd dprintf, agent"
 }
 
 gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \

Re: [PATCH] Fix bug 15433 - GDB crashes when using agent dprintf, %s format, and an in-line string

[Yao Qi]

On 05/11/2013 01:54 PM, Hui Zhu wrote:
> @@ -2302,6 +2302,9 @@ build_target_command_list (struct bp_loc
>   		 need to parse the command to bytecodes again.  */
>   	      aexpr = parse_cmd_to_aexpr (bl->address,
>   					  loc->owner->extra_string);
> +	      if (aexpr == NULL)
> +		error (_("Agent is not support commands of breakpoint %d."),
> +		       bl->owner->number);
>   	      loc->cmd_bytecode = aexpr;
>
>   	      if (!aexpr)

The existing code has considered the situation that we have a NULL 
bytecode expression.  Several lines of code below this line of patch do 
something,

	  /* If we have a NULL bytecode expression, it means something
	     went wrong or we have a null command expression.  */
	  if (!loc->cmd_bytecode)
	    {
	      null_command_or_parse_error = 1;
	      break;
	    }
	}
     }

   /* If anything failed, then we're not doing target-side commands,
      and so clean up.  */
   if (null_command_or_parse_error)

Your patch doesn't look right, and probably we need to examine why 
dprintf breakpoint is not handled well in this part.

-- 
Yao (齐尧)

Re: [PATCH] Fix bug 15433 - GDB crashes when using agent dprintf, %s format, and an in-line string

[Hui Zhu]

Hi Yao,

Thanks for your review.

On Mon, May 13, 2013 at 3:13 PM, Yao Qi <yao@codesourcery.com> wrote:
> On 05/11/2013 01:54 PM, Hui Zhu wrote:
>>
>> @@ -2302,6 +2302,9 @@ build_target_command_list (struct bp_loc
>>                  need to parse the command to bytecodes again.  */
>>               aexpr = parse_cmd_to_aexpr (bl->address,
>>                                           loc->owner->extra_string);
>> +             if (aexpr == NULL)
>> +               error (_("Agent is not support commands of breakpoint
>> %d."),
>> +                      bl->owner->number);
>>               loc->cmd_bytecode = aexpr;
>>
>>               if (!aexpr)
>
>
> The existing code has considered the situation that we have a NULL bytecode
> expression.  Several lines of code below this line of patch do something,
>
>           /* If we have a NULL bytecode expression, it means something
>              went wrong or we have a null command expression.  */
>           if (!loc->cmd_bytecode)
>             {
>               null_command_or_parse_error = 1;
>               break;
>             }
>         }
>     }
>
>   /* If anything failed, then we're not doing target-side commands,
>      and so clean up.  */
>   if (null_command_or_parse_error)
>
> Your patch doesn't look right, and probably we need to examine why dprintf
> breakpoint is not handled well in this part.

I think the design of current build_target_command_list is not right.
It use null_command_or_parse_error that close with
build_target_condition_list.

For the condition, the commands of agent is same with gdb side.  So
when it got fail or NULL.  Agent can go back to let GDB handle this
condition.  So it have null_condition_or_parse_error.

But for commands, the commands of agent is not support by GDB side
(agent-printf).  So even if it go back to GDB, it still cannot be
handle.  So I think throw error in build_target_command_list is
better.

I will post patch to fix it later.

Thanks,
Hui

Re: [PATCH] Fix bug 15433 - GDB crashes when using agent dprintf, %s format, and an in-line string

[Tom Tromey]

>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:

Hui> +	      if (aexpr == NULL)
Hui> +		error (_("Agent is not support commands of breakpoint %d."),
Hui> +		       bl->owner->number);

In addition to what Yao said, this change seems roughly equivalent to
simply removing the TRY_CATCH from parse_cmd_to_aexpr.  (You'd have to
clear loc->cmd_bytecode first, but it seems to me that this has to be
done anyway, because parse_cmd_to_aexpr can already call error...)

I also still don't understand the code to set null_command_or_parse_error.
Maybe you explained it to me last time, but I forget.  If it is actually
correct as-is, I would appreciate a comment explaining it.

Tom

Re: [PATCH] Fix bug 15433 - GDB crashes when using agent dprintf, %s format, and an in-line string

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]

Hi Tom,

Thanks for your review.

On Tue, May 14, 2013 at 12:35 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>
> Hui> +        if (aexpr == NULL)
> Hui> +          error (_("Agent is not support commands of breakpoint %d."),
> Hui> +                 bl->owner->number);
>
> In addition to what Yao said, this change seems roughly equivalent to
> simply removing the TRY_CATCH from parse_cmd_to_aexpr.  (You'd have to
> clear loc->cmd_bytecode first, but it seems to me that this has to be
> done anyway, because parse_cmd_to_aexpr can already call error...)

Agree with you.  Updated patches for it.

>
> I also still don't understand the code to set null_command_or_parse_error.
> Maybe you explained it to me last time, but I forget.  If it is actually
> correct as-is, I would appreciate a comment explaining it.

I posted some introduction about it in
http://sourceware.org/ml/gdb-patches/2013-05/msg00425.html .  I hope
it will be helpful to you to understand this function.
And I suggest we can discussion this function in thread
http://sourceware.org/ml/gdb-patches/2013-04/msg00839.html ([PATCH/v2]
fix Bug 15180 Agent style dprintf does not respect conditions, I will
ping it later) that I did a lot of change about this function.

Thanks,
Hui

2013-05-14  Hui Zhu  <hui@codesourcery.com>

	PR gdb/15433
	* breakpoint.c (parse_cmd_to_aexpr): Remove TRY_CATCH for gen_printf.

2013-05-14  Hui Zhu  <hui@codesourcery.com>

	PR gdb/15433
	* gdb.base/dprintf.exp: Test unsupport commands on target.

[-- Attachment #2: dprintf-cmd-not-support.txt --]
[-- Type: text/plain, Size: 828 bytes --]

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2239,22 +2239,9 @@ parse_cmd_to_aexpr (CORE_ADDR scope, cha
 	++cmdrest;
     }
 
-  /* We don't want to stop processing, so catch any errors
-     that may show up.  */
-  TRY_CATCH (ex, RETURN_MASK_ERROR)
-    {
-      aexpr = gen_printf (scope, gdbarch, 0, 0,
-			  format_start, format_end - format_start,
-			  fpieces, nargs, argvec);
-    }
-
-  if (ex.reason < 0)
-    {
-      /* If we got here, it means the command could not be parsed to a valid
-	 bytecode expression and thus can't be evaluated on the target's side.
-	 It's no use iterating through the other commands.  */
-      return NULL;
-    }
+  aexpr = gen_printf (scope, gdbarch, 0, 0,
+		      format_start, format_end - format_start,
+		      fpieces, nargs, argvec);
 
   do_cleanups (old_cleanups);
 

[-- Attachment #3: dprintf-cmd-not-support-test.txt --]
[-- Type: text/plain, Size: 531 bytes --]

--- a/gdb/testsuite/gdb.base/dprintf.exp
+++ b/gdb/testsuite/gdb.base/dprintf.exp
@@ -131,6 +131,11 @@ if $target_can_dprintf {
 	"\[\r\n\]\tbreakpoint already hit 2 times"
 	"\[\r\n\]        agent-printf \"arg=%d, g=%d\\\\n\", arg, g"
     }
+
+    # Test unsupport commands.
+    gdb_test "dprintf $dp_location1,\"%s\\n\", \"test\"" "Dprintf .*"
+    gdb_test "continue" ".*Unsupported operator OP_STRING .* in expression.*" \
+      "3rd dprintf, agent"
 }
 
 gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \

Re: [PATCH] Fix bug 15433 - GDB crashes when using agent dprintf, %s format, and an in-line string

[Tom Tromey]

>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:

Hui> 2013-05-14  Hui Zhu  <hui@codesourcery.com>
Hui> 	PR gdb/15433
Hui> 	* breakpoint.c (parse_cmd_to_aexpr): Remove TRY_CATCH for gen_printf.

It isn't clear to me if this patch is still pending; mostly my fault, I
think, for letting patch review slide so long.  But still, for clarity,
could you say which dprintf patches still need review?  That would help
a lot.

thanks,
Tom