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

Keith Seitz
2015-05-19T21:30:19Z

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