[PATCH/v4 1/3] fix Bug 15180 Agent style dprintf does not respect conditions (gdb)
Hui Zhu
hui_zhu@mentor.com
Mon Jan 27 15:35:00 GMT 2014
This patch doesn't has big chanage with previous version. Just update
follow git upstream.
There are a lot of thread around this bug that make all these patches
difficult to review. So I re-send the patches for this issue.
the core issue of this bug is the command is not well grouped or associated
with condition in target side.
And after I check the code of GDB, I found that in gdb part, when it
build target commands list (build_target_command_list) and condition
list (build_target_condition_list) to bl->target_info, they do not
good grouped commands and condition. Also they have another issue for
example, not handle disable breakpoints very well.
So I merge the two functions together to function
build_target_command_list_and_condition_list.
It will do check for all the commands and conditions in a address.
After that, push commands and conditions bytecodes to target_info. It
will push NULL to it if need.
With this NULL, because commands and conditions have the same number,
then it can be grouped.
In the end of this function, it will pust a NULL conditions, it can
handle the BL has commands on target but always need let GDB handle
the conditions.
After that, I updated remote_add_target_side_commands and
remote_add_target_side_condition. Then if it got NULL condition or
commands. It will send bytecodes with 0 size to gdbserver.
gdb still have issue if a breakpoints with conditions and dprintf with
condions. so add function dprintf_breakpoint_hit for it.
Thanks,
Hui
2014-01-27 Hui Zhu <hui@codesourcery.com>
PR gdb/15180
* breakpoint.c (build_target_condition_list,
build_target_command_list): Removed.
(build_target_condition_list_first, build_target_command_list_first,
build_target_command_list_and_condition_list): New functions.
(insert_bp_location): Call
build_target_command_list_and_condition_list.
(dprintf_breakpoint_hit): New function.
(initialize_breakpoint_ops): Set dprintf_breakpoint_hit.
* remote.c (remote_add_target_side_condition): Handle NULL.
(remote_add_target_side_commands): Ditto.
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2081,19 +2081,19 @@ parse_cond_to_aexpr (CORE_ADDR scope, st
return aexpr;
}
-/* Based on location BL, create a list of breakpoint conditions to be
- passed on to the target. If we have duplicated locations with different
- conditions, we will add such conditions to the list. The idea is
that the
- target will evaluate the list of conditions and will only notify GDB
when
- one of them is true. */
+/* Do a first pass to check for location and duplicated locations with
+ different conditions.
+ Return true, if all locations do not have commands. Otherwise,
+ return false. */
-static void
-build_target_condition_list (struct bp_location *bl)
+static int
+build_target_condition_list_first (struct bp_location *bl, int
empty_commands,
+ int *back_to_gdb)
{
struct bp_location **locp = NULL, **loc2p;
int null_condition_or_parse_error = 0;
- int modified = bl->needs_update;
struct bp_location *loc;
+ int empty_conditions = 1;
/* Release conditions left over from a previous insert. */
VEC_free (agent_expr_p, bl->target_info.conditions);
@@ -2104,19 +2104,14 @@ build_target_condition_list (struct bp_l
side. */
if (gdb_evaluates_breakpoint_condition_p ()
|| !target_supports_evaluation_of_breakpoint_conditions ())
- return;
+ return empty_conditions;
- /* Do a first pass to check for locations with no assigned
- conditions or conditions that fail to parse to a valid agent
expression
- bytecode. If any of these happen, then it's no use to send conditions
- to the target since this location will always trigger and generate a
- response back to GDB. */
ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
{
loc = (*loc2p);
if (is_breakpoint (loc->owner) && loc->pspace->num ==
bl->pspace->num)
{
- if (modified)
+ if (bl->needs_update)
{
struct agent_expr *aexpr;
@@ -2125,66 +2120,51 @@ build_target_condition_list (struct bp_l
force_breakpoint_reinsertion). We just
need to parse the condition to bytecodes again. */
aexpr = parse_cond_to_aexpr (bl->address, loc->cond);
- loc->cond_bytecode = aexpr;
- /* Check if we managed to parse the conditional expression
- correctly. If not, we will not send this condition
- to the target. */
- if (aexpr)
- continue;
+ if (loc->cond_bytecode)
+ free_agent_expr (loc->cond_bytecode);
+ loc->cond_bytecode = aexpr;
}
- /* If we have a NULL bytecode expression, it means something
- went wrong or we have a null condition expression. */
- if (!loc->cond_bytecode)
+ if (loc->cond_bytecode != NULL)
{
- null_condition_or_parse_error = 1;
- break;
+ /* If one loc have condition and target need condition of LOC,
+ function should return 1. */
+ if (loc->cond
+ && is_breakpoint (loc->owner)
+ && loc->pspace->num == bl->pspace->num
+ && loc->owner->enable_state == bp_enabled
+ && loc->enabled)
+ empty_conditions = 0;
}
- }
- }
-
- /* If any of these happened, it means we will have to evaluate the
conditions
- for the location's address on gdb's side. It is no use keeping
bytecodes
- for all the other duplicate locations, thus we free all of them here.
-
- This is so we have a finer control over which locations'
conditions are
- being evaluated by GDB or the remote stub. */
- if (null_condition_or_parse_error)
- {
- ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
- {
- loc = (*loc2p);
- if (is_breakpoint (loc->owner) && loc->pspace->num == bl->pspace->num)
+ else
{
- /* Only go as far as the first NULL bytecode is
- located. */
- if (!loc->cond_bytecode)
- return;
-
- free_agent_expr (loc->cond_bytecode);
- loc->cond_bytecode = NULL;
+ /* If we have a NULL bytecode expression, it means something
+ went wrong or we have a null condition expression. */
+ if (empty_commands)
+ {
+ /* If the locs do not have any commands, it means we will
+ have to evaluate the conditions for the location's
+ address on gdb's side. */
+ empty_conditions = 1;
+ break;
+ }
+ else if (loc->cmd_bytecode == NULL
+ && is_breakpoint (loc->owner)
+ && loc->owner->enable_state == bp_enabled
+ && loc->enabled)
+ {
+ /* If an enabled breakpoints that doesn't have commands
+ in target doesn't have condition too, it means target
+ will have to evaluate the conditions for the location's
+ address on gdb's side. */
+ *back_to_gdb = 1;
+ }
}
}
}
- /* No NULL conditions or failed bytecode generation. Build a
condition list
- for this location's address. */
- ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
- {
- loc = (*loc2p);
- if (loc->cond
- && is_breakpoint (loc->owner)
- && loc->pspace->num == bl->pspace->num
- && loc->owner->enable_state == bp_enabled
- && loc->enabled)
- /* Add the condition to the vector. This will be used later to send the
- conditions to the target. */
- VEC_safe_push (agent_expr_p, bl->target_info.conditions,
- loc->cond_bytecode);
- }
-
- return;
+ return empty_conditions;
}
/* Parses a command described by string CMD into an agent expression
@@ -2277,40 +2257,42 @@ parse_cmd_to_aexpr (CORE_ADDR scope, cha
return aexpr;
}
-/* Based on location BL, create a list of breakpoint commands to be
- passed on to the target. If we have duplicated locations with
- different commands, we will add any such to the list. */
+/* Do a first pass to check for location and duplicated locations with
+ different commands.
+ Parse all the commands to a valid agent expression bytecode set it to
+ LOC->CMD_BYTECODE.
+ If a location doesn't have commands, set LOC->CMD_BYTECODE to NULL.
+ Return true, if all locations do not have commands. Otherwise,
+ return false. */
-static void
-build_target_command_list (struct bp_location *bl)
+static int
+build_target_command_list_first (struct bp_location *bl)
{
struct bp_location **locp = NULL, **loc2p;
- int null_command_or_parse_error = 0;
- int modified = bl->needs_update;
struct bp_location *loc;
+ int empty_commands = 1;
/* Release commands left over from a previous insert. */
VEC_free (agent_expr_p, bl->target_info.tcommands);
- /* For now, limit to agent-style dprintf breakpoints. */
- if (bl->owner->type != bp_dprintf
- || strcmp (dprintf_style, dprintf_style_agent) != 0)
- return;
+ /* For now, limit to agent-style dprintf breakpoints.
+ Even if BL is not a dprintf breakpoint, its duplicated
+ locations maybe a dprintf breakpoint. So does check the type
+ of BL->OWNER. */
+ if (strcmp (dprintf_style, dprintf_style_agent) != 0)
+ return empty_commands;
if (!target_can_run_breakpoint_commands ())
- return;
+ return empty_commands;
- /* Do a first pass to check for locations with no assigned
- conditions or conditions that fail to parse to a valid agent
expression
- bytecode. If any of these happen, then it's no use to send conditions
- to the target since this location will always trigger and generate a
- response back to GDB. */
ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
{
loc = (*loc2p);
- if (is_breakpoint (loc->owner) && loc->pspace->num ==
bl->pspace->num)
+ if (is_breakpoint (loc->owner)
+ && loc->pspace->num == bl->pspace->num
+ && loc->owner->type == bp_dprintf)
{
- if (modified)
+ if (bl->needs_update)
{
struct agent_expr *aexpr;
@@ -2320,63 +2302,136 @@ 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);
- loc->cmd_bytecode = aexpr;
- if (!aexpr)
- continue;
+ if (loc->cmd_bytecode)
+ free_agent_expr (loc->cmd_bytecode);
+ loc->cmd_bytecode = aexpr;
}
- /* 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 target need commands of LOC.
+ current function need return false. */
+ if (loc->cmd_bytecode != NULL
+ && loc->owner->extra_string
+ && is_breakpoint (loc->owner)
+ && loc->pspace->num == bl->pspace->num
+ && loc->owner->enable_state == bp_enabled
+ && loc->enabled)
+ empty_commands = 0;
}
}
- /* If anything failed, then we're not doing target-side commands,
- and so clean up. */
- if (null_command_or_parse_error)
+ bl->target_info.persist = 0;
+ /* Maybe flag this location as persistent. */
+ if (empty_commands == 0 && disconnected_dprintf)
+ bl->target_info.persist = 1;
+
+ return empty_commands;
+}
+
+static void
+build_target_command_list_and_condition_list (struct bp_location *bl)
+{
+ int empty_commands, empty_conditions, back_to_gdb = 0;
+ struct bp_location **locp = NULL, **loc2p;
+ struct bp_location *loc;
+ static struct agent_expr null_expr =
{
- ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
+ NULL,
+ 0,
+ 0,
+ NULL,
+ 0,
+ 0,
+ 0,
+ 0,0,
+ 0,
+ 0,
+ 0,
+ 0,
+ 0,
+ };
+
+ empty_commands = build_target_command_list_first (bl);
+ empty_conditions = build_target_condition_list_first (bl, empty_commands,
+ &back_to_gdb);
+
+ if (empty_commands && empty_conditions)
+ return;
+
+ /* Do the second pass to check for location and duplicated locations
+ and push the condition bytecodes and commands bytecodes to
+ BL->TARGET_INFO in same order.
+ Make the condition bytecodes and commands bytecodes of a breakpoint
+ have same sequence number in BL->TARGET_INFO.CONDITIONS and
+ BL->TARGET_INFO.TCOMMANDS. Then they will be send to target in same
+ sequence number. And target will get the right condition is for the
+ each commands. */
+ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
+ {
+ loc = (*loc2p);
+ if (is_breakpoint (loc->owner) && loc->pspace->num ==
bl->pspace->num)
{
- loc = (*loc2p);
- if (is_breakpoint (loc->owner)
- && loc->pspace->num == bl->pspace->num)
+ struct agent_expr *cond_bytecode = &null_expr;
+ struct agent_expr *cmd_bytecode = &null_expr;
+
+ if (!empty_commands
+ && loc->owner->extra_string
+ && is_breakpoint (loc->owner)
+ && loc->pspace->num == bl->pspace->num
+ && loc->owner->enable_state == bp_enabled
+ && loc->enabled)
+ cmd_bytecode = loc->cmd_bytecode;
+
+ if (!empty_conditions
+ && loc->cond
+ && is_breakpoint (loc->owner)
+ && loc->pspace->num == bl->pspace->num
+ && loc->owner->enable_state == bp_enabled
+ && loc->enabled
+ && !(cmd_bytecode == NULL && back_to_gdb))
+ cond_bytecode = loc->cond_bytecode;
+
+ if (cond_bytecode == &null_expr && cmd_bytecode == &null_expr)
+ continue;
+
+ /* Push commands.
+ If all the commands in this address is 0 size, it doesn't
+ need send 0 size commands bytecode to target. */
+ if (!empty_commands)
{
- /* Only go as far as the first NULL bytecode is
- located. */
- if (loc->cmd_bytecode == NULL)
- return;
+ /* If doesn't have any conditions in this address, it doesn't
+ needn't 0 size commands for the sequence number. */
+ if (cmd_bytecode != &null_expr || !empty_conditions)
+ VEC_safe_push (agent_expr_p, bl->target_info.tcommands,
+ cmd_bytecode);
+ }
- free_agent_expr (loc->cmd_bytecode);
- loc->cmd_bytecode = NULL;
+ /* Push codition.
+ If all the conditions in this address is 0 size, it doesn't
+ need send 0 size condition bytecode to target. */
+ if (!empty_conditions)
+ {
+ /* If doesn't have any commands in this address, it doesn't
+ needn't 0 size condition for the sequence number. */
+ if (cond_bytecode != &null_expr || !empty_commands)
+ VEC_safe_push (agent_expr_p, bl->target_info.conditions,
+ cond_bytecode);
}
}
}
- /* No NULL commands or failed bytecode generation. Build a command list
- for this location's address. */
- ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address)
+ if (back_to_gdb)
{
- loc = (*loc2p);
- if (loc->owner->extra_string
- && is_breakpoint (loc->owner)
- && loc->pspace->num == bl->pspace->num
- && loc->owner->enable_state == bp_enabled
- && loc->enabled)
- /* Add the command to the vector. This will be used later
- to send the commands to the target. */
- VEC_safe_push (agent_expr_p, bl->target_info.tcommands,
- loc->cmd_bytecode);
+ /* Because this BL has commands on target but always need let GDB
+ handle the conditions, so we push a NULL commands and NULL
+ conditionlet target know this BL need back to GDB side. */
+ if (!empty_commands)
+ VEC_safe_push (agent_expr_p, bl->target_info.tcommands, &null_expr);
+ if (!empty_conditions)
+ VEC_safe_push (agent_expr_p, bl->target_info.conditions, &null_expr);
}
- bl->target_info.persist = 0;
- /* Maybe flag this location as persistent. */
- if (bl->owner->type == bp_dprintf && disconnected_dprintf)
- bl->target_info.persist = 1;
+ bl->needs_update = 0;
}
/* Insert a low-level "breakpoint" of some type. BL is the breakpoint
@@ -2420,12 +2475,7 @@ insert_bp_location (struct bp_location *
can decide when to stop and notify GDB. */
if (is_breakpoint (bl->owner))
- {
- build_target_condition_list (bl);
- build_target_command_list (bl);
- /* Reset the modification marker. */
- bl->needs_update = 0;
- }
+ build_target_command_list_and_condition_list (bl);
if (bl->loc_type == bp_loc_software_breakpoint
|| bl->loc_type == bp_loc_hardware_breakpoint)
@@ -13522,6 +13572,19 @@ tracepoint_probe_decode_linespec (struct
static struct breakpoint_ops tracepoint_probe_breakpoint_ops;
+/* Implement the "breakpoint_hit" breakpoint_ops method for dprintf. */
+
+static int
+dprintf_breakpoint_hit (const struct bp_location *bl,
+ struct address_space *aspace, CORE_ADDR bp_addr,
+ const struct target_waitstatus *ws)
+{
+ if (strcmp (dprintf_style, dprintf_style_agent) != 0)
+ return bkpt_breakpoint_hit (bl, aspace, bp_addr, ws);
+
+ return 0;
+}
+
/* Dprintf breakpoint_ops methods. */
static void
@@ -16110,6 +16173,7 @@ initialize_breakpoint_ops (void)
ops = &dprintf_breakpoint_ops;
*ops = bkpt_base_breakpoint_ops;
+ ops->breakpoint_hit = dprintf_breakpoint_hit;
ops->re_set = dprintf_re_set;
ops->resources_needed = bkpt_resources_needed;
ops->print_it = bkpt_print_it;
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8208,10 +8208,18 @@ remote_add_target_side_condition (struct
VEC_iterate (agent_expr_p, bp_tgt->conditions, ix, aexpr);
ix++)
{
- xsnprintf (buf, buf_end - buf, "X%x,", aexpr->len);
- buf += strlen (buf);
- for (i = 0; i < aexpr->len; ++i)
- buf = pack_hex_byte (buf, aexpr->buf[i]);
+ if (aexpr)
+ {
+ xsnprintf (buf, buf_end - buf, "X%x,", aexpr->len);
+ buf += strlen (buf);
+ for (i = 0; i < aexpr->len; ++i)
+ buf = pack_hex_byte (buf, aexpr->buf[i]);
+ }
+ else
+ {
+ xsnprintf (buf, buf_end - buf, "X0,");
+ buf += strlen (buf);
+ }
*buf = '\0';
}
return 0;
@@ -8229,19 +8237,27 @@ remote_add_target_side_commands (struct
buf += strlen (buf);
- sprintf (buf, ";cmds:%x,", bp_tgt->persist);
- buf += strlen (buf);
-
/* Concatenate all the agent expressions that are commands into the
cmds parameter. */
for (ix = 0;
VEC_iterate (agent_expr_p, bp_tgt->tcommands, ix, aexpr);
ix++)
{
- sprintf (buf, "X%x,", aexpr->len);
+ sprintf (buf, ";cmds:%x,", bp_tgt->persist);
buf += strlen (buf);
- for (i = 0; i < aexpr->len; ++i)
- buf = pack_hex_byte (buf, aexpr->buf[i]);
+
+ if (aexpr)
+ {
+ sprintf (buf, "X%x,", aexpr->len);
+ buf += strlen (buf);
+ for (i = 0; i < aexpr->len; ++i)
+ buf = pack_hex_byte (buf, aexpr->buf[i]);
+ }
+ else
+ {
+ sprintf (buf, "X0,");
+ buf += strlen (buf);
+ }
*buf = '\0';
}
}
More information about the Gdb-patches
mailing list