This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v3 4/9] Explicit locations: introduce address locations


Keith Seitz <keiths@redhat.com> writes:
> This patch adds support for address locations, of the form "*ADDR".
> [Support for address linespecs has been removed/replaced by this "new"
> location type.] This patch also converts any existing address locations
> from its previous linespec type.
>
> gdb/ChangeLog:
>
> 	* breakpoint.c (create_thread_event_breakpoint): Convert
> 	linespec to address location.
> 	(init_breakpoint_sal): Likewise.
> 	* linespec.c (canonicalize_linespec): Do not handle address
> 	locations here.
> 	(convert_address_location_to_sals): New function; contents moved
> 	from ...
> 	(convert_linespc_to_sals): ... here.
> 	(parse_linespec): Remove address locations from linespec grammar.
> 	Remove handling of address locations.
> 	(linespec_lex_to_end): Remove handling of address linespecs.
> 	(event_location_to_sals): Handle ADDRESS_LOCATION.
> 	(linespec_expression_to_pc): Export.
> 	* linespec.h (linespec_expression_to_pc): Add declaration.
> 	* location.c (struct event_location.u) <address>: New member.
> 	(new_address_location, get_address_location): New functions.
> 	(copy_event_location): Handle address locations.
> 	(delete_event_location): Likewise.
> 	(event_location_to_string): Likewise.
> 	(string_to_event_location): Likewise.
> 	(event_location_empty_p): Likewise.
> 	* location.h (enum event_location_type): Add ADDRESS_LOCATION.
> 	(new_address_location, get_address_location): Declare.
> 	* python/py-finishbreakpoint.c (bpfinishpy_init): Convert linespec
> 	to address location.
> 	* spu-tdep.c (spu_catch_start): Likewise.

Hi.
A few comments inline.
grep for ^====

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 5c3be10..ec0c6f8 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7629,19 +7629,14 @@ delete_std_terminate_breakpoint (void)
>  struct breakpoint *
>  create_thread_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
>  {
> -  char *tmp;
>    struct breakpoint *b;
> -  struct cleanup *cleanup;
>  
>    b = create_internal_breakpoint (gdbarch, address, bp_thread_event,
>  				  &internal_breakpoint_ops);
>  
>    b->enable_state = bp_enabled;
>    /* location has to be used or breakpoint_re_set will delete me.  */
> -  tmp = xstrprintf ("*%s", paddress (b->loc->gdbarch, b->loc->address));
> -  cleanup = make_cleanup (xfree, tmp);
> -  b->location = new_linespec_location (&tmp);
> -  do_cleanups (cleanup);
> +  b->location = new_address_location (b->loc->address);
>  
>    update_global_location_list_nothrow (UGLL_MAY_INSERT);
>  
> @@ -9614,16 +9609,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
>    if (location != NULL)
>      b->location = location;
>    else
> -    {
> -      char *tmp;
> -      struct cleanup *cleanup;
> -
> -      tmp = xstrprintf ("*%s",
> -			    paddress (b->loc->gdbarch, b->loc->address));
> -      cleanup = make_cleanup (xfree, tmp);
> -      b->location = new_linespec_location (&tmp);
> -      do_cleanups (cleanup);
> -   }
> +    b->location = new_address_location (b->loc->address);
>    b->filter = filter;
>  }
>  
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 29e32e3..9c36d6c 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -316,7 +316,7 @@ static void iterate_over_file_blocks (struct symtab *symtab,
>  static void initialize_defaults (struct symtab **default_symtab,
>  				 int *default_line);
>  
> -static CORE_ADDR linespec_expression_to_pc (const char **exp_ptr);
> +CORE_ADDR linespec_expression_to_pc (const char **exp_ptr);
>  
>  static struct symtabs_and_lines decode_objc (struct linespec_state *self,
>  					     linespec_p ls,
> @@ -1768,79 +1768,69 @@ static void
>  canonicalize_linespec (struct linespec_state *state, const linespec_p ls)
>  {
>    char *tmp;
> +  struct ui_file *buf;
> +  int need_colon = 0;
> +  struct cleanup *cleanup;
>  
>    /* 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)
> +  buf = mem_fileopen ();
> +  cleanup = make_cleanup_ui_file_delete (buf);
> +
> +  if (ls->source_filename)
>      {
> -      tmp = ASTRDUP (ls->expression);
> -      state->canonical->location = new_linespec_location (&tmp);
> +      fputs_unfiltered (ls->source_filename, buf);
> +      need_colon = 1;
>      }
> -  else
> -    {
> -      struct ui_file *buf;
> -      int need_colon = 0;
> -      struct cleanup *cleanup;
>  
> -      buf = mem_fileopen ();
> -      cleanup = make_cleanup_ui_file_delete (buf);
> +  if (ls->function_name)
> +    {
> +      if (need_colon)
> +	fputc_unfiltered (':', buf);
> +      fputs_unfiltered (ls->function_name, buf);
> +      need_colon = 1;
> +    }
>  
> -      if (ls->source_filename)
> -	{
> -	  fputs_unfiltered (ls->source_filename, buf);
> -	  need_colon = 1;
> -	}
> +  if (ls->label_name)
> +    {
> +      if (need_colon)
> +	fputc_unfiltered (':', buf);
>  
> -      if (ls->function_name)
> +      if (ls->function_name == NULL)
>  	{
> -	  if (need_colon)
> -	    fputc_unfiltered (':', buf);
> -	  fputs_unfiltered (ls->function_name, buf);
> -	  need_colon = 1;
> +	  struct symbol *s;
> +
> +	  /* No function was specified, so add the symbol name.  */
> +	  gdb_assert (ls->labels.function_symbols != NULL
> +		      && (VEC_length (symbolp, ls->labels.function_symbols)
> +			  == 1));
> +	  s = VEC_index (symbolp, ls->labels.function_symbols, 0);
> +	  fputs_unfiltered (SYMBOL_NATURAL_NAME (s), buf);
> +	  fputc_unfiltered (':', buf);
>  	}
>  
> -      if (ls->label_name)
> -	{
> -	  if (need_colon)
> -	    fputc_unfiltered (':', buf);
> -
> -	  if (ls->function_name == NULL)
> -	    {
> -	      struct symbol *s;
> -
> -	      /* No function was specified, so add the symbol name.  */
> -	      gdb_assert (ls->labels.function_symbols != NULL
> -			  && (VEC_length (symbolp, ls->labels.function_symbols)
> -			      == 1));
> -	      s = VEC_index (symbolp, ls->labels.function_symbols, 0);
> -	      fputs_unfiltered (SYMBOL_NATURAL_NAME (s), buf);
> -	      fputc_unfiltered (':', buf);
> -	    }
> -
> -	  fputs_unfiltered (ls->label_name, buf);
> -	  need_colon = 1;
> -	  state->canonical->special_display = 1;
> -	}
> -
> -      if (ls->line_offset.sign != LINE_OFFSET_UNKNOWN)
> -	{
> -	  if (need_colon)
> -	    fputc_unfiltered (':', buf);
> -	  fprintf_filtered (buf, "%s%d",
> -			    (ls->line_offset.sign == LINE_OFFSET_NONE ? ""
> -			     : (ls->line_offset.sign
> -				== LINE_OFFSET_PLUS ? "+" : "-")),
> -			    ls->line_offset.offset);
> -	}
> +      fputs_unfiltered (ls->label_name, buf);
> +      need_colon = 1;
> +      state->canonical->special_display = 1;
> +    }
>  
> -      tmp = ui_file_xstrdup (buf, NULL);
> -      make_cleanup (xfree, tmp);
> -      state->canonical->location = new_linespec_location (&tmp);
> -      do_cleanups (cleanup);
> +  if (ls->line_offset.sign != LINE_OFFSET_UNKNOWN)
> +    {
> +      if (need_colon)
> +	fputc_unfiltered (':', buf);
> +      fprintf_filtered (buf, "%s%d",
> +			(ls->line_offset.sign == LINE_OFFSET_NONE ? ""
> +			 : (ls->line_offset.sign
> +			    == LINE_OFFSET_PLUS ? "+" : "-")),
> +			ls->line_offset.offset);
>      }
> +
> +  tmp = ui_file_xstrdup (buf, NULL);
> +  make_cleanup (xfree, tmp);
> +  state->canonical->location = new_linespec_location (&tmp);
> +  do_cleanups (cleanup);
>  }
>  
>  /* Given a line offset in LS, construct the relevant SALs.  */
> @@ -1994,6 +1984,27 @@ create_sals_line_offset (struct linespec_state *self,
>    return values;
>  }
>  
> +/* Convert the given ADDRESS into SaLs.  */
> +
> +static struct symtabs_and_lines
> +convert_address_location_to_sals (struct linespec_state *self,
> +				  CORE_ADDR address)
> +{
> +  struct symtab_and_line sal;
> +  struct symtabs_and_lines sals = {NULL, 0};
> +
> +  sal = find_pc_line (address, 0);
> +  sal.pc = address;
> +  sal.section = find_pc_overlay (address);
> +  sal.explicit_pc = 1;
> +  add_sal_to_sals (self, &sals, &sal, core_addr_to_string (address), 1);
> +
> +  if (self->canonical != NULL)
> +    self->canonical->location = new_address_location (address);

====
Modifying self->canonical->location is an undocumented side-effect.
Can you augment the function comment to document and explain this?

> +
> +  return sals;
> +}
> +
>  /* Create and return SALs from the linespec LS.  */
>  
>  static struct symtabs_and_lines
> @@ -2001,18 +2012,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
>  {
>    struct symtabs_and_lines sals = {NULL, 0};
>  
> -  if (ls->expression != NULL)
> -    {
> -      struct symtab_and_line sal;
> -
> -      /* We have an expression.  No other attribute is allowed.  */
> -      sal = find_pc_line (ls->expr_pc, 0);
> -      sal.pc = ls->expr_pc;
> -      sal.section = find_pc_overlay (ls->expr_pc);
> -      sal.explicit_pc = 1;
> -      add_sal_to_sals (state, &sals, &sal, ls->expression, 1);
> -    }
> -  else if (ls->labels.label_symbols != NULL)
> +  if (ls->labels.label_symbols != NULL)
>      {
>        /* We have just a bunch of functions/methods or labels.  */
>        int i;
> @@ -2110,8 +2110,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
>  
>     The basic grammar of linespecs:
>  
> -   linespec -> expr_spec | var_spec | basic_spec
> -   expr_spec -> '*' STRING
> +   linespec -> var_spec | basic_spec
>     var_spec -> '$' (STRING | NUMBER)
>  
>     basic_spec -> file_offset_spec | function_spec | label_spec
> @@ -2207,33 +2206,7 @@ parse_linespec (linespec_parser *parser, const char *arg)
>    token = linespec_lexer_lex_one (parser);
>  
>    /* It must be either LSTOKEN_STRING or LSTOKEN_NUMBER.  */
> -  if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '*')
> -    {
> -      char *expr;
> -      const char *copy;
> -
> -      /* User specified an expression, *EXPR.  */
> -      copy = expr = copy_token_string (token);
> -      cleanup = make_cleanup (xfree, expr);
> -      PARSER_RESULT (parser)->expr_pc = linespec_expression_to_pc (&copy);
> -      discard_cleanups (cleanup);
> -      PARSER_RESULT (parser)->expression = expr;
> -
> -      /* This is a little hacky/tricky.  If linespec_expression_to_pc
> -	 did not evaluate the entire token, then we must find the
> -	 string COPY inside the original token buffer.  */
> -      if (*copy != '\0')
> -	{
> -	  PARSER_STREAM (parser) = strstr (parser->lexer.saved_arg, copy);
> -	  gdb_assert (PARSER_STREAM (parser) != NULL);
> -	}
> -
> -      /* Consume the token.  */
> -      linespec_lexer_consume_token (parser);
> -
> -      goto convert_to_sals;
> -    }
> -  else if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '$')
> +  if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '$')
>      {
>        char *var;
>  
> @@ -2457,20 +2430,6 @@ linespec_lex_to_end (char **stringp)
>  	  token = linespec_lexer_peek_token (&parser);
>  	  if (token.type == LSTOKEN_COMMA)
>  	    break;
> -
> -	  /* For addresses advance the parser stream past
> -	     any parsed input and stop lexing.  */
> -	  if (token.type == LSTOKEN_STRING
> -	      && *LS_TOKEN_STOKEN (token).ptr == '*')
> -	    {
> -	      const char *arg, *orig;
> -
> -	      orig = arg = *stringp;
> -	      (void) linespec_expression_to_pc (&arg);
> -	      PARSER_STREAM (&parser) += arg - orig;
> -	      break;
> -	    }
> -
>  	  token = linespec_lexer_consume_token (&parser);
>  
>  	  /* Keywords are okay after the first token.  */
> @@ -2508,6 +2467,12 @@ event_location_to_sals (linespec_parser *parser,
>        }
>        break;
>  
> +    case ADDRESS_LOCATION:
> +      result
> +	= convert_address_location_to_sals (PARSER_STATE (parser),
> +					    get_address_location (location));
> +      break;
> +
>      default:
>        gdb_assert_not_reached ("unhandled event location type");
>      }
> @@ -2693,7 +2658,7 @@ initialize_defaults (struct symtab **default_symtab, int *default_line)
>  /* Evaluate the expression pointed to by EXP_PTR into a CORE_ADDR,
>     advancing EXP_PTR past any parsed text.  */
>  
> -static CORE_ADDR
> +CORE_ADDR
>  linespec_expression_to_pc (const char **exp_ptr)
>  {
>    if (current_program_space->executing_startup)
> diff --git a/gdb/linespec.h b/gdb/linespec.h
> index 5f5bc71..8055f55 100644
> --- a/gdb/linespec.h
> +++ b/gdb/linespec.h
> @@ -156,4 +156,9 @@ extern struct symtabs_and_lines decode_line_with_last_displayed (char *, int);
>     STRINGP will be advanced to this point.  */
>  
>  extern void linespec_lex_to_end (char **stringp);
> +
> +/* Evaluate the expression pointed to by EXP_PTR into a CORE_ADDR,
> +   advancing EXP_PTR past any parsed text.  */
> +
> +extern CORE_ADDR linespec_expression_to_pc (const char **exp_ptr);
>  #endif /* defined (LINESPEC_H) */
> diff --git a/gdb/location.c b/gdb/location.c
> index 39e09c1..c1f4e19 100644
> --- a/gdb/location.c
> +++ b/gdb/location.c
> @@ -45,6 +45,10 @@ struct event_location
>         probes.  */
>      char *addr_string;
>  #define EL_LINESPEC(PTR) ((PTR)->u.addr_string)
> +
> +    /* An address in the inferior.  */
> +    CORE_ADDR address;
> +#define EL_ADDRESS(PTR) (PTR)->u.address
>    } u;
>  
>    /* Cached string representation of this location.  This is used, e.g., to
> @@ -95,6 +99,28 @@ get_linespec_location (const struct event_location *location)
>  /* See description in location.h.  */
>  
>  struct event_location *
> +new_address_location (CORE_ADDR addr)
> +{
> +  struct event_location *location;
> +
> +  location = XCNEW (struct event_location);
> +  EL_TYPE (location) = ADDRESS_LOCATION;
> +  EL_ADDRESS (location) = addr;
> +  return location;
> +}
> +
> +/* See description in location.h.  */
> +
> +CORE_ADDR
> +get_address_location (const struct event_location *location)
> +{
> +  gdb_assert (EL_TYPE (location) == ADDRESS_LOCATION);
> +  return EL_ADDRESS (location);
> +}
> +
> +/* See description in location.h.  */
> +
> +struct event_location *
>  copy_event_location (const struct event_location *src)
>  {
>    struct event_location *dst;
> @@ -111,6 +137,10 @@ copy_event_location (const struct event_location *src)
>  	EL_LINESPEC (dst) = xstrdup (EL_LINESPEC (src));
>        break;
>  
> +    case ADDRESS_LOCATION:
> +      EL_ADDRESS (dst) = EL_ADDRESS (src);
> +      break;
> +
>      default:
>        gdb_assert_not_reached ("unknown event location type");
>      }
> @@ -151,6 +181,10 @@ delete_event_location (struct event_location *location)
>  	  xfree (EL_LINESPEC (location));
>  	  break;
>  
> +	case ADDRESS_LOCATION:
> +	  /* Nothing to do.  */
> +	  break;
> +
>  	default:
>  	  gdb_assert_not_reached ("unknown event location type");
>  	}
> @@ -176,6 +210,12 @@ event_location_to_string_const (const struct event_location *location)
>  	result = xstrdup (EL_LINESPEC (location));
>        break;
>  
> +    case ADDRESS_LOCATION:
> +      result
> +	= xstrprintf ("*%s",
> +		      core_addr_to_string (EL_ADDRESS (location)));
> +      break;
> +
>      default:
>        gdb_assert_not_reached ("unknown event location type");
>      }
> @@ -203,7 +243,23 @@ string_to_event_location (char **stringp,
>  {
>    struct event_location *location;
>  
> -  location = new_linespec_location (stringp);
> +  /* First, check if the string is an address location.  */
> +  if (*stringp != NULL && **stringp == '*')
> +    {
> +      const char *arg, *orig;
> +      CORE_ADDR addr;
> +
> +      orig = arg = *stringp;
> +      addr = linespec_expression_to_pc (&arg);
> +      location = new_address_location (addr);
> +      *stringp += arg - orig;
> +    }
> +  else
> +    {
> +      /* Everything else is a linespec.  */
> +      location = new_linespec_location (stringp);
> +    }
> +
>    return location;
>  }
>  
> @@ -218,6 +274,9 @@ event_location_empty_p (const struct event_location *location)
>        /* Linespecs are never "empty."  (NULL is a valid linespec)  */
>        return 0;
>  
> +    case ADDRESS_LOCATION:
> +      return 0;
> +
>      default:
>        gdb_assert_not_reached ("unknown event location type");
>      }
> diff --git a/gdb/location.h b/gdb/location.h
> index 992f21e..39db10e 100644
> --- a/gdb/location.h
> +++ b/gdb/location.h
> @@ -28,7 +28,10 @@ struct event_location;
>  enum event_location_type
>  {
>    /* A traditional linespec.  */
> -  LINESPEC_LOCATION
> +  LINESPEC_LOCATION,
> +
> +  /* An address in the inferior.  */
> +  ADDRESS_LOCATION
>  };
>  
>  /* Return the type of the given event location.  */
> @@ -64,6 +67,18 @@ extern struct event_location *
>  extern const char *
>    get_linespec_location (const struct event_location *location);
>  
> +/* Create a new address location.  The return result is malloc'd
> +   and should be freed with delete_event_location.  */
> +
> +extern struct event_location *
> +  new_address_location (CORE_ADDR addr);
> +
> +/* Return the address location (a CORE_ADDR) of the given event_location
> +   (which must be of type ADDRESS_LOCATION).  */
> +
> +extern CORE_ADDR
> +  get_address_location (const struct event_location *location);
> +
>  /* Free an event location and any associated data.  */
>  
>  extern void delete_event_location (struct event_location *location);
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index bbb408a..d145dfa 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -166,9 +166,8 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>    struct frame_id frame_id;
>    PyObject *internal = NULL;
>    int internal_bp = 0;
> -  CORE_ADDR finish_pc, pc;
> +  CORE_ADDR pc;
>    volatile struct gdb_exception except;
> -  char small_buf[100], *p;
>    struct symbol *function;
>  
>    if (!PyArg_ParseTupleAndKeywords (args, kwargs, "|OO", keywords,
> @@ -291,10 +290,7 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>        struct cleanup *back_to;
>  
>        /* Set a breakpoint on the return address.  */
> -      finish_pc = get_frame_pc (prev_frame);
> -      xsnprintf (small_buf, sizeof (small_buf), "*%s", hex_string (finish_pc));
> -      p = small_buf;
> -      location = new_linespec_location (&p);
> +      location = new_address_location (get_frame_pc (prev_frame));
>        back_to = make_cleanup_delete_event_location (location);
>        create_breakpoint (python_gdbarch,
>                           location, NULL, thread, NULL,
> diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
> index 86af2ae..61dc98d 100644
> --- a/gdb/spu-tdep.c
> +++ b/gdb/spu-tdep.c
> @@ -1957,7 +1957,6 @@ spu_catch_start (struct objfile *objfile)
>    CORE_ADDR pc;
>    struct event_location *location;
>    struct cleanup *back_to;
> -  char *p;
>  
>    /* Do this only if requested by "set spu stop-on-load on".  */
>    if (!spu_stop_on_load_p)
> @@ -2001,8 +2000,7 @@ spu_catch_start (struct objfile *objfile)
>  
>    /* Use a numerical address for the set_breakpoint command to avoid having
>       the breakpoint re-set incorrectly.  */
> -  p = ASTRDUP (core_addr_to_string (pc));
> -  location = new_linespec_location (&p);
> +  location = new_address_location (pc);
>    back_to = make_cleanup_delete_event_location (location);
>    create_breakpoint (get_objfile_arch (objfile), location,
>  		     NULL /* cond_string */, -1 /* thread */,


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]