This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 3/9] Explicit locations: use new location API
- From: Keith Seitz <keiths at redhat dot com>
- To: Doug Evans <xdje42 at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 07 May 2015 10:03:31 -0700
- Subject: Re: [PATCH v3 3/9] Explicit locations: use new location API
- Authentication-results: sourceware.org; auth=none
- References: <20150217220619 dot 1312 dot 39861 dot stgit at valrhona dot uglyboxes dot com> <20150217220643 dot 1312 dot 93245 dot stgit at valrhona dot uglyboxes dot com> <m34mq4l9s1 dot fsf at sspiff dot org>
On 03/01/2015 12:53 PM, Doug Evans wrote:
> Keith Seitz<keiths@redhat.com> writes:
>
>> >-/* 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_ARG parameter. If
>> >- non-zero, the function will parse ARG, extracting location,
>> >- condition, thread and extra string. Otherwise, ARG is just the
>> >- breakpoint's location, with condition, thread, and extra string
>> >- specified by the COND_STRING, THREAD and EXTRA_STRING parameters.
>> >+/* Set a breakpoint. This function is shared between CLI and MI functions
>> >+ for setting a breakpoint at LOCATION.
>> >+
>> >+ This function has two major modes of operations, selected by the PARSE_ARG
>> >+ parameter.
>> >+
>> >+ If PARSE_ARG is zero, LOCATION is just the breakpoint's location,
>> >+ with condition, thread, and extra string specified by the COND_STRING,
>> >+ THREAD, and EXTRA_STRING parameters.
>> >+
>> >+ If PARSE_ARG is non-zero and LOCATION is a linespec location,
>> >+ this function will attempt to extract the location, condition, thread,
>> >+ and extra string from the linespec stored in LOCATION.
>> >+ For non-linespec locations EXTRA_STRING is parsed for condition, thread,
>> >+ and extra string.
>> >+
>> > 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. */
>> >+ from the internal breakpoint count.
>> >+
>> >+ Returns true if any breakpoint was created; false otherwise. */
>> >
>> > int
>> > create_breakpoint (struct gdbarch *gdbarch,
>> >- char *arg, char *cond_string,
>> >+ const struct event_location *location, char *cond_string,
>> > int thread, char *extra_string,
>> > int parse_arg,
> ====
> Rename parse_arg to parse_location or parse_extra?
> ["parse_arg" made sense when "arg" was a parameter, but it's confusing now]
Sure. I've picked parse_extra and updated the comment. That also works
well with your next comment:
>> >@@ -10082,6 +10130,8 @@ create_breakpoint (struct gdbarch *gdbarch,
>> > breakpoint. */
>> > if (!pending)
>> > {
>> >+ char *arg = extra_string;
> ====
> I gather having "arg" is here to minimize changes, but it reduces
> readability a bit.
I've simply removed the local variable `arg' and passed extra_string to
find_condition_and_thread.
>> >@@ -10159,7 +10222,15 @@ create_breakpoint (struct gdbarch *gdbarch,
>> > }
>> > b->cond_string = cond_string;
>> > }
>> >- b->extra_string = NULL;
>> >+
>> >+ /* Make a private copy of extra_string for the breakpoint. */
>> >+ if (extra_string != NULL)
>> >+ {
>> >+ b->extra_string = xstrdup (extra_string);
>> >+ make_cleanup (xfree, b->extra_string);
> ====
> This cleanup is odd. I would have expected a cleanup for "b" to
> completely clean itself up. However, it looks like no cleanup for "b"
> is ever created. So that's an existing bug which I wouldn't impose
> on this patchset to fix, but OTOH this cleanup still feels wrong.
I've rewritten/rearranged this a little bit so that it more closely
mimics what is done with cond_string immediately before it. [And, you're
right -- `b' doesn't get "cleaned up" in the way you'd expect. It took
me a couple of read-throughs to figure this out, too.]
> Also, the docs of extra_string in breakpoint.h are lacking.
Docs for create_breakpoint don't even exist in breakpoint.h! :-) I've
moved them there.
> It's not something that has to be fixed with this patchset,
> but I'm having to dig too deep to figure out how extra_string
> is different from cond_string. Can I trouble you to clarify things?
I have made an attempt at it. [It turns out that this needed rewriting
anyway -- I apparently forgot to fully update the comment on this last
revision.]
>> >@@ -10206,16 +10277,21 @@ break_command_1 (char *arg, int flag, int from_tty)
>> > : bp_breakpoint);
>> > struct breakpoint_ops *ops;
>> > const char *arg_cp = arg;
>> >+ struct event_location *location;
>> >+ struct cleanup *cleanup;
>> >+
>> >+ location = string_to_event_location (&arg, current_language);
>> >+ cleanup = make_cleanup_delete_event_location (location);
>> >
>> > /* Matching breakpoints on probes. */
>> >- if (arg && probe_linespec_to_ops (&arg_cp) != NULL)
>> >+ if (arg_cp != NULL && probe_linespec_to_ops (&arg_cp) != NULL)
> ====
> Extra space before &&.
>
Fixed.
>> >@@ -14582,21 +14691,37 @@ location_to_sals (struct breakpoint *b, char *addr_string, int *found)
>> >
>> > for (i = 0; i < sals.nelts; ++i)
>> > resolve_sal_pc (&sals.sals[i]);
>> >- if (b->condition_not_parsed && s && s[0])
>> >+ if (b->condition_not_parsed)
>> > {
>> >- char *cond_string, *extra_string;
>> >- int thread, task;
>> >+ const char *s = b->extra_string;
>> >
>> >- find_condition_and_thread (s, sals.sals[0].pc,
>> >- &cond_string, &thread, &task,
>> >- &extra_string);
>> >- if (cond_string)
>> >- b->cond_string = cond_string;
>> >- b->thread = thread;
>> >- b->task = task;
>> >- if (extra_string)
>> >- b->extra_string = extra_string;
>> >- b->condition_not_parsed = 0;
>> >+ if (s != NULL && *s != '\0')
> ====
> If we have a rule that b->extra_string is never "" (only NULL) then
> we can simplify this, and even remove this extra nested "if".
> E.g., just have
>
> + if (b->condition_not_parsed && b->extra_string != NULL)
>
Yes, good catch!
>> >@@ -433,10 +434,9 @@ linespec_lexer_lex_keyword (const char *p)
>> > int len = strlen (linespec_keywords[i]);
>> >
>> > /* If P begins with one of the keywords and the next
>> >- character is not a valid identifier character,
>> >- we have found a keyword. */
>> >+ character is whitespace, we have found a keyword. */
>> > if (strncmp (p, linespec_keywords[i], len) == 0
>> >- && !(isalnum (p[len]) || p[len] == '_'))
>> >+ && isspace (p[len]))
> ====
> I think I'm ok with this change, but it seems separate from this patchset.
> Submit it separately?
Some time ago, I committed a more complete patch for this.
>> >@@ -1765,21 +1765,29 @@ linespec_parse_basic (linespec_parser *parser)
>> > STATE->canonical. */
>> >
>> > static void
>> >-canonicalize_linespec (struct linespec_state *state, linespec_p ls)
>> >+canonicalize_linespec (struct linespec_state *state, const linespec_p ls)
>> > {
>> >+ char *tmp;
>> >+
>> > /* 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->addr_string = xstrdup (ls->expression);
>> >+ {
>> >+ tmp = ASTRDUP (ls->expression);
> ====
> No need to make this change now (assuming it's doable),
> but I'm curious if the parser needs to modify its argument
> or whether we can pass a const char * and remove the ASTRDUP here
> (and elsewhere).
>
s/its argument/its state/ ? Otherwise I'm not entirely sure what you
mean. The linespec parser takes all const arguments already
(parse_linespec [now] takes const char *, decode_line* take const struct
event_location *).
I could change canonicalize_linespec to take the canonical location
instead of the entire parser state, though, yes, that's doable, but it
still needs to be mutable.
>> >@@ -1999,8 +2001,10 @@ 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 */,
>> >+ p = ASTRDUP (core_addr_to_string (pc));
> ====
> The leading '*' got dropped.
> I can imagine this is just temporary anyway, as a later patch
> (address locations) redoes this again.
> I'm ok with leaving it as is, but I'd be remiss if I didn't
> point it out.
Yes, it did drop the '*'! For completeness, I've put it back, even if
temporary.
Keith