Re: [PATCH v3 3/9] Explicit locations: use new location API

[Keith Seitz <keiths at redhat dot com>]

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