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

[Keith Seitz <keiths@redhat.com>]

On 05/17/2015 10:19 PM, Doug Evans wrote:
> Keith Seitz <keiths@redhat.com> writes:
>> diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
>> index 927176f..9449aa5 100644
>> --- a/gdb/break-catch-throw.c
>> +++ b/gdb/break-catch-throw.c
>> @@ -35,6 +35,7 @@
>>  #include "cp-abi.h"
>>  #include "gdb_regex.h"
>>  #include "cp-support.h"
>> +#include "location.h"
>>  
>>  /* Enums for exception-handling support.  */
>>  enum exception_event_kind
>> @@ -210,25 +211,31 @@ re_set_exception_catchpoint (struct breakpoint *self)
>>    struct symtabs_and_lines sals_end = {0};
>>    struct cleanup *cleanup;
>>    enum exception_event_kind kind = classify_exception_breakpoint (self);
>> +  struct event_location *location;
>>  
>>    /* We first try to use the probe interface.  */
>>    TRY
>>      {
>>        char *spec = ASTRDUP (exception_functions[kind].probe);
>>  
> 
> Not something to be done with this patch (let's reach
> closure and get this sucker checked in :-)),
> but IWBN to have an API where we didn't have to do the
> ASTRDUP.  E.g., have new_linespec_location_const or some such.

Understood.

> 
>> -      sals = parse_probes (&spec, NULL);
>> +      location = new_linespec_location (&spec);
>> +      cleanup = make_cleanup_delete_event_location (location);
>> +      sals = parse_probes (location, NULL);
>> +      do_cleanups (cleanup);
>>      }
>>  
>>    CATCH (e, RETURN_MASK_ERROR)
>>      {
>> -
>>        /* Using the probe interface failed.  Let's fallback to the normal
>>  	 catchpoint mode.  */
>>        TRY
>>  	{
>>  	  char *spec = ASTRDUP (exception_functions[kind].function);
>>  
>> -	  self->ops->decode_location (self, &spec, &sals);
>> +	  location = new_linespec_location (&spec);
>> +	  cleanup = make_cleanup_delete_event_location (location);
>> +	  self->ops->decode_location (self, location, &sals);
>> +	  do_cleanups (cleanup);
>>  	}
>>        CATCH (ex, RETURN_MASK_ERROR)
>>  	{
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 31b1f82..549bfd0 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
[snip]
>> @@ -13451,19 +13525,28 @@ dprintf_after_condition_true (struct bpstats *bs)
>>     markers (`-m').  */
>>  
>>  static void
>> -strace_marker_create_sals_from_location (char **arg,
>> +strace_marker_create_sals_from_location (const struct event_location *location,
>>  					 struct linespec_result *canonical,
>> -					 enum bptype type_wanted,
>> -					 char *addr_start, char **copy_arg)
>> +					 enum bptype type_wanted)
>>  {
>>    struct linespec_sals lsal;
>> +  const char *arg_start, *arg;
>>  
>> -  lsal.sals = decode_static_tracepoint_spec (arg);
>> +  arg = arg_start = get_linespec_location (location);
>> +  lsal.sals = decode_static_tracepoint_spec (&arg);
>>  
>> -  *copy_arg = savestring (addr_start, *arg - addr_start);
>> +  if (canonical != NULL)
> 
> Why the canonical != NULL test here?
> Outside this "if" below it's assumed canonical != NULL.
> 

Right -- fixed that. Thank you.

>> +    {
>> +      char *str;
>> +      struct cleanup *cleanup;
>>  
>> -  canonical->addr_string = xstrdup (*copy_arg);
>> -  lsal.canonical = xstrdup (*copy_arg);
>> +      str = savestring (arg_start, arg - arg_start);
>> +      cleanup = make_cleanup (xfree, str);
>> +      canonical->location = new_linespec_location (&str);
>> +      do_cleanups (cleanup);
>> +    }
>> +
>> +  lsal.canonical = xstrdup (event_location_to_string (canonical->location));
>>    VEC_safe_push (linespec_sals, canonical->sals, &lsal);
>>  }
>>  
[snip]
>> @@ -14068,22 +14158,21 @@ update_breakpoint_locations (struct breakpoint *b,
>>    update_global_location_list (UGLL_MAY_INSERT);
>>  }
>>  
>> -/* Find the SaL locations corresponding to the given ADDR_STRING.
>> +/* Find the SaL locations corresponding to the given LOCATION.
>>     On return, FOUND will be 1 if any SaL was found, zero otherwise.  */
>>  
>>  static struct symtabs_and_lines
>> -location_to_sals (struct breakpoint *b, char *addr_string, int *found)
>> +location_to_sals (struct breakpoint *b, struct event_location *location,
>> +		  int *found)
>>  {
>> -  char *s;
>>    struct symtabs_and_lines sals = {0};
>>    struct gdb_exception exception = exception_none;
>>  
>>    gdb_assert (b->ops != NULL);
>> -  s = addr_string;
>>  
>>    TRY
>>      {
>> -      b->ops->decode_location (b, &s, &sals);
>> +      b->ops->decode_location (b, location, &sals);
>>      }
>>    CATCH (e, RETURN_MASK_ERROR)
>>      {
>> @@ -14125,12 +14214,13 @@ 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 && b->extra_string != NULL)
>>  	{
>>  	  char *cond_string, *extra_string;
>>  	  int thread, task;
>> +	  const char *orig = b->extra_string;
>>  
>> -	  find_condition_and_thread (s, sals.sals[0].pc,
>> +	  find_condition_and_thread (b->extra_string, sals.sals[0].pc,
>>  				     &cond_string, &thread, &task,
>>  				     &extra_string);
>>  	  if (cond_string)
> 
> The code is a bit odd here, at first glance.
> We xfree b->extra_string before assigning a new value,
> but not b->cond_string.
> I'm guessing this is because the latter will always be NULL here.
> Can you add an assert to that effect?  I.e.,
> 
>   gdb_assert (b->cond_string == NULL);

Yeah, you have that right. At least that's how I read and interpreted
that. I've added the assert.

New revision coming your way shortly.

Keith