[RFA 2/3] Demote to sw watchpoint only in update_watchpoint

Thiago Jung Bauermann
2011-04-18T21:22:50Z

Hi,

watch_command_1 duplicates logic from update_watchpoint to decide
whether the newly created watchpoint should be a software watchpoint or
a hardware watchpoint, and also to error out if trying to set up a read
or access watchpoint without having debug registers available.

I was trying to adapt the logic in both places for masked watchpoints,
but it's hard to anticipate in watch_command_1 what update_watchpoint
will ultimately do without duplicating a good portion of the latter. The
code in watch_command_1 was getting complicated and hard to get right in
all situations for something which would be done again later anyway.

I decided that it was cleaner if watch_command_1 just delegated
everything to update_watchpoint. This patch makes it create a
watchpoint, call update_watchpoint and then delete it if some error is
hit. The only drawback I can think of is that the aborted watchpoint
will "consume" one breakpoint number, and thus GDB will skip one number
when creating the next breakpoint/watchpoint. This doesn't seem
important to me.

Tested without regressions on ppc-linux, ppc64-linux and i386-linux. Ok?
--
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

2011-04-18 Thiago Jung Bauermann <bauerman@br.ibm.com>

* breakpoint.c (update_watchpoint): Change between software and
hardware watchpoint for all kinds of watchpoints, not just
read/write ones. Determine b->exact value here instead of
in watch_command_1. Error out if there are not enough resources
for a read or access hardware watchpoint.
(watch_command_1): Remove logic of checking whether there are
enough resources available, since update_watchpoint will do that
work now. Don't set b->exact here. Catch exceptions thrown by
update_watchpoint and delete the watchpoint.
(can_use_hardware_watchpoint): Remove exact_watchpoints argument.
Use target_exact_watchpoints instead.
(delete_breakpoint): Add announce argument to control whether
observers are notified of the deletion. Update all callers.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 744057a..2bfdfb0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -101,7 +101,7 @@ static void clear_command (char *, int);

static void catch_command (char *, int);

-static int can_use_hardware_watchpoint (struct value *, int);
+static int can_use_hardware_watchpoint (struct value *);

static void break_command_1 (char *, int, int);

@@ -1404,19 +1404,22 @@ update_watchpoint (struct breakpoint *b, int reparse)
an ordinary watchpoint depending on the hardware support
and free hardware slots. REPARSE is set when the inferior
is started. */
- if ((b->type == bp_watchpoint || b->type == bp_hardware_watchpoint)
- && reparse)
+ if (reparse)
{
int reg_cnt;
enum bp_loc_type loc_type;
struct bp_location *bl;

- reg_cnt = can_use_hardware_watchpoint (val_chain, b->exact);
+ reg_cnt = can_use_hardware_watchpoint (val_chain);

if (reg_cnt)
{
int i, target_resources_ok, other_type_used;

+ /* Use an exact watchpoint when there's only one memory region to be
+ watched, and only one debug register is needed to watch it. */
+ b->exact = target_exact_watchpoints && reg_cnt == 1;
+
/* We need to determine how many resources are already
used for all other hardware watchpoints plus this one
to see if we still have enough resources to also fit
@@ -1424,7 +1427,8 @@ update_watchpoint (struct breakpoint *b, int reparse)
hw_watchpoint_used_count call below counts this
watchpoint, make sure that it is marked as a hardware
watchpoint. */
- b->type = bp_hardware_watchpoint;
+ if (b->type == bp_watchpoint)
+ b->type = bp_hardware_watchpoint;

i = hw_watchpoint_used_count (bp_hardware_watchpoint,
&other_type_used);
@@ -1432,8 +1436,22 @@ update_watchpoint (struct breakpoint *b, int reparse)
target_resources_ok = target_can_use_hardware_watchpoint
(bp_hardware_watchpoint, i, other_type_used);
if (target_resources_ok <= 0)
- b->type = bp_watchpoint;
+ {
+ if (target_resources_ok == 0
+ && b->type != bp_hardware_watchpoint)
+ error (_("Target does not support this type of "
+ "hardware watchpoint."));
+ else if (target_resources_ok < 0
+ && b->type != bp_hardware_watchpoint)
+ error (_("Target can only support one kind "
+ "of HW watchpoint at a time."));
+ else
+ b->type = bp_watchpoint;
+ }
}
+ else if (b->type != bp_hardware_watchpoint)
+ error (_("Expression cannot be implemented with "
+ "read/access watchpoint."));
else
b->type = bp_watchpoint;

@@ -1796,7 +1814,7 @@ breakpoint_program_space_exit (struct program_space *pspace)
ALL_BREAKPOINTS_SAFE (b, b_temp)
{
if (b->pspace == pspace)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}

/* Breakpoints set through other program spaces could have locations
@@ -2382,14 +2400,14 @@ update_breakpoints_after_exec (void)
/* Solib breakpoints must be explicitly reset after an exec(). */
if (b->type == bp_shlib_event)
{
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
continue;
}

/* JIT breakpoints must be explicitly reset after an exec(). */
if (b->type == bp_jit_event)
{
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
continue;
}

@@ -2399,14 +2417,14 @@ update_breakpoints_after_exec (void)
|| b->type == bp_longjmp_master || b->type == bp_std_terminate_master
|| b->type == bp_exception_master)
{
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
continue;
}

/* Step-resume breakpoints are meaningless after an exec(). */
if (b->type == bp_step_resume)
{
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
continue;
}

@@ -2415,7 +2433,7 @@ update_breakpoints_after_exec (void)
if (b->type == bp_longjmp || b->type == bp_longjmp_resume
|| b->type == bp_exception || b->type == bp_exception_resume)
{
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
continue;
}

@@ -2464,7 +2482,7 @@ update_breakpoints_after_exec (void)
a.out. */
if (b->addr_string == NULL)
{
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
continue;
}
}
@@ -2736,7 +2754,7 @@ breakpoint_init_inferior (enum inf_context context)
(gdb) tar rem :9999 # remote Windows gdbserver.
*/

- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
break;

case bp_watchpoint:
@@ -2746,7 +2764,7 @@ breakpoint_init_inferior (enum inf_context context)

/* Likewise for watchpoints on local expressions. */
if (b->exp_valid_block != NULL)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
else if (context == inf_starting)
{
/* Reset val field to force reread of starting value in
@@ -5970,7 +5988,7 @@ delete_longjmp_breakpoint (int thread)
if (b->type == bp_longjmp || b->type == bp_exception)
{
if (b->thread == thread)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}
}

@@ -6026,7 +6044,7 @@ delete_std_terminate_breakpoint (void)

ALL_BREAKPOINTS_SAFE (b, b_tmp)
if (b->type == bp_std_terminate)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}

struct breakpoint *
@@ -6054,7 +6072,7 @@ remove_thread_event_breakpoints (void)
ALL_BREAKPOINTS_SAFE (b, b_tmp)
if (b->type == bp_thread_event
&& b->loc->pspace == current_program_space)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}

struct lang_and_radix
@@ -6085,7 +6103,7 @@ remove_jit_event_breakpoints (void)
ALL_BREAKPOINTS_SAFE (b, b_tmp)
if (b->type == bp_jit_event
&& b->loc->pspace == current_program_space)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}

void
@@ -6096,7 +6114,7 @@ remove_solib_event_breakpoints (void)
ALL_BREAKPOINTS_SAFE (b, b_tmp)
if (b->type == bp_shlib_event
&& b->loc->pspace == current_program_space)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}

struct breakpoint *
@@ -8789,6 +8807,7 @@ static void
watch_command_1 (char *arg, int accessflag, int from_tty,
int just_location, int internal)
{
+ volatile struct gdb_exception e;
struct breakpoint *b, *scope_breakpoint = NULL;
struct expression *exp;
struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL;
@@ -8800,9 +8819,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
int toklen;
char *cond_start = NULL;
char *cond_end = NULL;
- int i, other_type_used, target_resources_ok = 0;
enum bptype bp_type;
- int reg_cnt = 0;
int thread = -1;
int pc = 0;

@@ -8932,28 +8949,6 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
else
bp_type = bp_hardware_watchpoint;

- reg_cnt = can_use_hardware_watchpoint (val, target_exact_watchpoints);
- if (reg_cnt == 0 && bp_type != bp_hardware_watchpoint)
- error (_("Expression cannot be implemented with read/access watchpoint."));
- if (reg_cnt != 0)
- {
- i = hw_watchpoint_used_count (bp_type, &other_type_used);
- target_resources_ok =
- target_can_use_hardware_watchpoint (bp_type, i + reg_cnt,
- other_type_used);
- if (target_resources_ok == 0 && bp_type != bp_hardware_watchpoint)
- error (_("Target does not support this type of hardware watchpoint."));
-
- if (target_resources_ok < 0 && bp_type != bp_hardware_watchpoint)
- error (_("Target can only support one kind "
- "of HW watchpoint at a time."));
- }
-
- /* Change the type of breakpoint to an ordinary watchpoint if a
- hardware watchpoint could not be set. */
- if (!reg_cnt || target_resources_ok <= 0)
- bp_type = bp_watchpoint;
-
frame = block_innermost_frame (exp_valid_block);

/* If the expression is "local", then set up a "watchpoint scope"
@@ -9022,10 +9017,6 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
b->val_valid = 1;
b->ops = &watchpoint_breakpoint_ops;

- /* Use an exact watchpoint when there's only one memory region to be
- watched, and only one debug register is needed to watch it. */
- b->exact = target_exact_watchpoints && reg_cnt == 1;
-
if (cond_start)
b->cond_string = savestring (cond_start, cond_end - cond_start);
else
@@ -9053,9 +9044,18 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
if (!just_location)
value_free_to_mark (mark);

- /* Finally update the new watchpoint. This creates the locations
- that should be inserted. */
- update_watchpoint (b, 1);
+ TRY_CATCH (e, RETURN_MASK_ALL)
+ {
+ /* Finally update the new watchpoint. This creates the locations
+ that should be inserted. */
+ update_watchpoint (b, 1);
+ }
+ if (e.reason < 0)
+ {
+ delete_breakpoint (b, 0);
+ throw_exception (e);
+ }
+
if (internal)
/* Do not mention breakpoints with a negative number, but do
notify observers. */
@@ -9066,14 +9066,10 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
}

/* Return count of debug registers needed to watch the given expression.
- If EXACT_WATCHPOINTS is 1, then consider that only the address of
- the start of the watched region will be monitored (i.e., all accesses
- will be aligned). This uses less debug registers on some targets.
-
If the watchpoint cannot be handled in hardware return zero. */

static int
-can_use_hardware_watchpoint (struct value *v, int exact_watchpoints)
+can_use_hardware_watchpoint (struct value *v)
{
int found_memory_cnt = 0;
struct value *head = v;
@@ -9129,7 +9125,7 @@ can_use_hardware_watchpoint (struct value *v, int exact_watchpoints)
int len;
int num_regs;

- len = (exact_watchpoints
+ len = (target_exact_watchpoints
&& is_scalar_type_recursive (vtype))?
1 : TYPE_LENGTH (value_type (v));

@@ -9245,9 +9241,9 @@ until_break_command_continuation (void *arg)
{
struct until_break_command_continuation_args *a = arg;

- delete_breakpoint (a->breakpoint);
+ delete_breakpoint (a->breakpoint, 1);
if (a->breakpoint2)
- delete_breakpoint (a->breakpoint2);
+ delete_breakpoint (a->breakpoint2, 1);
delete_longjmp_breakpoint (a->thread_num);
}

@@ -9976,7 +9972,7 @@ clear_command (char *arg, int from_tty)
{
if (from_tty)
printf_unfiltered ("%d ", b->number);
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}
if (from_tty)
putchar_unfiltered ('\n');
@@ -9995,12 +9991,12 @@ breakpoint_auto_delete (bpstat bs)
if (bs->breakpoint_at
&& bs->breakpoint_at->disposition == disp_del
&& bs->stop)
- delete_breakpoint (bs->breakpoint_at);
+ delete_breakpoint (bs->breakpoint_at, 1);

ALL_BREAKPOINTS_SAFE (b, b_tmp)
{
if (b->disposition == disp_del_at_next_stop)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}
}

@@ -10441,10 +10437,11 @@ bpstat_remove_breakpoint_callback (struct thread_info *th, void *data)
}

/* Delete a breakpoint and clean up all traces of it in the data
- structures. */
+ structures. If ANNOUNCE is 1, then notifies observers that
+ the breakpoint was deleted. */

void
-delete_breakpoint (struct breakpoint *bpt)
+delete_breakpoint (struct breakpoint *bpt, int announce)
{
struct breakpoint *b;

@@ -10487,7 +10484,8 @@ delete_breakpoint (struct breakpoint *bpt)
bpt->related_breakpoint = bpt;
}

- observer_notify_breakpoint_deleted (bpt->number);
+ if (announce)
+ observer_notify_breakpoint_deleted (bpt->number);

if (breakpoint_chain == bpt)
breakpoint_chain = bpt->next;
@@ -10544,7 +10542,7 @@ delete_breakpoint (struct breakpoint *bpt)
static void
do_delete_breakpoint_cleanup (void *b)
{
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}

struct cleanup *
@@ -10559,7 +10557,7 @@ make_cleanup_delete_breakpoint (struct breakpoint *b)
static void
do_delete_breakpoint (struct breakpoint *b, void *ignore)
{
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}

void
@@ -10610,7 +10608,7 @@ delete_command (char *arg, int from_tty)
&& b->type != bp_std_terminate_master
&& b->type != bp_exception_master
&& b->number >= 0)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}
}
}
@@ -11073,7 +11071,7 @@ breakpoint_re_set_one (void *bint)
if (b->addr_string == NULL)
{
/* Anything without a string can't be re-set. */
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
return 0;
}

@@ -11127,7 +11125,7 @@ breakpoint_re_set_one (void *bint)
case bp_longjmp_master:
case bp_std_terminate_master:
case bp_exception_master:
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
break;

/* This breakpoint is special, it's set up when the inferior
@@ -12114,7 +12112,7 @@ delete_trace_command (char *arg, int from_tty)
{
if (is_tracepoint (b)
&& b->number >= 0)
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
}
}
}
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 7a9c2d4..bf827e5 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -953,7 +953,7 @@ extern void breakpoint_init_inferior (enum inf_context);

extern struct cleanup *make_cleanup_delete_breakpoint (struct breakpoint *);

-extern void delete_breakpoint (struct breakpoint *);
+extern void delete_breakpoint (struct breakpoint *, int);

extern void breakpoint_auto_delete (bpstat);

diff --git a/gdb/elfread.c b/gdb/elfread.c
index 2d589a4..277a093 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1036,7 +1036,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
case bp_gnu_ifunc_resolver:
break;
case bp_gnu_ifunc_resolver_return:
- delete_breakpoint (b);
+ delete_breakpoint (b, 1);
break;
default:
internal_error (__FILE__, __LINE__,
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 3dc13e3..f0e9d96 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1501,7 +1501,7 @@ finish_command_continuation (void *arg)
that the *stopped notification includes the return value. */
if (bs != NULL && tp->control.proceed_to_finish)
observer_notify_normal_stop (bs, 1 /* print frame */);
- delete_breakpoint (a->breakpoint);
+ delete_breakpoint (a->breakpoint, 1);
delete_longjmp_breakpoint (inferior_thread ()->num);
}

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 0c21bfc..4450e60 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -302,7 +302,7 @@ bppy_delete_breakpoint (PyObject *self, PyObject *args)

BPPY_REQUIRE_VALID (self_bp);

- delete_breakpoint (self_bp->bp);
+ delete_breakpoint (self_bp->bp, 1);

Py_RETURN_NONE;
}
diff --git a/gdb/thread.c b/gdb/thread.c
index 6ad1807..929c335 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -87,7 +87,7 @@ delete_step_resume_breakpoint (struct thread_info *tp)
{
if (tp && tp->control.step_resume_breakpoint)
{
- delete_breakpoint (tp->control.step_resume_breakpoint);
+ delete_breakpoint (tp->control.step_resume_breakpoint, 1);
tp->control.step_resume_breakpoint = NULL;
}
}
@@ -97,7 +97,7 @@ delete_exception_resume_breakpoint (struct thread_info *tp)
{
if (tp && tp->control.exception_resume_breakpoint)
{
- delete_breakpoint (tp->control.exception_resume_breakpoint);
+ delete_breakpoint (tp->control.exception_resume_breakpoint, 1);
tp->control.exception_resume_breakpoint = NULL;
}
}