On Fri, May 30, 2014 at 11:18 AM, Keith Seitz <keiths@redhat.com> wrote:
> This is an update of this patch based on changes from reviews of previous
> patches.
>
> Keith
>
> Changes since last version:
> - remove SAVE_SPEC stuff
> - canonicalize_linespec: remove expression stuff
> - ditto convert_linespec_to_sals
> - event_location_to_string_const: compute return value
> (aka remove SAVE_SPEC)
> - new_address_location
>
> ChangeLog
> 2014-05-29 Keith Seitz <keiths@redhat.com>
>
>
> * breakpoint.c (create_thread_event_breakpoint): Convert
> linespec to address location.
> (init_breakpoint_sal): Likewise.
> * linespec.c (canonicalize_linespec): Do not handle address
> locations here.
>
> (convert_address_location_to_sals): New function; contents moved
> from ...
> (convert_linespc_to_sals): ... here.
> (parse_linespec): Remove address locations from linespec grammar.
>
> Remove handling of address locations.
> (event_location_to_sals): Handle ADDRESS_LOCATION.
>
> (linespec_expression_to_pc): Export.
> * linespec.h (linespec_expression_to_pc): Add declaration.
> * location.c (copy_event_location): Handle address locations.
> (delete_event_location): Likewise.
> (event_location_to_string): Likewise.
> (string_to_event_location): Likewise.
> (event_location_empty_p): Likewise.
> * location.h (enum event_location_type): Add EVENT_LOCATION_ADDRESS.
> (struct event_location.u) <address>: New member.
> (ADDRESS_LOCATION): Define accessor macro.
>
> * python/py-finishbreakpoint.c (bpfinishpy_init): Remove local
> variable `finish_pc'.
> Convert linespec to address location.
> * spu-tdep.c (spu_catch_start): Convert linespec to address
> location.
>
Hi.
I've given the series another read through.
Not as detailed a read through as I want to, but that'll have to wait for a bit.
I do have one comment that I thought I'd forward on now though ...
Maybe I'm asking for too much in a C-based world,
but IWBN if code like this was done differently.
It's not a blocker, but I would like your thoughts.
@@ -291,10 +290,8 @@ bpfinishpy_init (PyObject *self, PyObject *args,
PyObject *kwargs)
struct event_location location;
/* Set a breakpoint on the return address. */
- initialize_event_location (&location, LINESPEC_LOCATION);
- finish_pc = get_frame_pc (prev_frame);
- xsnprintf (small_buf, sizeof (small_buf), "*%s", hex_string (finish_pc));
- EVENT_LOCATION_LINESPEC (&location) = small_buf;
+ initialize_event_location (&location, ADDRESS_LOCATION);
+ EVENT_LOCATION_ADDRESS (&location) = get_frame_pc (prev_frame);
create_breakpoint (python_gdbarch,
&location, NULL, thread, NULL,
0
[apologies if there's a cut-n-paste error there - I hope that comes through ok]
There's still too much implementation detail exposed here for my
taste, but this is C so we have to compromise. In this case,
bpfinishpy_init has to take on the responsibility of knowing what
needs to be done to properly initialize an ADDRESS_LOCATION - we can't
call a constructor because the object is on the stack.
I think there are a few cases in the patch set like this, so IWBN, if
possible, to do something better here.
I can think of two solutions. Maybe there's another even better one.
1) Have a "placement new"-like constructor akin to the existing
new_foo_location constructors we have that takes a pointer to the
object to initialize.
2) Always use the existing new_foo_location constructors and free the
object when done.
Thoughts?
On Mon, Jun 9, 2014 at 11:12 AM, Keith Seitz <keiths@redhat.com> wrote:
> On 06/08/2014 02:58 PM, Doug Evans wrote:
>
>> I've given the series another read through.
>> Not as detailed a read through as I want to, but that'll have to wait for
>> a bit.
>>
>
> I really appreciate your efforts here. I owe you a beverage of your choice
> should we ever meet in person again!
Got Guinness? :-)
>> @@ -291,10 +290,8 @@ bpfinishpy_init (PyObject *self, PyObject *args,
>> PyObject *kwargs)
>> struct event_location location;
>>
>> /* Set a breakpoint on the return address. */
>> - initialize_event_location (&location, LINESPEC_LOCATION);
>> - finish_pc = get_frame_pc (prev_frame);
>> - xsnprintf (small_buf, sizeof (small_buf), "*%s", hex_string
>> (finish_pc));
>> - EVENT_LOCATION_LINESPEC (&location) = small_buf;
>> + initialize_event_location (&location, ADDRESS_LOCATION);
>> + EVENT_LOCATION_ADDRESS (&location) = get_frame_pc (prev_frame);
>> create_breakpoint (python_gdbarch,
>> &location, NULL, thread, NULL,
>> 0
>> I can think of two solutions. Maybe there's another even better one.
>>
>> 1) Have a "placement new"-like constructor akin to the existing
>> new_foo_location constructors we have that takes a pointer to the
>> object to initialize.
>> 2) Always use the existing new_foo_location constructors and free the
>> object when done.
>
>
> My purpose on this was efficiency. I haven't a clue how often a python
> script could call this, so I opted to handle memory allocations a little
> more tightly. I have no real objections to either solution, but I guess I
> would opt for #2 since that both exists and is already widely used.
>
> Do you want me to submit an update to the series for this, or shall I wait
> for more feedback?
When we last left our story I was about to leave for holidays, was
gone for most of June and half of July. Vacation time is over, alas,
back to work ... :-)
No need to submit an updated series until I get any remaining comments
out for this series.
Thanks!
@@ -7538,9 +7538,7 @@ create_thread_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
b->enable_state = bp_enabled;
/* location has to be used or breakpoint_re_set will delete me. */
- b->location = new_linespec_location (NULL);
- EVENT_LOCATION_LINESPEC (b->location)
- = xstrprintf ("*%s", paddress (b->loc->gdbarch, b->loc->address));
+ b->location = new_address_location (b->loc->address);
update_global_location_list_nothrow (1);
@@ -9423,11 +9421,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
if (location != NULL)
b->location = location;
else
- {
- b->location = new_linespec_location (NULL);
- EVENT_LOCATION_LINESPEC (b->location)
- = xstrprintf ("*%s", paddress (b->loc->gdbarch, b->loc->address));
- }
+ b->location = new_address_location (b->loc->address);
b->filter = filter;
}
@@ -317,7 +317,7 @@ static void iterate_over_file_blocks (struct symtab *symtab,
static void initialize_defaults (struct symtab **default_symtab,
int *default_line);
-static CORE_ADDR linespec_expression_to_pc (const char **exp_ptr);
+CORE_ADDR linespec_expression_to_pc (const char **exp_ptr);
static struct symtabs_and_lines decode_objc (struct linespec_state *self,
linespec_p ls,
@@ -1766,77 +1766,68 @@ linespec_parse_basic (linespec_parser *parser)
static void
canonicalize_linespec (struct linespec_state *state, linespec_p ls)
{
+ struct ui_file *buf;
+ int need_colon = 0;
+
/* If canonicalization was not requested, no need to do anything. */
if (!state->canonical)
return;
- /* Shortcut expressions, which can only appear by themselves. */
- if (ls->expression != NULL)
- {
- state->canonical->location
- = new_linespec_location (ls->expression);
- }
- else
- {
- struct ui_file *buf;
- int need_colon = 0;
+ buf = mem_fileopen ();
- buf = mem_fileopen ();
+ state->canonical->location = new_linespec_location (NULL);
- state->canonical->location = new_linespec_location (NULL);
+ if (ls->source_filename)
+ {
+ fputs_unfiltered (ls->source_filename, buf);
+ need_colon = 1;
+ }
- if (ls->source_filename)
- {
- fputs_unfiltered (ls->source_filename, buf);
- need_colon = 1;
- }
+ if (ls->function_name)
+ {
+ if (need_colon)
+ fputc_unfiltered (':', buf);
+ fputs_unfiltered (ls->function_name, buf);
+ need_colon = 1;
+ }
- if (ls->function_name)
- {
- if (need_colon)
- fputc_unfiltered (':', buf);
- fputs_unfiltered (ls->function_name, buf);
- need_colon = 1;
- }
+ if (ls->label_name)
+ {
+ if (need_colon)
+ fputc_unfiltered (':', buf);
- if (ls->label_name)
+ if (ls->function_name == NULL)
{
- if (need_colon)
- fputc_unfiltered (':', buf);
-
- if (ls->function_name == NULL)
- {
- struct symbol *s;
-
- /* No function was specified, so add the symbol name. */
- gdb_assert (ls->labels.function_symbols != NULL
- && (VEC_length (symbolp, ls->labels.function_symbols)
- == 1));
- s = VEC_index (symbolp, ls->labels.function_symbols, 0);
- fputs_unfiltered (SYMBOL_NATURAL_NAME (s), buf);
- fputc_unfiltered (':', buf);
- }
-
- fputs_unfiltered (ls->label_name, buf);
- need_colon = 1;
- state->canonical->special_display = 1;
+ struct symbol *s;
+
+ /* No function was specified, so add the symbol name. */
+ gdb_assert (ls->labels.function_symbols != NULL
+ && (VEC_length (symbolp, ls->labels.function_symbols)
+ == 1));
+ s = VEC_index (symbolp, ls->labels.function_symbols, 0);
+ fputs_unfiltered (SYMBOL_NATURAL_NAME (s), buf);
+ fputc_unfiltered (':', buf);
}
- if (ls->line_offset.sign != LINE_OFFSET_UNKNOWN)
- {
- if (need_colon)
- fputc_unfiltered (':', buf);
- fprintf_filtered (buf, "%s%d",
- (ls->line_offset.sign == LINE_OFFSET_NONE ? ""
- : (ls->line_offset.sign
- == LINE_OFFSET_PLUS ? "+" : "-")),
- ls->line_offset.offset);
- }
+ fputs_unfiltered (ls->label_name, buf);
+ need_colon = 1;
+ state->canonical->special_display = 1;
+ }
- EVENT_LOCATION_LINESPEC (state->canonical->location)
- = ui_file_xstrdup (buf, NULL);
- ui_file_delete (buf);
+ if (ls->line_offset.sign != LINE_OFFSET_UNKNOWN)
+ {
+ if (need_colon)
+ fputc_unfiltered (':', buf);
+ fprintf_filtered (buf, "%s%d",
+ (ls->line_offset.sign == LINE_OFFSET_NONE ? ""
+ : (ls->line_offset.sign
+ == LINE_OFFSET_PLUS ? "+" : "-")),
+ ls->line_offset.offset);
}
+
+ EVENT_LOCATION_LINESPEC (state->canonical->location)
+ = ui_file_xstrdup (buf, NULL);
+ ui_file_delete (buf);
}
/* Given a line offset in LS, construct the relevant SALs. */
@@ -1990,6 +1981,27 @@ create_sals_line_offset (struct linespec_state *self,
return values;
}
+/* Convert the given ADDRESS into SaLs. */
+
+static struct symtabs_and_lines
+convert_address_location_to_sals (struct linespec_state *self,
+ CORE_ADDR address)
+{
+ struct symtab_and_line sal;
+ struct symtabs_and_lines sals = {NULL, 0};
+
+ sal = find_pc_line (address, 0);
+ sal.pc = address;
+ sal.section = find_pc_overlay (address);
+ sal.explicit_pc = 1;
+ add_sal_to_sals (self, &sals, &sal, core_addr_to_string (address), 1);
+
+ if (self->canonical != NULL)
+ self->canonical->location = new_address_location (address);
+
+ return sals;
+}
+
/* Create and return SALs from the linespec LS. */
static struct symtabs_and_lines
@@ -1997,18 +2009,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
{
struct symtabs_and_lines sals = {NULL, 0};
- if (ls->expression != NULL)
- {
- struct symtab_and_line sal;
-
- /* We have an expression. No other attribute is allowed. */
- sal = find_pc_line (ls->expr_pc, 0);
- sal.pc = ls->expr_pc;
- sal.section = find_pc_overlay (ls->expr_pc);
- sal.explicit_pc = 1;
- add_sal_to_sals (state, &sals, &sal, ls->expression, 1);
- }
- else if (ls->labels.label_symbols != NULL)
+ if (ls->labels.label_symbols != NULL)
{
/* We have just a bunch of functions/methods or labels. */
int i;
@@ -2108,8 +2109,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
The basic grammar of linespecs:
- linespec -> expr_spec | var_spec | basic_spec
- expr_spec -> '*' STRING
+ linespec -> var_spec | basic_spec
var_spec -> '$' (STRING | NUMBER)
basic_spec -> file_offset_spec | function_spec | label_spec
@@ -2205,33 +2205,7 @@ parse_linespec (linespec_parser *parser, const char **argptr)
token = linespec_lexer_lex_one (parser);
/* It must be either LSTOKEN_STRING or LSTOKEN_NUMBER. */
- if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '*')
- {
- char *expr;
- const char *copy;
-
- /* User specified an expression, *EXPR. */
- copy = expr = copy_token_string (token);
- cleanup = make_cleanup (xfree, expr);
- PARSER_RESULT (parser)->expr_pc = linespec_expression_to_pc (©);
- discard_cleanups (cleanup);
- PARSER_RESULT (parser)->expression = expr;
-
- /* This is a little hacky/tricky. If linespec_expression_to_pc
- did not evaluate the entire token, then we must find the
- string COPY inside the original token buffer. */
- if (*copy != '\0')
- {
- PARSER_STREAM (parser) = strstr (parser->lexer.saved_arg, copy);
- gdb_assert (PARSER_STREAM (parser) != NULL);
- }
-
- /* Consume the token. */
- linespec_lexer_consume_token (parser);
-
- goto convert_to_sals;
- }
- else if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '$')
+ if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '$')
{
char *var;
@@ -2453,6 +2427,12 @@ event_location_to_sals (linespec_parser *parser,
}
break;
+ case ADDRESS_LOCATION:
+ result
+ = convert_address_location_to_sals (PARSER_STATE (parser),
+ EVENT_LOCATION_ADDRESS (location));
+ break;
+
default:
gdb_assert_not_reached ("unhandled event location type");
}
@@ -2647,7 +2627,7 @@ initialize_defaults (struct symtab **default_symtab, int *default_line)
/* Evaluate the expression pointed to by EXP_PTR into a CORE_ADDR,
advancing EXP_PTR past any parsed text. */
-static CORE_ADDR
+CORE_ADDR
linespec_expression_to_pc (const char **exp_ptr)
{
if (current_program_space->executing_startup)
@@ -152,4 +152,8 @@ extern struct symtabs_and_lines decode_line_with_current_source (char *, int);
extern struct symtabs_and_lines decode_line_with_last_displayed (char *, int);
+/* Evaluate the expression pointed to by EXP_PTR into a CORE_ADDR,
+ advancing EXP_PTR past any parsed text. */
+
+extern CORE_ADDR linespec_expression_to_pc (const char **exp_ptr);
#endif /* defined (LINESPEC_H) */
@@ -53,6 +53,20 @@ new_linespec_location (const char *linespec)
return location;
}
+/* Create a new address location. The return result is malloc'd
+ and should be freed with delete_event_location. */
+
+struct event_location *
+new_address_location (CORE_ADDR addr)
+{
+ struct event_location *location;
+
+ location = XNEW (struct event_location);
+ initialize_event_location (location, ADDRESS_LOCATION);
+ EVENT_LOCATION_ADDRESS (location) = addr;
+ return location;
+}
+
/* Return a copy of the given SRC location. */
struct event_location *
@@ -71,6 +85,10 @@ copy_event_location (const struct event_location *src)
EVENT_LOCATION_LINESPEC (dst) = xstrdup (EVENT_LOCATION_LINESPEC (src));
break;
+ case ADDRESS_LOCATION:
+ EVENT_LOCATION_ADDRESS (dst) = EVENT_LOCATION_ADDRESS (src);
+ break;
+
default:
gdb_assert_not_reached ("unknown event location type");
}
@@ -111,6 +129,10 @@ delete_event_location (struct event_location *location)
xfree (EVENT_LOCATION_LINESPEC (location));
break;
+ case ADDRESS_LOCATION:
+ /* Nothing to do. */
+ break;
+
default:
gdb_assert_not_reached ("unknown event location type");
}
@@ -138,6 +160,12 @@ event_location_to_string_const (const struct event_location *location)
result = xstrdup (EVENT_LOCATION_LINESPEC (location));
break;
+ case ADDRESS_LOCATION:
+ result
+ = xstrprintf ("*%s",
+ core_addr_to_string (EVENT_LOCATION_ADDRESS (location)));
+ break;
+
default:
gdb_assert_not_reached ("unknown event location type");
}
@@ -171,9 +199,24 @@ string_to_event_location (char **stringp,
{
struct event_location *location;
- location = new_linespec_location (*stringp);
- if (*stringp != NULL)
- *stringp += strlen (*stringp);
+ /* First, check if the string is an address location. */
+ if (*stringp != NULL && **stringp == '*')
+ {
+ const char *arg, *orig;
+ CORE_ADDR addr;
+
+ orig = arg = *stringp;
+ addr = linespec_expression_to_pc (&arg);
+ location = new_address_location (addr);
+ *stringp += arg - orig;
+ }
+ else
+ {
+ /* Everything else is a linespec. */
+ location = new_linespec_location (*stringp);
+ if (*stringp != NULL)
+ *stringp += strlen (*stringp);
+ }
return location;
}
@@ -188,6 +231,9 @@ event_location_empty_p (const struct event_location *location)
case LINESPEC_LOCATION:
return EVENT_LOCATION_LINESPEC (location) == NULL;
+ case ADDRESS_LOCATION:
+ return 0;
+
default:
gdb_assert_not_reached ("unknown event location type");
}
@@ -27,7 +27,10 @@ struct language_defn;
enum event_location_type
{
/* A traditional linespec. */
- LINESPEC_LOCATION
+ LINESPEC_LOCATION,
+
+ /* An address in the inferior. */
+ ADDRESS_LOCATION
};
/* An event location used to set a stop event in the inferior.
@@ -47,6 +50,10 @@ struct event_location
probes. */
char *addr_string;
#define EVENT_LOCATION_LINESPEC(S) ((S)->u.addr_string)
+
+ /* An address in the inferior. */
+ CORE_ADDR address;
+#define EVENT_LOCATION_ADDRESS(S) ((S)->u.address)
} u;
/* Cached string representation of this location. This is used to
@@ -85,6 +92,12 @@ extern struct cleanup *
extern struct event_location *
new_linespec_location (const char *linespec);
+/* Create a new address location. The return result is malloc'd
+ and should be freed with delete_event_location. */
+
+extern struct event_location *
+ new_address_location (CORE_ADDR addr);
+
/* Return a copy of the given SRC location. */
extern struct event_location *
@@ -167,9 +167,8 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
struct frame_id frame_id;
PyObject *internal = NULL;
int internal_bp = 0;
- CORE_ADDR finish_pc, pc;
+ CORE_ADDR pc;
volatile struct gdb_exception except;
- char small_buf[100];
struct symbol *function;
if (!PyArg_ParseTupleAndKeywords (args, kwargs, "|OO", keywords,
@@ -291,10 +290,8 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
struct event_location location;
/* Set a breakpoint on the return address. */
- initialize_event_location (&location, LINESPEC_LOCATION);
- finish_pc = get_frame_pc (prev_frame);
- xsnprintf (small_buf, sizeof (small_buf), "*%s", hex_string (finish_pc));
- EVENT_LOCATION_LINESPEC (&location) = small_buf;
+ initialize_event_location (&location, ADDRESS_LOCATION);
+ EVENT_LOCATION_ADDRESS (&location) = get_frame_pc (prev_frame);
create_breakpoint (python_gdbarch,
&location, NULL, thread, NULL,
0,
@@ -2001,8 +2001,8 @@ spu_catch_start (struct objfile *objfile)
/* Use a numerical address for the set_breakpoint command to avoid having
the breakpoint re-set incorrectly. */
- initialize_event_location (&location, LINESPEC_LOCATION);
- EVENT_LOCATION_LINESPEC (&location) = xstrdup (core_addr_to_string (pc));
+ initialize_event_location (&location, ADDRESS_LOCATION);
+ EVENT_LOCATION_ADDRESS (&location) = pc;
create_breakpoint (get_objfile_arch (objfile), &location,
NULL /* cond_string */, -1 /* thread */,
NULL /* extra_string */,