* [PATCH] change the arguments of create_breakpoint to a struct
@ 2013-02-21 5:40 Hui Zhu
2013-02-21 13:58 ` Sergio Durigan Junior
0 siblings, 1 reply; 3+ messages in thread
From: Hui Zhu @ 2013-02-21 5:40 UTC (permalink / raw)
To: gdb-patches ml, insight; +Cc: Pedro Alves, Tom Tromey, Stan Shebs, Keith Seitz
[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]
Hi,
According to the discussion in
http://sourceware.org/ml/gdb/2012-08/msg00001.html
I post a new patch for GDB to change the arguments of
create_breakpoint to a struct.
And I also add a function create_breakpoint to Initialize all the
element of this struct to its default value. Then if we want to add
more argument to this function, we can just add the default value
Initialize code to this function and don't need update all the
function that call create_breakpoint.
And I post a patch for insight to change the arguments of
create_breakpoint to a struct.
Please help me review it.
Thanks,
Hui
2013-02-21 Hui Zhu <hui_zhu@mentor.com>
* breakpoint.c (create_breakpoint_init): New function.
(create_breakpoint): Change arguments to cb.
(break_command_1): Call function create_breakpoint_init.
Change arguments of create_breakpoint to cb.
(dprintf_command): Ditto.
(handle_gnu_v3_exceptions): Ditto.
(trace_command): Ditto.
(ftrace_command): Ditto.
(strace_command): Ditto.
(create_tracepoint_from_upload): Ditto.
* breakpoint.h (create_breakpoint_s): New struct.
(create_breakpoint_init): New extern.
(create_breakpoint): Change arguments to cb.
* mi/mi-cmd-break.c (mi_cmd_break_insert): Call function
create_breakpoint_init.
Change arguments of create_breakpoint to cb.
* python/py-breakpoint.c (bppy_init): Ditto.
* python/py-finishbreakpoint.c (bpfinishpy_init): Ditto.
* spu-tdep.c (spu_catch_start): Ditto.
2013-02-21 Hui Zhu <hui_zhu@mentor.com>
* generic/gdbtk-bp.c (gdb_set_bp): Call function
create_breakpoint_init.
Change arguments of create_breakpoint to cb.
[-- Attachment #2: create_breakpoint.txt --]
[-- Type: text/plain, Size: 22758 bytes --]
--- a/breakpoint.c
+++ b/breakpoint.c
@@ -9493,32 +9493,27 @@ decode_static_tracepoint_spec (char **ar
return sals;
}
+/* Initialize all the element of CB to its default value. */
+
+void
+create_breakpoint_init (struct create_breakpoint_s *cb)
+{
+ memset (cb, '\0', sizeof (*cb));
+ cb->type_wanted = bp_breakpoint;
+ cb->pending_break_support = AUTO_BOOLEAN_TRUE;
+ cb->enabled = 1;
+}
+
/* Set a breakpoint. This function is shared between CLI and MI
- functions for setting a breakpoint. This function has two major
- modes of operations, selected by the PARSE_CONDITION_AND_THREAD
- parameter. If non-zero, the function will parse arg, extracting
- breakpoint location, address and thread. Otherwise, ARG is just
- the location of breakpoint, with condition and thread specified by
- the COND_STRING and THREAD parameters. If INTERNAL is non-zero,
- the breakpoint number will be allocated from the internal
- breakpoint count. Returns true if any breakpoint was created;
- false otherwise. */
+ functions for setting a breakpoint.
+ Returns true if any breakpoint was created; false otherwise. */
int
-create_breakpoint (struct gdbarch *gdbarch,
- char *arg, char *cond_string,
- int thread, char *extra_string,
- int parse_condition_and_thread,
- int tempflag, enum bptype type_wanted,
- int ignore_count,
- enum auto_boolean pending_break_support,
- const struct breakpoint_ops *ops,
- int from_tty, int enabled, int internal,
- unsigned flags)
+create_breakpoint (struct create_breakpoint_s *cb)
{
volatile struct gdb_exception e;
char *copy_arg = NULL;
- char *addr_start = arg;
+ char *addr_start = cb->arg;
struct linespec_result canonical;
struct cleanup *old_chain;
struct cleanup *bkpt_chain = NULL;
@@ -9526,14 +9521,15 @@ create_breakpoint (struct gdbarch *gdbar
int task = 0;
int prev_bkpt_count = breakpoint_count;
- gdb_assert (ops != NULL);
+ gdb_assert (cb->ops != NULL);
init_linespec_result (&canonical);
TRY_CATCH (e, RETURN_MASK_ALL)
{
- ops->create_sals_from_address (&arg, &canonical, type_wanted,
- addr_start, ©_arg);
+ cb->ops->create_sals_from_address (&cb->arg, &canonical,
+ cb->type_wanted,
+ addr_start, ©_arg);
}
/* If caller is interested in rc value from parse, set value. */
@@ -9551,16 +9547,16 @@ create_breakpoint (struct gdbarch *gdbar
/* If pending breakpoint support is turned off, throw
error. */
- if (pending_break_support == AUTO_BOOLEAN_FALSE)
+ if (cb->pending_break_support == AUTO_BOOLEAN_FALSE)
throw_exception (e);
exception_print (gdb_stderr, e);
/* If pending breakpoint support is auto query and the user
selects no, then simply return the error code. */
- if (pending_break_support == AUTO_BOOLEAN_AUTO
+ if (cb->pending_break_support == AUTO_BOOLEAN_AUTO
&& !nquery (_("Make %s pending on future shared library load? "),
- bptype_string (type_wanted)))
+ bptype_string (cb->type_wanted)))
return 0;
/* At this point, either the user was queried about setting
@@ -9608,13 +9604,13 @@ create_breakpoint (struct gdbarch *gdbar
}
/* Fast tracepoints may have additional restrictions on location. */
- if (!pending && type_wanted == bp_fast_tracepoint)
+ if (!pending && cb->type_wanted == bp_fast_tracepoint)
{
int ix;
struct linespec_sals *iter;
for (ix = 0; VEC_iterate (linespec_sals, canonical.sals, ix, iter); ++ix)
- check_fast_tracepoint_sals (gdbarch, &iter->sals);
+ check_fast_tracepoint_sals (cb->gdbarch, &iter->sals);
}
/* Verify that condition can be parsed, before setting any
@@ -9626,7 +9622,7 @@ create_breakpoint (struct gdbarch *gdbar
lsal = VEC_index (linespec_sals, canonical.sals, 0);
- if (parse_condition_and_thread)
+ if (cb->parse_condition_and_thread)
{
char *rest;
/* Here we only parse 'arg' to separate condition
@@ -9634,36 +9630,40 @@ create_breakpoint (struct gdbarch *gdbar
sal is OK. When setting the breakpoint we'll
re-parse it in context of each sal. */
- find_condition_and_thread (arg, lsal->sals.sals[0].pc, &cond_string,
- &thread, &task, &rest);
- if (cond_string)
- make_cleanup (xfree, cond_string);
+ find_condition_and_thread (cb->arg, lsal->sals.sals[0].pc,
+ &cb->cond_string,
+ &cb->thread, &task, &rest);
+ if (cb->cond_string)
+ make_cleanup (xfree, cb->cond_string);
if (rest)
make_cleanup (xfree, rest);
if (rest)
- extra_string = rest;
+ cb->extra_string = rest;
}
else
{
/* Create a private copy of condition string. */
- if (cond_string)
+ if (cb->cond_string)
{
- cond_string = xstrdup (cond_string);
- make_cleanup (xfree, cond_string);
+ cb->cond_string = xstrdup (cb->cond_string);
+ make_cleanup (xfree, cb->cond_string);
}
/* Create a private copy of any extra string. */
- if (extra_string)
+ if (cb->extra_string)
{
- extra_string = xstrdup (extra_string);
- make_cleanup (xfree, extra_string);
+ cb->extra_string = xstrdup (cb->extra_string);
+ make_cleanup (xfree, cb->extra_string);
}
}
- ops->create_breakpoints_sal (gdbarch, &canonical, lsal,
- cond_string, extra_string, type_wanted,
- tempflag ? disp_del : disp_donttouch,
- thread, task, ignore_count, ops,
- from_tty, enabled, internal, flags);
+ cb->ops->create_breakpoints_sal (cb->gdbarch, &canonical, lsal,
+ cb->cond_string, cb->extra_string,
+ cb->type_wanted,
+ cb->tempflag ? disp_del
+ : disp_donttouch,
+ cb->thread, task, cb->ignore_count,
+ cb->ops, cb->from_tty, cb->enabled,
+ cb->internal, cb->flags);
}
else
{
@@ -9671,7 +9671,7 @@ create_breakpoint (struct gdbarch *gdbar
make_cleanup (xfree, copy_arg);
- if (is_tracepoint_type (type_wanted))
+ if (is_tracepoint_type (cb->type_wanted))
{
struct tracepoint *t;
@@ -9681,31 +9681,32 @@ create_breakpoint (struct gdbarch *gdbar
else
b = XNEW (struct breakpoint);
- init_raw_breakpoint_without_location (b, gdbarch, type_wanted, ops);
+ init_raw_breakpoint_without_location (b, cb->gdbarch, cb->type_wanted,
+ cb->ops);
b->addr_string = copy_arg;
- if (parse_condition_and_thread)
+ if (cb->parse_condition_and_thread)
b->cond_string = NULL;
else
{
/* Create a private copy of condition string. */
- if (cond_string)
+ if (cb->cond_string)
{
- cond_string = xstrdup (cond_string);
- make_cleanup (xfree, cond_string);
+ cb->cond_string = xstrdup (cb->cond_string);
+ make_cleanup (xfree, cb->cond_string);
}
- b->cond_string = cond_string;
+ b->cond_string = cb->cond_string;
}
b->extra_string = NULL;
- b->ignore_count = ignore_count;
- b->disposition = tempflag ? disp_del : disp_donttouch;
+ b->ignore_count = cb->ignore_count;
+ b->disposition = cb->tempflag ? disp_del : disp_donttouch;
b->condition_not_parsed = 1;
- b->enable_state = enabled ? bp_enabled : bp_disabled;
- if ((type_wanted != bp_breakpoint
- && type_wanted != bp_hardware_breakpoint) || thread != -1)
+ b->enable_state = cb->enabled ? bp_enabled : bp_disabled;
+ if ((cb->type_wanted != bp_breakpoint
+ && cb->type_wanted != bp_hardware_breakpoint) || cb->thread != -1)
b->pspace = current_program_space;
- install_breakpoint (internal, b, 0);
+ install_breakpoint (cb->internal, b, 0);
}
if (VEC_length (linespec_sals, canonical.sals) > 1)
@@ -9743,6 +9744,7 @@ break_command_1 (char *arg, int flag, in
: bp_breakpoint);
struct breakpoint_ops *ops;
const char *arg_cp = arg;
+ struct create_breakpoint_s cb;
/* Matching breakpoints on probes. */
if (arg && probe_linespec_to_ops (&arg_cp) != NULL)
@@ -9750,17 +9752,16 @@ break_command_1 (char *arg, int flag, in
else
ops = &bkpt_breakpoint_ops;
- create_breakpoint (get_current_arch (),
- arg,
- NULL, 0, NULL, 1 /* parse arg */,
- tempflag, type_wanted,
- 0 /* Ignore count */,
- pending_break_support,
- ops,
- from_tty,
- 1 /* enabled */,
- 0 /* internal */,
- 0);
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_current_arch ();
+ cb.arg = arg;
+ cb.parse_condition_and_thread = 1;
+ cb.tempflag = tempflag;
+ cb.type_wanted = type_wanted;
+ cb.pending_break_support = pending_break_support;
+ cb.ops = ops;
+ cb.from_tty = from_tty;
+ create_breakpoint (&cb);
}
/* Helper function for break_command_1 and disassemble_command. */
@@ -9925,17 +9926,17 @@ stopat_command (char *arg, int from_tty)
static void
dprintf_command (char *arg, int from_tty)
{
- create_breakpoint (get_current_arch (),
- arg,
- NULL, 0, NULL, 1 /* parse arg */,
- 0, bp_dprintf,
- 0 /* Ignore count */,
- pending_break_support,
- &dprintf_breakpoint_ops,
- from_tty,
- 1 /* enabled */,
- 0 /* internal */,
- 0);
+ struct create_breakpoint_s cb;
+
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_current_arch ();
+ cb.arg = arg;
+ cb.parse_condition_and_thread = 1;
+ cb.type_wanted = bp_dprintf;
+ cb.pending_break_support = pending_break_support;
+ cb.ops = &dprintf_breakpoint_ops;
+ cb.from_tty = from_tty;
+ create_breakpoint (&cb);
}
static void
@@ -11650,22 +11651,22 @@ handle_gnu_v3_exceptions (int tempflag,
enum exception_event_kind ex_event, int from_tty)
{
char *trigger_func_name;
+ struct create_breakpoint_s cb;
if (ex_event == EX_EVENT_CATCH)
trigger_func_name = "__cxa_begin_catch";
else
trigger_func_name = "__cxa_throw";
- create_breakpoint (get_current_arch (),
- trigger_func_name, cond_string, -1, NULL,
- 0 /* condition and thread are valid. */,
- tempflag, bp_breakpoint,
- 0,
- AUTO_BOOLEAN_TRUE /* pending */,
- &gnu_v3_exception_catchpoint_ops, from_tty,
- 1 /* enabled */,
- 0 /* internal */,
- 0);
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_current_arch ();
+ cb.arg = trigger_func_name;
+ cb.cond_string = cond_string;
+ cb.thread = -1;
+ cb.tempflag = tempflag;
+ cb.ops = &gnu_v3_exception_catchpoint_ops;
+ cb.from_tty = from_tty;
+ create_breakpoint (&cb);
return 1;
}
@@ -15098,39 +15099,38 @@ trace_command (char *arg, int from_tty)
{
struct breakpoint_ops *ops;
const char *arg_cp = arg;
+ struct create_breakpoint_s cb;
if (arg && probe_linespec_to_ops (&arg_cp))
ops = &tracepoint_probe_breakpoint_ops;
else
ops = &tracepoint_breakpoint_ops;
- create_breakpoint (get_current_arch (),
- arg,
- NULL, 0, NULL, 1 /* parse arg */,
- 0 /* tempflag */,
- bp_tracepoint /* type_wanted */,
- 0 /* Ignore count */,
- pending_break_support,
- ops,
- from_tty,
- 1 /* enabled */,
- 0 /* internal */, 0);
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_current_arch ();
+ cb.arg = arg;
+ cb.parse_condition_and_thread = 1;
+ cb.type_wanted = bp_tracepoint;
+ cb.pending_break_support = pending_break_support;
+ cb.ops = ops;
+ cb.from_tty = from_tty;
+ create_breakpoint (&cb);
}
static void
ftrace_command (char *arg, int from_tty)
{
- create_breakpoint (get_current_arch (),
- arg,
- NULL, 0, NULL, 1 /* parse arg */,
- 0 /* tempflag */,
- bp_fast_tracepoint /* type_wanted */,
- 0 /* Ignore count */,
- pending_break_support,
- &tracepoint_breakpoint_ops,
- from_tty,
- 1 /* enabled */,
- 0 /* internal */, 0);
+ struct create_breakpoint_s cb;
+
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_current_arch ();
+ cb.arg = arg;
+ cb.parse_condition_and_thread = 1;
+ cb.type_wanted = bp_fast_tracepoint;
+ cb.pending_break_support = pending_break_support;
+ cb.ops = &tracepoint_breakpoint_ops;
+ cb.from_tty = from_tty;
+ create_breakpoint (&cb);
}
/* strace command implementation. Creates a static tracepoint. */
@@ -15139,6 +15139,7 @@ static void
strace_command (char *arg, int from_tty)
{
struct breakpoint_ops *ops;
+ struct create_breakpoint_s cb;
/* Decide if we are dealing with a static tracepoint marker (`-m'),
or with a normal static tracepoint. */
@@ -15147,17 +15148,15 @@ strace_command (char *arg, int from_tty)
else
ops = &tracepoint_breakpoint_ops;
- create_breakpoint (get_current_arch (),
- arg,
- NULL, 0, NULL, 1 /* parse arg */,
- 0 /* tempflag */,
- bp_static_tracepoint /* type_wanted */,
- 0 /* Ignore count */,
- pending_break_support,
- ops,
- from_tty,
- 1 /* enabled */,
- 0 /* internal */, 0);
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_current_arch ();
+ cb.arg = arg;
+ cb.parse_condition_and_thread = 1;
+ cb.type_wanted = bp_static_tracepoint;
+ cb.pending_break_support = pending_break_support;
+ cb.ops = ops;
+ cb.from_tty = from_tty;
+ create_breakpoint (&cb);
}
/* Set up a fake reader function that gets command lines from a linked
@@ -15189,6 +15188,7 @@ create_tracepoint_from_upload (struct up
{
char *addr_str, small_buf[100];
struct tracepoint *tp;
+ struct create_breakpoint_s cb;
if (utp->at_string)
addr_str = utp->at_string;
@@ -15210,20 +15210,17 @@ create_tracepoint_from_upload (struct up
warning (_("Uploaded tracepoint %d condition "
"has no source form, ignoring it"),
utp->number);
-
- if (!create_breakpoint (get_current_arch (),
- addr_str,
- utp->cond_string, -1, NULL,
- 0 /* parse cond/thread */,
- 0 /* tempflag */,
- utp->type /* type_wanted */,
- 0 /* Ignore count */,
- pending_break_support,
- &tracepoint_breakpoint_ops,
- 0 /* from_tty */,
- utp->enabled /* enabled */,
- 0 /* internal */,
- CREATE_BREAKPOINT_FLAGS_INSERTED))
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_current_arch ();
+ cb.arg = addr_str;
+ cb.cond_string = utp->cond_string;
+ cb.thread = -1;
+ cb.type_wanted = utp->type;
+ cb.pending_break_support = pending_break_support;
+ cb.ops = &tracepoint_breakpoint_ops;
+ cb.enabled = utp->enabled;
+ cb.flags = CREATE_BREAKPOINT_FLAGS_INSERTED;
+ if (!create_breakpoint (&cb))
return NULL;
/* Get the tracepoint we just created. */
--- a/breakpoint.h
+++ b/breakpoint.h
@@ -1265,17 +1265,79 @@ enum breakpoint_create_flags
CREATE_BREAKPOINT_FLAGS_INSERTED = 1 << 0
};
-extern int create_breakpoint (struct gdbarch *gdbarch, char *arg,
- char *cond_string, int thread,
- char *extra_string,
- int parse_condition_and_thread,
- int tempflag, enum bptype wanted_type,
- int ignore_count,
- enum auto_boolean pending_break_support,
- const struct breakpoint_ops *ops,
- int from_tty,
- int enabled,
- int internal, unsigned flags);
+/* This argument struct for function CREATE_BREAKPOINT. */
+
+struct create_breakpoint_s
+{
+ /* GDBARCH will be initialized to NULL in
+ function CREATE_BREAKPOINT_INIT. */
+ struct gdbarch *gdbarch;
+
+ /* ARG will be initialized to NULL in function CREATE_BREAKPOINT_INIT. */
+ char *arg;
+
+ /* COND_STRING will be initialized to NULL in function
+ CREATE_BREAKPOINT_INIT. */
+ char *cond_string;
+
+ /* THREAD will be initialized to 0 in function CREATE_BREAKPOINT_INIT. */
+ int thread;
+
+ /* EXTRA_STRING will be initialized to NULL in function
+ CREATE_BREAKPOINT_INIT. */
+ char *extra_string;
+
+ /* Function CREATE_BREAKPOINT has two major modes of operations,
+ selected by the PARSE_CONDITION_AND_THREAD parameter.
+ If non-zero, the function will parse arg, extracting
+ breakpoint location, address and thread.
+ Otherwise, ARG is just the location of breakpoint,
+ with condition and thread specified by
+ the COND_STRING and THREAD parameters.
+ It will be initialized to 0 in function CREATE_BREAKPOINT_INIT. */
+ int parse_condition_and_thread;
+
+ /* TEMPFLAG will be initialized to 0 in
+ function CREATE_BREAKPOINT_INIT. */
+ int tempflag;
+
+ /* WANTED_TYPE will be initialized to bp_breakpoint in
+ function CREATE_BREAKPOINT_INIT. */
+ enum bptype type_wanted;
+
+ /* IGNORE_COUNT will be initialized to 0 in
+ function CREATE_BREAKPOINT_INIT. */
+ int ignore_count;
+
+ /* PENDING_BREAK_SUPPORT will be initialized to AUTO_BOOLEAN_TRUE in
+ function CREATE_BREAKPOINT_INIT. */
+ enum auto_boolean pending_break_support;
+
+ /* OPS will be initialized to NULL in
+ function CREATE_BREAKPOINT_INIT. */
+ const struct breakpoint_ops *ops;
+
+ /* FROM_TTY will be initialized to 0 in
+ function CREATE_BREAKPOINT_INIT. */
+ int from_tty;
+
+ /* ENABLED will be initialized to 1 in
+ function CREATE_BREAKPOINT_INIT. */
+ int enabled;
+
+ /* If INTERNAL is non-zero, the breakpoint number will be allocated
+ from the internal breakpoint count.
+ It will be initialized to 0 in function CREATE_BREAKPOINT_INIT. */
+ int internal;
+
+ /* FLAGS will be initialized to 0 in
+ function CREATE_BREAKPOINT_INIT. */
+ unsigned flags;
+};
+
+extern void create_breakpoint_init (struct create_breakpoint_s *cb);
+
+extern int create_breakpoint (struct create_breakpoint_s *cb);
extern void insert_breakpoints (void);
--- a/mi/mi-cmd-break.c
+++ b/mi/mi-cmd-break.c
@@ -100,8 +100,7 @@ mi_cmd_break_insert (char *command, char
int enabled = 1;
int tracepoint = 0;
struct cleanup *back_to;
- enum bptype type_wanted;
- struct breakpoint_ops *ops;
+ struct create_breakpoint_s cb;
enum opt
{
@@ -178,20 +177,21 @@ mi_cmd_break_insert (char *command, char
"fast" is a misnomer, actually, "jump" would be more appropriate.
A simulator or an emulator could conceivably implement fast
regular non-jump based tracepoints. */
- type_wanted = (tracepoint
- ? (hardware ? bp_fast_tracepoint : bp_tracepoint)
- : (hardware ? bp_hardware_breakpoint : bp_breakpoint));
- ops = tracepoint ? &tracepoint_breakpoint_ops : &bkpt_breakpoint_ops;
-
- create_breakpoint (get_current_arch (), address, condition, thread,
- NULL,
- 0 /* condition and thread are valid. */,
- temp_p, type_wanted,
- ignore_count,
- pending ? AUTO_BOOLEAN_TRUE : AUTO_BOOLEAN_FALSE,
- ops, 0, enabled, 0, 0);
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_current_arch ();
+ cb.arg = address;
+ cb.cond_string = condition;
+ cb.thread = thread;
+ cb.tempflag = temp_p;
+ cb.type_wanted = (tracepoint
+ ? (hardware ? bp_fast_tracepoint : bp_tracepoint)
+ : (hardware ? bp_hardware_breakpoint : bp_breakpoint));
+ cb.ignore_count = ignore_count;
+ cb.pending_break_support = pending ? AUTO_BOOLEAN_TRUE : AUTO_BOOLEAN_FALSE;
+ cb.ops = tracepoint ? &tracepoint_breakpoint_ops : &bkpt_breakpoint_ops;
+ cb.enabled = enabled;
+ create_breakpoint (&cb);
do_cleanups (back_to);
-
}
enum wp_type
--- a/python/py-breakpoint.c
+++ b/python/py-breakpoint.c
@@ -621,14 +621,15 @@ bppy_init (PyObject *self, PyObject *arg
{
case bp_breakpoint:
{
- create_breakpoint (python_gdbarch,
- copy, NULL, -1, NULL,
- 0,
- 0, bp_breakpoint,
- 0,
- AUTO_BOOLEAN_TRUE,
- &bkpt_breakpoint_ops,
- 0, 1, internal_bp, 0);
+ struct create_breakpoint_s cb;
+
+ create_breakpoint_init (&cb);
+ cb.gdbarch = python_gdbarch;
+ cb.arg = copy;
+ cb.thread = -1;
+ cb.ops = &bkpt_breakpoint_ops;
+ cb.internal = internal_bp;
+ create_breakpoint (&cb);
break;
}
case bp_watchpoint:
--- a/python/py-finishbreakpoint.c
+++ b/python/py-finishbreakpoint.c
@@ -281,20 +281,19 @@ bpfinishpy_init (PyObject *self, PyObjec
TRY_CATCH (except, RETURN_MASK_ALL)
{
+ struct create_breakpoint_s cb;
/* Set a breakpoint on the return address. */
finish_pc = get_frame_pc (prev_frame);
xsnprintf (small_buf, sizeof (small_buf), "*%s", hex_string (finish_pc));
- addr_str = small_buf;
- create_breakpoint (python_gdbarch,
- addr_str, NULL, thread, NULL,
- 0,
- 1 /*temp_flag*/,
- bp_breakpoint,
- 0,
- AUTO_BOOLEAN_TRUE,
- &bkpt_breakpoint_ops,
- 0, 1, internal_bp, 0);
+ create_breakpoint_init (&cb);
+ cb.gdbarch = python_gdbarch;
+ cb.arg = small_buf;
+ cb.thread = thread;
+ cb.tempflag = 1;
+ cb.ops = &bkpt_breakpoint_ops;
+ cb.internal = internal_bp;
+ create_breakpoint (&cb);
}
GDB_PY_SET_HANDLE_EXCEPTION (except);
--- a/spu-tdep.c
+++ b/spu-tdep.c
@@ -1898,6 +1898,7 @@ spu_catch_start (struct objfile *objfile
struct symtab *symtab;
CORE_ADDR pc;
char buf[32];
+ struct create_breakpoint_s cb;
/* Do this only if requested by "set spu stop-on-load on". */
if (!spu_stop_on_load_p)
@@ -1940,15 +1941,14 @@ spu_catch_start (struct objfile *objfile
/* Use a numerical address for the set_breakpoint command to avoid having
the breakpoint re-set incorrectly. */
xsnprintf (buf, sizeof buf, "*%s", core_addr_to_string (pc));
- create_breakpoint (get_objfile_arch (objfile), buf /* arg */,
- NULL /* cond_string */, -1 /* thread */,
- NULL /* extra_string */,
- 0 /* parse_condition_and_thread */, 1 /* tempflag */,
- bp_breakpoint /* type_wanted */,
- 0 /* ignore_count */,
- AUTO_BOOLEAN_FALSE /* pending_break_support */,
- &bkpt_breakpoint_ops /* ops */, 0 /* from_tty */,
- 1 /* enabled */, 0 /* internal */, 0);
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_objfile_arch (objfile);
+ cb.arg = buf;
+ cb.thread = -1;
+ cb.tempflag = 1;
+ cb.pending_break_support = AUTO_BOOLEAN_FALSE;
+ cb.ops = &bkpt_breakpoint_ops;
+ create_breakpoint (&cb);
}
[-- Attachment #3: insight-create_breakpoint.txt --]
[-- Type: text/plain, Size: 986 bytes --]
--- a/gdbtk/generic/gdbtk-bp.c
+++ b/gdbtk/generic/gdbtk-bp.c
@@ -530,16 +530,20 @@ gdb_set_bp (ClientData clientData, Tcl_I
TRY_CATCH (e, RETURN_MASK_ALL)
{
- create_breakpoint (get_current_arch (), address, condition, thread,
- NULL,
- 0 /* condition and thread are valid */,
- temp,
- bp_breakpoint /* type wanted */,
- ignore_count,
- (pending ? AUTO_BOOLEAN_TRUE : AUTO_BOOLEAN_FALSE),
- &bkpt_breakpoint_ops,
- 0 /* from_tty */,
- enabled, 0, 0);
+ struct create_breakpoint_s cb;
+
+ create_breakpoint_init (&cb);
+ cb.gdbarch = get_current_arch ();
+ cb.arg = address;
+ cb.cond_string = condition;
+ cb.thread = thread;
+ cb.tempflag = temp;
+ cb.ignore_count = ignore_count;
+ cb.pending_break_support = pending ? AUTO_BOOLEAN_TRUE
+ : AUTO_BOOLEAN_FALSE;
+ cb.ops = &bkpt_breakpoint_ops;
+ cb.enabled = enabled;
+ create_breakpoint (&cb);
}
if (e.reason < 0)
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] change the arguments of create_breakpoint to a struct
2013-02-21 5:40 [PATCH] change the arguments of create_breakpoint to a struct Hui Zhu
@ 2013-02-21 13:58 ` Sergio Durigan Junior
2013-02-22 2:15 ` Hui Zhu
0 siblings, 1 reply; 3+ messages in thread
From: Sergio Durigan Junior @ 2013-02-21 13:58 UTC (permalink / raw)
To: Hui Zhu
Cc: gdb-patches ml, insight, Pedro Alves, Tom Tromey, Stan Shebs,
Keith Seitz
On Thursday, February 21 2013, Hui Zhu wrote:
> According to the discussion in
> http://sourceware.org/ml/gdb/2012-08/msg00001.html
>
> I post a new patch for GDB to change the arguments of
> create_breakpoint to a struct.
> And I also add a function create_breakpoint to Initialize all the
> element of this struct to its default value. Then if we want to add
> more argument to this function, we can just add the default value
> Initialize code to this function and don't need update all the
> function that call create_breakpoint.
>
> And I post a patch for insight to change the arguments of
> create_breakpoint to a struct.
>
> Please help me review it.
Thanks for the patch, Hui. A few comments.
> --- a/breakpoint.c
> +++ b/breakpoint.c
> @@ -9493,32 +9493,27 @@ decode_static_tracepoint_spec (char **ar
> return sals;
> }
>
> +/* Initialize all the element of CB to its default value. */
I think it is better to write "... to their default values." Also
missing double-space after period.
> +void
> +create_breakpoint_init (struct create_breakpoint_s *cb)
> +{
> + memset (cb, '\0', sizeof (*cb));
> + cb->type_wanted = bp_breakpoint;
> + cb->pending_break_support = AUTO_BOOLEAN_TRUE;
> + cb->enabled = 1;
> +}
I would change the name of the fuction to `init_breakpoint_properties'
or something (see more below).
I have read the discussion on the link you posted above, but still I
want to confirm something: would it be possible to make this function
take all the arguments that create_breakpoint et al now take? My point
is that, IMO, it would be clearer to pass all those arguments to this
function directly, instead of filing them in the respective callers.
Matter of taste, of couse.
> /* Set a breakpoint. This function is shared between CLI and MI
> - functions for setting a breakpoint. This function has two major
> - modes of operations, selected by the PARSE_CONDITION_AND_THREAD
> - parameter. If non-zero, the function will parse arg, extracting
> - breakpoint location, address and thread. Otherwise, ARG is just
> - the location of breakpoint, with condition and thread specified by
> - the COND_STRING and THREAD parameters. If INTERNAL is non-zero,
> - the breakpoint number will be allocated from the internal
> - breakpoint count. Returns true if any breakpoint was created;
> - false otherwise. */
> + functions for setting a breakpoint.
> + Returns true if any breakpoint was created; false otherwise. */
You have changed only the function prototype and they way the function
accesses its arguments, but not the function behaviour itself, so I
would be against changing this comment. Maybe it could be rewritten to
inform that the fields are now part of the CB struct.
> int
> -create_breakpoint (struct gdbarch *gdbarch,
> - char *arg, char *cond_string,
> - int thread, char *extra_string,
> - int parse_condition_and_thread,
> - int tempflag, enum bptype type_wanted,
> - int ignore_count,
> - enum auto_boolean pending_break_support,
> - const struct breakpoint_ops *ops,
> - int from_tty, int enabled, int internal,
> - unsigned flags)
> +create_breakpoint (struct create_breakpoint_s *cb)
> {
[...]
> @@ -9743,6 +9744,7 @@ break_command_1 (char *arg, int flag, in
> : bp_breakpoint);
> struct breakpoint_ops *ops;
> const char *arg_cp = arg;
> + struct create_breakpoint_s cb;
>
> /* Matching breakpoints on probes. */
> if (arg && probe_linespec_to_ops (&arg_cp) != NULL)
> @@ -9750,17 +9752,16 @@ break_command_1 (char *arg, int flag, in
> else
> ops = &bkpt_breakpoint_ops;
>
> - create_breakpoint (get_current_arch (),
> - arg,
> - NULL, 0, NULL, 1 /* parse arg */,
> - tempflag, type_wanted,
> - 0 /* Ignore count */,
> - pending_break_support,
> - ops,
> - from_tty,
> - 1 /* enabled */,
> - 0 /* internal */,
> - 0);
> + create_breakpoint_init (&cb);
> + cb.gdbarch = get_current_arch ();
> + cb.arg = arg;
> + cb.parse_condition_and_thread = 1;
> + cb.tempflag = tempflag;
> + cb.type_wanted = type_wanted;
> + cb.pending_break_support = pending_break_support;
> + cb.ops = ops;
> + cb.from_tty = from_tty;
> + create_breakpoint (&cb);
Just to make my argument clearer, my suggestion is to actually replace
this with:
init_breakpoint_properties (&cb, get_current_arch (), arg,
... );
create_breakpoint (&cb);
Don't know, it seems more organized to me. But of course, you might
just want to wait for some maintainer to give his suggestion :-).
> --- a/breakpoint.h
> +++ b/breakpoint.h
> @@ -1265,17 +1265,79 @@ enum breakpoint_create_flags
> CREATE_BREAKPOINT_FLAGS_INSERTED = 1 << 0
> };
>
> -extern int create_breakpoint (struct gdbarch *gdbarch, char *arg,
> - char *cond_string, int thread,
> - char *extra_string,
> - int parse_condition_and_thread,
> - int tempflag, enum bptype wanted_type,
> - int ignore_count,
> - enum auto_boolean pending_break_support,
> - const struct breakpoint_ops *ops,
> - int from_tty,
> - int enabled,
> - int internal, unsigned flags);
> +/* This argument struct for function CREATE_BREAKPOINT. */
May I suggest rewritting this comment? Something like:
/* Structure which holds the properties of breakpoints and their
variations (tracepoints, watchpoints, etc.). It is used primarily
by the function `create_breakpoint'. */
or some such.
> +struct create_breakpoint_s
<http://sourceware.org/ml/gdb/2012-08/msg00004.html>
I liked Stan's proposal, to name the structure "struct
breakpoint_properties", so I suggest using this name.
> +{
> + /* GDBARCH will be initialized to NULL in
> + function CREATE_BREAKPOINT_INIT. */
> + struct gdbarch *gdbarch;
I think this kind of comment is not really necessary. I would change it
to something like:
/* The gdbarch associated with the breakpoint. */
Same for the rest.
[...]
> + /* Function CREATE_BREAKPOINT has two major modes of operations,
> + selected by the PARSE_CONDITION_AND_THREAD parameter.
> + If non-zero, the function will parse arg, extracting
> + breakpoint location, address and thread.
> + Otherwise, ARG is just the location of breakpoint,
> + with condition and thread specified by
> + the COND_STRING and THREAD parameters.
> + It will be initialized to 0 in function CREATE_BREAKPOINT_INIT. */
> + int parse_condition_and_thread;
This comment refers to the `create_breakpoint' function, so it should
stay there IMO.
Aside of that, I have no other comments. Thanks again for doing this.
--
Sergio
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] change the arguments of create_breakpoint to a struct
2013-02-21 13:58 ` Sergio Durigan Junior
@ 2013-02-22 2:15 ` Hui Zhu
0 siblings, 0 replies; 3+ messages in thread
From: Hui Zhu @ 2013-02-22 2:15 UTC (permalink / raw)
To: Sergio Durigan Junior
Cc: gdb-patches ml, insight, Pedro Alves, Tom Tromey, Stan Shebs,
Keith Seitz
Hi Sergio,
Thanks for your review.
On Thu, Feb 21, 2013 at 9:58 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Thursday, February 21 2013, Hui Zhu wrote:
>
>> According to the discussion in
>> http://sourceware.org/ml/gdb/2012-08/msg00001.html
>>
>> I post a new patch for GDB to change the arguments of
>> create_breakpoint to a struct.
>> And I also add a function create_breakpoint to Initialize all the
>> element of this struct to its default value. Then if we want to add
>> more argument to this function, we can just add the default value
>> Initialize code to this function and don't need update all the
>> function that call create_breakpoint.
>>
>> And I post a patch for insight to change the arguments of
>> create_breakpoint to a struct.
>>
>> Please help me review it.
>
> Thanks for the patch, Hui. A few comments.
>
>> --- a/breakpoint.c
>> +++ b/breakpoint.c
>> @@ -9493,32 +9493,27 @@ decode_static_tracepoint_spec (char **ar
>> return sals;
>> }
>>
>> +/* Initialize all the element of CB to its default value. */
>
> I think it is better to write "... to their default values." Also
> missing double-space after period.
>
>> +void
>> +create_breakpoint_init (struct create_breakpoint_s *cb)
>> +{
>> + memset (cb, '\0', sizeof (*cb));
>> + cb->type_wanted = bp_breakpoint;
>> + cb->pending_break_support = AUTO_BOOLEAN_TRUE;
>> + cb->enabled = 1;
>> +}
>
> I would change the name of the fuction to `init_breakpoint_properties'
> or something (see more below).
>
> I have read the discussion on the link you posted above, but still I
> want to confirm something: would it be possible to make this function
> take all the arguments that create_breakpoint et al now take? My point
> is that, IMO, it would be clearer to pass all those arguments to this
> function directly, instead of filing them in the respective callers.
> Matter of taste, of couse.
Cool! That is a good part to discussion. When I planed to do this
patch, I thought about let this function do it.
I thought is If move all the argument of create_breakpoint to this
function. Then this function will become another create_breakpoint.
Then I thought is maybe we can select some arguments that don't have
default value. But it need more discussion on that.
And about cleaning up, I think is is good comments too. I thought is
maybe we can do both of them: update arguments to a struct and
cleaning up them.
I am sorry that I didn't post a new patch because I think maybe we
need more discussion on this issue.
Thanks,
Hui
>
>> /* Set a breakpoint. This function is shared between CLI and MI
>> - functions for setting a breakpoint. This function has two major
>> - modes of operations, selected by the PARSE_CONDITION_AND_THREAD
>> - parameter. If non-zero, the function will parse arg, extracting
>> - breakpoint location, address and thread. Otherwise, ARG is just
>> - the location of breakpoint, with condition and thread specified by
>> - the COND_STRING and THREAD parameters. If INTERNAL is non-zero,
>> - the breakpoint number will be allocated from the internal
>> - breakpoint count. Returns true if any breakpoint was created;
>> - false otherwise. */
>> + functions for setting a breakpoint.
>> + Returns true if any breakpoint was created; false otherwise. */
>
> You have changed only the function prototype and they way the function
> accesses its arguments, but not the function behaviour itself, so I
> would be against changing this comment. Maybe it could be rewritten to
> inform that the fields are now part of the CB struct.
>
>> int
>> -create_breakpoint (struct gdbarch *gdbarch,
>> - char *arg, char *cond_string,
>> - int thread, char *extra_string,
>> - int parse_condition_and_thread,
>> - int tempflag, enum bptype type_wanted,
>> - int ignore_count,
>> - enum auto_boolean pending_break_support,
>> - const struct breakpoint_ops *ops,
>> - int from_tty, int enabled, int internal,
>> - unsigned flags)
>> +create_breakpoint (struct create_breakpoint_s *cb)
>> {
>
> [...]
>
>> @@ -9743,6 +9744,7 @@ break_command_1 (char *arg, int flag, in
>> : bp_breakpoint);
>> struct breakpoint_ops *ops;
>> const char *arg_cp = arg;
>> + struct create_breakpoint_s cb;
>>
>> /* Matching breakpoints on probes. */
>> if (arg && probe_linespec_to_ops (&arg_cp) != NULL)
>> @@ -9750,17 +9752,16 @@ break_command_1 (char *arg, int flag, in
>> else
>> ops = &bkpt_breakpoint_ops;
>>
>> - create_breakpoint (get_current_arch (),
>> - arg,
>> - NULL, 0, NULL, 1 /* parse arg */,
>> - tempflag, type_wanted,
>> - 0 /* Ignore count */,
>> - pending_break_support,
>> - ops,
>> - from_tty,
>> - 1 /* enabled */,
>> - 0 /* internal */,
>> - 0);
>> + create_breakpoint_init (&cb);
>> + cb.gdbarch = get_current_arch ();
>> + cb.arg = arg;
>> + cb.parse_condition_and_thread = 1;
>> + cb.tempflag = tempflag;
>> + cb.type_wanted = type_wanted;
>> + cb.pending_break_support = pending_break_support;
>> + cb.ops = ops;
>> + cb.from_tty = from_tty;
>> + create_breakpoint (&cb);
>
> Just to make my argument clearer, my suggestion is to actually replace
> this with:
>
>
> init_breakpoint_properties (&cb, get_current_arch (), arg,
> ... );
> create_breakpoint (&cb);
>
> Don't know, it seems more organized to me. But of course, you might
> just want to wait for some maintainer to give his suggestion :-).
>
>> --- a/breakpoint.h
>> +++ b/breakpoint.h
>> @@ -1265,17 +1265,79 @@ enum breakpoint_create_flags
>> CREATE_BREAKPOINT_FLAGS_INSERTED = 1 << 0
>> };
>>
>> -extern int create_breakpoint (struct gdbarch *gdbarch, char *arg,
>> - char *cond_string, int thread,
>> - char *extra_string,
>> - int parse_condition_and_thread,
>> - int tempflag, enum bptype wanted_type,
>> - int ignore_count,
>> - enum auto_boolean pending_break_support,
>> - const struct breakpoint_ops *ops,
>> - int from_tty,
>> - int enabled,
>> - int internal, unsigned flags);
>> +/* This argument struct for function CREATE_BREAKPOINT. */
>
> May I suggest rewritting this comment? Something like:
>
> /* Structure which holds the properties of breakpoints and their
> variations (tracepoints, watchpoints, etc.). It is used primarily
> by the function `create_breakpoint'. */
>
> or some such.
>
>> +struct create_breakpoint_s
>
> <http://sourceware.org/ml/gdb/2012-08/msg00004.html>
>
> I liked Stan's proposal, to name the structure "struct
> breakpoint_properties", so I suggest using this name.
>
>> +{
>> + /* GDBARCH will be initialized to NULL in
>> + function CREATE_BREAKPOINT_INIT. */
>> + struct gdbarch *gdbarch;
>
> I think this kind of comment is not really necessary. I would change it
> to something like:
>
> /* The gdbarch associated with the breakpoint. */
>
> Same for the rest.
>
> [...]
>
>> + /* Function CREATE_BREAKPOINT has two major modes of operations,
>> + selected by the PARSE_CONDITION_AND_THREAD parameter.
>> + If non-zero, the function will parse arg, extracting
>> + breakpoint location, address and thread.
>> + Otherwise, ARG is just the location of breakpoint,
>> + with condition and thread specified by
>> + the COND_STRING and THREAD parameters.
>> + It will be initialized to 0 in function CREATE_BREAKPOINT_INIT. */
>> + int parse_condition_and_thread;
>
> This comment refers to the `create_breakpoint' function, so it should
> stay there IMO.
>
> Aside of that, I have no other comments. Thanks again for doing this.
>
> --
> Sergio
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-02-22 2:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-21 5:40 [PATCH] change the arguments of create_breakpoint to a struct Hui Zhu
2013-02-21 13:58 ` Sergio Durigan Junior
2013-02-22 2:15 ` Hui Zhu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).