[RFA,5/9] Explicit locations v2 - Add probe locations

  This is also an update of this patch based on feedback from previous 
patch reviews.

Keith

Changes since last revision:
   - use make_cleanup_delete_event_location
   - remove SAVE_SPEC
   - new_probe_location

ChangeLog
2014-05-29  Keith Seitz  <keiths@redhat.com>

	* break-catch-throw.c (re_set_exception_catchpoint): Convert
	linespec for stap probe to probe location.
	* breakpoint.c (create_longjmp_master_breakpoint): Likewise.
	(create_exception_master_breakpoint): Likewise.
	(break_command_1): Remove local variable `arg_cp'.
	Check location type to set appropriate breakpoint ops methods.
	(trace_command): Likewise.
	* linespec.c (event_location_to_sals): Handle probe locations.
	* location.c (copy_event_location): Likewise.
	(delete_event_location): Likewise.
	(event_location_to_string): Likewise.
	(string_to_event_location): Likewise.
	(event_location_empty_p): Handel probe locations.
	* location.h (enum event_location_type): Add PROBE_LOCATION.
	(EVENT_LOCATION_PROBE): Define accessor macro.
	* probe.c (parse_probes): Assert that LOCATION is a probe location.
	Convert linespec into probe location.
  

Hi.  Comments inline, mostly nits.

Keith Seitz writes:
 > This is also an update of this patch based on feedback from previous 
 > patch reviews.
 > 
 > Keith
 > 
 > Changes since last revision:
 >    - use make_cleanup_delete_event_location
 >    - remove SAVE_SPEC
 >    - new_probe_location
 > 
 > ChangeLog
 > 2014-05-29  Keith Seitz  <keiths@redhat.com>
 > 
 > 	* break-catch-throw.c (re_set_exception_catchpoint): Convert
 > 	linespec for stap probe to probe location.
 > 	* breakpoint.c (create_longjmp_master_breakpoint): Likewise.
 > 	(create_exception_master_breakpoint): Likewise.
 > 	(break_command_1): Remove local variable `arg_cp'.
 > 	Check location type to set appropriate breakpoint ops methods.
 > 	(trace_command): Likewise.
 > 	* linespec.c (event_location_to_sals): Handle probe locations.
 > 	* location.c (copy_event_location): Likewise.
 > 	(delete_event_location): Likewise.
 > 	(event_location_to_string): Likewise.
 > 	(string_to_event_location): Likewise.
 > 	(event_location_empty_p): Handel probe locations.

"Handle ...", or just "Likewise."

 > 	* location.h (enum event_location_type): Add PROBE_LOCATION.
 > 	(EVENT_LOCATION_PROBE): Define accessor macro.
 > 	* probe.c (parse_probes): Assert that LOCATION is a probe location.
 > 	Convert linespec into probe location.
 > 
 > 
 > 
 > diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
 > index e2c8365..207e7af 100644
 > --- a/gdb/break-catch-throw.c
 > +++ b/gdb/break-catch-throw.c
 > @@ -214,8 +214,8 @@ re_set_exception_catchpoint (struct breakpoint *self)
 >      {
 >        struct event_location location;
 >  
 > -      initialize_event_location (&location, LINESPEC_LOCATION);
 > -      EVENT_LOCATION_LINESPEC (&location)
 > +      initialize_event_location (&location, PROBE_LOCATION);
 > +      EVENT_LOCATION_PROBE (&location)
 >  	= ASTRDUP (exception_functions[kind].probe);
 >        sals = parse_probes (&location, NULL);
 >      }
 > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
 > index ef60f44..345b104 100644
 > --- a/gdb/breakpoint.c
 > +++ b/gdb/breakpoint.c
 > @@ -3387,7 +3387,7 @@ create_longjmp_master_breakpoint (void)
 >  					      bp_longjmp_master,
 >  					      &internal_breakpoint_ops);
 >  	      b->location
 > -		= new_linespec_location ("-probe-stap libc:longjmp");
 > +		= new_probe_location ("-probe-stap libc:longjmp");
 >  	      b->enable_state = bp_disabled;
 >  	    }
 >  
 > @@ -3551,7 +3551,7 @@ create_exception_master_breakpoint (void)
 >  					      bp_exception_master,
 >  					      &internal_breakpoint_ops);
 >  	      b->location
 > -		= new_linespec_location ("-probe-stap libgcc:unwind");
 > +		= new_probe_location ("-probe-stap libgcc:unwind");
 >  	      b->enable_state = bp_disabled;
 >  	    }
 >  
 > @@ -10068,7 +10068,6 @@ break_command_1 (char *arg, int flag, int from_tty)
 >  			     ? bp_hardware_breakpoint
 >  			     : bp_breakpoint);
 >    struct breakpoint_ops *ops;
 > -  const char *arg_cp = arg;
 >    struct event_location *location;
 >    struct cleanup *cleanup;
 >  
 > @@ -10076,7 +10075,8 @@ break_command_1 (char *arg, int flag, int from_tty)
 >    cleanup = make_cleanup_delete_event_location (location);
 >  
 >    /* Matching breakpoints on probes.  */
 > -  if (arg_cp != NULL  && probe_linespec_to_ops (&arg_cp) != NULL)
 > +  if (location != NULL
 > +      && EVENT_LOCATION_TYPE (location) == PROBE_LOCATION)
 >      ops = &bkpt_probe_breakpoint_ops;
 >    else
 >      ops = &bkpt_breakpoint_ops;
 > @@ -15432,11 +15432,11 @@ trace_command (char *arg, int from_tty)
 >    struct breakpoint_ops *ops;
 >    struct event_location *location;
 >    struct cleanup *back_to;
 > -  const char *arg_cp = arg;
 >  
 >    location = string_to_event_location (&arg, current_language);
 >    back_to = make_cleanup_delete_event_location (location);
 > -  if (arg_cp != NULL && probe_linespec_to_ops (&arg_cp) != NULL)
 > +  if (location != NULL
 > +      && EVENT_LOCATION_TYPE (location) == PROBE_LOCATION)
 >      ops = &tracepoint_probe_breakpoint_ops;
 >    else
 >      ops = &tracepoint_breakpoint_ops;
 > diff --git a/gdb/linespec.c b/gdb/linespec.c
 > index 8ea0c7b..deda86e 100644
 > --- a/gdb/linespec.c
 > +++ b/gdb/linespec.c
 > @@ -2433,6 +2433,11 @@ event_location_to_sals (linespec_parser *parser,
 >  					    EVENT_LOCATION_ADDRESS (location));
 >        break;
 >  
 > +    case PROBE_LOCATION:
 > +      /* Probes are handled by their own decoders.  */
 > +       gdb_assert_not_reached ("attempt to decode probe location");

extra space of indentation

 > +      break;
 > +
 >      default:
 >        gdb_assert_not_reached ("unhandled event location type");
 >      }
 > diff --git a/gdb/location.c b/gdb/location.c
 > index 3c430db..eafa27d 100644
 > --- a/gdb/location.c
 > +++ b/gdb/location.c
 > @@ -67,6 +67,21 @@ new_address_location (CORE_ADDR addr)
 >    return location;
 >  }
 >  
 > +/* Create a new probe location.  The return result is malloc'd
 > +   and should be freed with delete_event_location.  */
 > +
 > +struct event_location *
 > +new_probe_location (const char *probe)
 > +{
 > +  struct event_location *location;
 > +
 > +  location = XNEW (struct event_location);
 > +  initialize_event_location (location, PROBE_LOCATION);
 > +  if (probe != NULL)
 > +    EVENT_LOCATION_PROBE (location) = xstrdup (probe);
 > +  return location;
 > +}
 > +
 >  /* Return a copy of the given SRC location.  */
 >  
 >  struct event_location *
 > @@ -89,6 +104,10 @@ copy_event_location (const struct event_location *src)
 >        EVENT_LOCATION_ADDRESS (dst) = EVENT_LOCATION_ADDRESS (src);
 >        break;
 >  
 > +    case PROBE_LOCATION:
 > +      EVENT_LOCATION_PROBE (dst) = xstrdup (EVENT_LOCATION_PROBE (src));
 > +      break;
 > +
 >      default:
 >        gdb_assert_not_reached ("unknown event location type");
 >      }
 > @@ -133,6 +152,10 @@ delete_event_location (struct event_location *location)
 >  	  /* Nothing to do.  */
 >  	  break;
 >  
 > +	case PROBE_LOCATION:
 > +	  xfree (EVENT_LOCATION_PROBE (location));
 > +	  break;
 > +
 >  	default:
 >  	  gdb_assert_not_reached ("unknown event location type");
 >  	}
 > @@ -166,6 +189,10 @@ event_location_to_string_const (const struct event_location *location)
 >  		      core_addr_to_string (EVENT_LOCATION_ADDRESS (location)));
 >        break;
 >  
 > +    case PROBE_LOCATION:
 > +      result = xstrdup (EVENT_LOCATION_PROBE (location));
 > +      break;
 > +
 >      default:
 >        gdb_assert_not_reached ("unknown event location type");
 >      }
 > @@ -212,10 +239,23 @@ string_to_event_location (char **stringp,
 >      }
 >    else
 >      {
 > -      /* Everything else is a linespec.  */
 > -      location = new_linespec_location (*stringp);
 > -      if (*stringp != NULL)
 > -	*stringp += strlen (*stringp);
 > +      const char *cs;
 > +
 > +      /* Next, try the input as a probe spec.  */
 > +      cs = *stringp;
 > +      if (cs != NULL && probe_linespec_to_ops (&cs) != NULL)
 > +	{
 > +	  location = new_probe_location (*stringp);
 > +	  if (*stringp != NULL)

This test is unnecessary (right?).

btw, when will this function be passed *stringp == NULL?
Can this function have an "early exit" if *stringp == NULL?
[assuming that makes sense]

 > +	    *stringp += strlen (*stringp);
 > +	}
 > +      else
 > +	{
 > +	  /* Everything else is a linespec.  */
 > +	  location = new_linespec_location (*stringp);
 > +	  if (*stringp != NULL)
 > +	    *stringp += strlen (*stringp);
 > +	}
 >      }
 >  
 >    return location;
 > @@ -234,6 +274,9 @@ event_location_empty_p (const struct event_location *location)
 >      case ADDRESS_LOCATION:
 >        return 0;
 >  
 > +    case PROBE_LOCATION:
 > +      return EVENT_LOCATION_PROBE (location) == NULL;
 > +
 >      default:
 >        gdb_assert_not_reached ("unknown event location type");
 >      }
 > diff --git a/gdb/location.h b/gdb/location.h
 > index 1daefb6..e6f14d9 100644
 > --- a/gdb/location.h
 > +++ b/gdb/location.h
 > @@ -30,7 +30,10 @@ enum event_location_type
 >    LINESPEC_LOCATION,
 >  
 >    /* An address in the inferior.  */
 > -  ADDRESS_LOCATION
 > +  ADDRESS_LOCATION,
 > +
 > +  /* A probe location.  */
 > +  PROBE_LOCATION
 >  };
 >  
 >  /* An event location used to set a stop event in the inferior.
 > @@ -50,6 +53,7 @@ struct event_location
 >         probes.  */
 >      char *addr_string;
 >  #define EVENT_LOCATION_LINESPEC(S) ((S)->u.addr_string)
 > +#define EVENT_LOCATION_PROBE(S) ((S)->u.addr_string)
 >  
 >      /* An address in the inferior.  */
 >      CORE_ADDR address;
 > @@ -98,6 +102,12 @@ extern struct event_location *
 >  extern struct event_location *
 >    new_address_location (CORE_ADDR addr);
 >  
 > +/* Create a new probe location.  The return result is malloc'd
 > +   and should be freed with delete_event_location.  */
 > +
 > +extern struct event_location *
 > +  new_probe_location (const char *probe);
 > +
 >  /* Return a copy of the given SRC location.  */
 >  
 >  extern struct event_location *
 > diff --git a/gdb/probe.c b/gdb/probe.c
 > index c7597d9..2ff5d86 100644
 > --- a/gdb/probe.c
 > +++ b/gdb/probe.c
 > @@ -58,7 +58,8 @@ parse_probes (struct event_location *location,
 >    result.sals = NULL;
 >    result.nelts = 0;
 >  
 > -  arg_start = EVENT_LOCATION_LINESPEC (location);
 > +  gdb_assert (EVENT_LOCATION_TYPE (location) == PROBE_LOCATION);
 > +  arg_start = EVENT_LOCATION_PROBE (location);
 >  
 >    cs = arg_start;
 >    probe_ops = probe_linespec_to_ops (&cs);
 > @@ -173,12 +174,12 @@ parse_probes (struct event_location *location,
 >      {
 >        canonical->special_display = 1;
 >        canonical->pre_expanded = 1;
 > -      canonical->location = new_linespec_location (NULL);
 > -      EVENT_LOCATION_LINESPEC (canonical->location)
 > +      canonical->location = new_probe_location (NULL);
 > +      EVENT_LOCATION_PROBE (canonical->location)
 >  	= savestring (arg_start, arg_end - arg_start);

I see why you pass NULL to new_probe_location and then set EVENT_LOCATION_PROBE
afterwards (otherwise two copies of the string would be malloc'd, and you'd
need to deal with freeing one of them).  One could add a version of
new_probe_location that took a char* and a length, but that seems excessive.
Another place where c++ would allow cleaner code.

 >      }
 >  
 > -  EVENT_LOCATION_LINESPEC (location) = arg_end;
 > +  EVENT_LOCATION_PROBE (location) = arg_end;
 >    do_cleanups (cleanup);

The function starts out with:

  arg_start = EVENT_LOCATION_PROBE (location);

and at the end we do this:

  EVENT_LOCATION_PROBE (location) = arg_end;

IWBN to document why we do this, it's not obvious to me why that is.
[doesn't have to be part of this patch set though]

 >  
 >    return result;
  

On Sat, Aug 2, 2014 at 4:38 PM, Doug Evans <dje@google.com> wrote:
> [...]
>  > diff --git a/gdb/probe.c b/gdb/probe.c
>  > index c7597d9..2ff5d86 100644
>  > --- a/gdb/probe.c
>  > +++ b/gdb/probe.c
>  > @@ -58,7 +58,8 @@ parse_probes (struct event_location *location,
>  >    result.sals = NULL;
>  >    result.nelts = 0;
>  >
>  > -  arg_start = EVENT_LOCATION_LINESPEC (location);
>  > +  gdb_assert (EVENT_LOCATION_TYPE (location) == PROBE_LOCATION);
>  > +  arg_start = EVENT_LOCATION_PROBE (location);
>  >
>  >    cs = arg_start;
>  >    probe_ops = probe_linespec_to_ops (&cs);
>  > @@ -173,12 +174,12 @@ parse_probes (struct event_location *location,
>  >      {
>  >        canonical->special_display = 1;
>  >        canonical->pre_expanded = 1;
>  > -      canonical->location = new_linespec_location (NULL);
>  > -      EVENT_LOCATION_LINESPEC (canonical->location)
>  > +      canonical->location = new_probe_location (NULL);
>  > +      EVENT_LOCATION_PROBE (canonical->location)
>  >      = savestring (arg_start, arg_end - arg_start);
>
> I see why you pass NULL to new_probe_location and then set EVENT_LOCATION_PROBE
> afterwards (otherwise two copies of the string would be malloc'd, and you'd
> need to deal with freeing one of them).  One could add a version of
> new_probe_location that took a char* and a length, but that seems excessive.
> Another place where c++ would allow cleaner code.

OTOH, is that, or any other variation, excessive?
It's preferable to not have users of object foo know how to physically
construct a foo.

/just a thought for discussion's sake - I wouldn't change the patch if
this is but one of many similar instances, unless it makes sense to
change all of them.