[05/12] gdb: add breakpoint::first_loc methods

  Add convenience first_loc methods to struct breakpoint (const and
non-const overloads).  A subsequent patch changes the list of locations
to be an intrusive_list and makes the actual list private, so these
spots would need to become something ugly like:

  -b->loc
  +*b->locations ().begin ()

That would make the code much heavier and not readable.  There is a
surprisingly big number of places that access the first location of
breakpoints.  Whether this is correct, or these spots fail to consider
the possibility of multi-location breakpoints, I don't know.  But
anyhow, I think that using this instead:

 b->first_loc ()

conveys the intention better than the other two forms.

Change-Id: Ibbefe3e4ca6cdfe570351fe7e2725f2ce11d1e95
---
 gdb/breakpoint.c        | 112 ++++++++++++++++++++--------------------
 gdb/breakpoint.h        |  14 +++++
 gdb/elfread.c           |   4 +-
 gdb/infrun.c            |  11 ++--
 gdb/tracectf.c          |   2 +-
 gdb/tracefile-tfile.c   |   2 +-
 gdb/tracefile.c         |   2 +-
 gdb/tracepoint.c        |   2 +-
 gdb/tui/tui-winsource.c |   2 +-
 9 files changed, 82 insertions(+), 69 deletions(-)
  

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> Add convenience first_loc methods to struct breakpoint (const and
> non-const overloads).  A subsequent patch changes the list of locations
> to be an intrusive_list and makes the actual list private, so these
> spots would need to become something ugly like:
>
>   -b->loc
>   +*b->locations ().begin ()

It took me longer than I'd like to admit to realise the leading -/+ here
were indicating that this was an extract from a diff.  Maybe there's
some way to make it clearer?  Or just drop the first line, I think the
second line is clear enough (given the description before).

>
> That would make the code much heavier and not readable.  There is a
> surprisingly big number of places that access the first location of
> breakpoints.  Whether this is correct, or these spots fail to consider
> the possibility of multi-location breakpoints, I don't know.  But
> anyhow, I think that using this instead:
>
>  b->first_loc ()
>
> conveys the intention better than the other two forms.
>
> Change-Id: Ibbefe3e4ca6cdfe570351fe7e2725f2ce11d1e95
> ---
>  gdb/breakpoint.c        | 112 ++++++++++++++++++++--------------------
>  gdb/breakpoint.h        |  14 +++++
>  gdb/elfread.c           |   4 +-
>  gdb/infrun.c            |  11 ++--
>  gdb/tracectf.c          |   2 +-
>  gdb/tracefile-tfile.c   |   2 +-
>  gdb/tracefile.c         |   2 +-
>  gdb/tracepoint.c        |   2 +-
>  gdb/tui/tui-winsource.c |   2 +-
>  9 files changed, 82 insertions(+), 69 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index fefe18644cf9..e9b6574bba77 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -4106,7 +4106,7 @@ breakpoint_init_inferior (enum inf_context context)
>  
>    for (breakpoint *b : all_breakpoints_safe ())
>      {
> -      if (b->has_locations () && b->loc->pspace != pspace)
> +      if (b->has_locations () && b->first_loc ().pspace != pspace)
>  	continue;
>  
>        switch (b->type)
> @@ -5633,7 +5633,7 @@ build_bpstat_chain (const address_space *aspace, CORE_ADDR bp_addr,
>  	     entire expression, not the individual locations.  For
>  	     read watchpoints, the watchpoints_triggered function has
>  	     checked all locations already.  */
> -	  if (b->type == bp_hardware_watchpoint && bl != b->loc)
> +	  if (b->type == bp_hardware_watchpoint && bl != &b->first_loc ())
>  	    break;
>  
>  	  if (!bl->enabled || bl->disabled_by_cond || bl->shlib_disabled)
> @@ -6160,8 +6160,7 @@ bp_location_condition_evaluator (const struct bp_location *bl)
>  /* Print the LOC location out of the list of B->LOC locations.  */
>  
>  static void
> -print_breakpoint_location (const breakpoint *b,
> -			   struct bp_location *loc)
> +print_breakpoint_location (const breakpoint *b, const bp_location *loc)
>  {
>    struct ui_out *uiout = current_uiout;
>  
> @@ -6361,11 +6360,11 @@ print_one_breakpoint_location (struct breakpoint *b,
>    if (loc == NULL 
>        && (b->has_locations ()
>  	  && (b->has_multiple_locations ()
> -	      || !b->loc->enabled || b->loc->disabled_by_cond)))
> +	      || !b->first_loc ().enabled || b->first_loc ().disabled_by_cond)))
>      header_of_multiple = true;
>  
> -  if (loc == NULL)
> -    loc = b->loc;
> +  if (loc == NULL && b->has_locations ())
> +    loc = &b->first_loc ();
>  
>    annotate_record ();
>  
> @@ -6465,7 +6464,7 @@ print_one_breakpoint_location (struct breakpoint *b,
>  	  if (!header_of_multiple)
>  	    print_breakpoint_location (b, loc);
>  	  if (b->has_locations ())
> -	    *last_loc = b->loc;
> +	    *last_loc = &b->first_loc ();
>  	}
>      }
>  
> @@ -6745,8 +6744,8 @@ print_one_breakpoint (breakpoint *b, const bp_location **last_loc, int allflag)
>  	  && (allflag
>  	      || (b->has_locations ()
>  		  && (b->has_multiple_locations ()
> -		      || !b->loc->enabled
> -		      || b->loc->disabled_by_cond))))
> +		      || !b->first_loc ().enabled
> +		      || b->first_loc ().disabled_by_cond))))
>  	{
>  	  gdb::optional<ui_out_emit_list> locations_list;
>  
> @@ -7743,7 +7742,7 @@ create_thread_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
>  
>    b->enable_state = bp_enabled;
>    /* locspec has to be used or breakpoint_re_set will delete me.  */
> -  b->locspec = new_address_location_spec (b->loc->address, NULL, 0);
> +  b->locspec = new_address_location_spec (b->first_loc ().address, NULL, 0);
>  
>    update_global_location_list_nothrow (UGLL_MAY_INSERT);
>  
> @@ -7771,7 +7770,7 @@ remove_jit_event_breakpoints (void)
>  {
>    for (breakpoint *b : all_breakpoints_safe ())
>      if (b->type == bp_jit_event
> -	&& b->loc->pspace == current_program_space)
> +	&& b->first_loc ().pspace == current_program_space)
>        delete_breakpoint (b);
>  }
>  
> @@ -7780,7 +7779,7 @@ remove_solib_event_breakpoints (void)
>  {
>    for (breakpoint *b : all_breakpoints_safe ())
>      if (b->type == bp_shlib_event
> -	&& b->loc->pspace == current_program_space)
> +	&& b->first_loc ().pspace == current_program_space)
>        delete_breakpoint (b);
>  }
>  
> @@ -7791,7 +7790,7 @@ remove_solib_event_breakpoints_at_next_stop (void)
>  {
>    for (breakpoint *b : all_breakpoints_safe ())
>      if (b->type == bp_shlib_event
> -	&& b->loc->pspace == current_program_space)
> +	&& b->first_loc ().pspace == current_program_space)
>        b->disposition = disp_del_at_next_stop;
>  }
>  
> @@ -7826,7 +7825,7 @@ create_and_insert_solib_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR add
>    /* Explicitly tell update_global_location_list to insert
>       locations.  */
>    b = create_solib_event_breakpoint_1 (gdbarch, address, UGLL_INSERT);
> -  if (!b->loc->inserted)
> +  if (!b->first_loc ().inserted)
>      {
>        delete_breakpoint (b);
>        return NULL;
> @@ -8178,17 +8177,18 @@ momentary_breakpoint_from_master (struct breakpoint *orig,
>    std::unique_ptr<breakpoint> copy
>      (new_momentary_breakpoint (orig->gdbarch, type, orig->pspace,
>  			       orig->frame_id, thread));
> +  bp_location &orig_loc = orig->first_loc ();

Maybe 'const bp_location &' ?

Otherwise, looks good.

Thanks,
Andrew

>    copy->loc = copy->allocate_location ();
>    set_breakpoint_location_function (copy->loc);
>  
> -  copy->loc->gdbarch = orig->loc->gdbarch;
> -  copy->loc->requested_address = orig->loc->requested_address;
> -  copy->loc->address = orig->loc->address;
> -  copy->loc->section = orig->loc->section;
> -  copy->loc->pspace = orig->loc->pspace;
> -  copy->loc->probe = orig->loc->probe;
> -  copy->loc->line_number = orig->loc->line_number;
> -  copy->loc->symtab = orig->loc->symtab;
> +  copy->loc->gdbarch = orig_loc.gdbarch;
> +  copy->loc->requested_address = orig_loc.requested_address;
> +  copy->loc->address = orig_loc.address;
> +  copy->loc->section = orig_loc.section;
> +  copy->loc->pspace = orig_loc.pspace;
> +  copy->loc->probe = orig_loc.probe;
> +  copy->loc->line_number = orig_loc.line_number;
> +  copy->loc->symtab = orig_loc.symtab;
>    copy->loc->enabled = loc_enabled;
>  
>    breakpoint *b = add_to_breakpoint_chain (std::move (copy));
> @@ -8575,7 +8575,7 @@ code_breakpoint::code_breakpoint (struct gdbarch *gdbarch_,
>    if (locspec_ != nullptr)
>      locspec = std::move (locspec_);
>    else
> -    locspec = new_address_location_spec (this->loc->address, NULL, 0);
> +    locspec = new_address_location_spec (this->first_loc ().address, NULL, 0);
>    filter = std::move (filter_);
>  }
>  
> @@ -9411,7 +9411,6 @@ ranged_breakpoint::print_it (const bpstat *bs) const
>  bool
>  ranged_breakpoint::print_one (const bp_location **last_loc) const
>  {
> -  struct bp_location *bl = loc;
>    struct value_print_options opts;
>    struct ui_out *uiout = current_uiout;
>  
> @@ -9425,8 +9424,8 @@ ranged_breakpoint::print_one (const bp_location **last_loc) const
>         by ranged_breakpoint::print_one_detail.  */
>      uiout->field_skip ("addr");
>    annotate_field (5);
> -  print_breakpoint_location (this, bl);
> -  *last_loc = bl;
> +  print_breakpoint_location (this, &this->first_loc ());
> +  *last_loc = &this->first_loc ();
>  
>    return true;
>  }
> @@ -9437,18 +9436,16 @@ void
>  ranged_breakpoint::print_one_detail (struct ui_out *uiout) const
>  {
>    CORE_ADDR address_start, address_end;
> -  struct bp_location *bl = loc;
> +  const bp_location &bl = this->first_loc ();
>    string_file stb;
>  
> -  gdb_assert (bl);
> -
> -  address_start = bl->address;
> -  address_end = address_start + bl->length - 1;
> +  address_start = bl.address;
> +  address_end = address_start + bl.length - 1;
>  
>    uiout->text ("\taddress range: ");
>    stb.printf ("[%s, %s]",
> -	      print_core_address (bl->gdbarch, address_start),
> -	      print_core_address (bl->gdbarch, address_end));
> +	      print_core_address (bl.gdbarch, address_start),
> +	      print_core_address (bl.gdbarch, address_end));
>    uiout->field_stream ("addr", stb);
>    uiout->text ("\n");
>  }
> @@ -9458,15 +9455,14 @@ ranged_breakpoint::print_one_detail (struct ui_out *uiout) const
>  void
>  ranged_breakpoint::print_mention () const
>  {
> -  struct bp_location *bl = loc;
> +  const bp_location &bl = this->first_loc ();
>    struct ui_out *uiout = current_uiout;
>  
> -  gdb_assert (bl);
>    gdb_assert (type == bp_hardware_breakpoint);
>  
>    uiout->message (_("Hardware assisted ranged breakpoint %d from %s to %s."),
> -		  number, paddress (bl->gdbarch, bl->address),
> -		  paddress (bl->gdbarch, bl->address + bl->length - 1));
> +		  number, paddress (bl.gdbarch, bl.address),
> +		  paddress (bl.gdbarch, bl.address + bl.length - 1));
>  }
>  
>  /* Implement the "print_recreate" method for ranged breakpoints.  */
> @@ -9992,7 +9988,7 @@ masked_watchpoint::print_one_detail (struct ui_out *uiout) const
>    gdb_assert (this->has_single_location ());
>  
>    uiout->text ("\tmask ");
> -  uiout->field_core_addr ("mask", loc->gdbarch, hw_wp_mask);
> +  uiout->field_core_addr ("mask", this->first_loc ().gdbarch, hw_wp_mask);
>    uiout->text ("\n");
>  }
>  
> @@ -10302,11 +10298,11 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
>  	  scope_breakpoint->frame_id = caller_frame_id;
>  
>  	  /* Set the address at which we will stop.  */
> -	  scope_breakpoint->loc->gdbarch = caller_arch;
> -	  scope_breakpoint->loc->requested_address = caller_pc;
> -	  scope_breakpoint->loc->address
> -	    = adjust_breakpoint_address (scope_breakpoint->loc->gdbarch,
> -					 scope_breakpoint->loc->requested_address,
> +	  bp_location &loc = scope_breakpoint->first_loc ();
> +	  loc.gdbarch = caller_arch;
> +	  loc.requested_address = caller_pc;
> +	  loc.address
> +	    = adjust_breakpoint_address (loc.gdbarch, loc.requested_address,
>  					 scope_breakpoint->type,
>  					 current_program_space);
>  	}
> @@ -11599,23 +11595,24 @@ code_breakpoint::say_where () const
>      }
>    else
>      {
> -      if (opts.addressprint || loc->symtab == NULL)
> +      const bp_location &bl = this->first_loc ();
> +      if (opts.addressprint || bl.symtab == nullptr)
>  	gdb_printf (" at %ps",
>  		    styled_string (address_style.style (),
> -				   paddress (loc->gdbarch,
> -					     loc->address)));
> -      if (loc->symtab != NULL)
> +				   paddress (bl.gdbarch,
> +					     bl.address)));
> +      if (bl.symtab != NULL)
>  	{
>  	  /* If there is a single location, we can print the location
>  	     more nicely.  */
>  	  if (!this->has_multiple_locations ())
>  	    {
>  	      const char *filename
> -		= symtab_to_filename_for_display (loc->symtab);
> +		= symtab_to_filename_for_display (bl.symtab);
>  	      gdb_printf (": file %ps, line %d.",
>  			  styled_string (file_name_style.style (),
>  					 filename),
> -			  loc->line_number);
> +			  bl.line_number);
>  	    }
>  	  else
>  	    /* This is not ideal, but each location may have a
> @@ -12629,14 +12626,14 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
>  	  uiout->field_signed ("line", sal2.line);
>  	  uiout->text ("\n");
>  
> -	  b->loc->line_number = sal2.line;
> -	  b->loc->symtab = sym != NULL ? sal2.symtab : NULL;
> +	  b->first_loc ().line_number = sal2.line;
> +	  b->first_loc ().symtab = sym != NULL ? sal2.symtab : NULL;
>  
>  	  std::unique_ptr<explicit_location_spec> els
>  	    (new explicit_location_spec ());
>  	  els->source_filename
>  	    = xstrdup (symtab_to_filename_for_display (sal2.symtab));
> -	  els->line_offset.offset = b->loc->line_number;
> +	  els->line_offset.offset = b->first_loc ().line_number;
>  	  els->line_offset.sign = LINE_OFFSET_NONE;
>  
>  	  b->locspec = std::move (els);
> @@ -12873,9 +12870,10 @@ code_breakpoint::location_spec_to_sals (location_spec *locspec,
>  	  && (condition_not_parsed
>  	      || (this->has_locations ()
>  		  && search_pspace != NULL
> -		  && loc->pspace != search_pspace)
> -	      || (this->has_locations () && loc->shlib_disabled)
> -	      || (this->has_locations () && loc->pspace->executing_startup)
> +		  && this->first_loc ().pspace != search_pspace)
> +	      || (this->has_locations () && this->first_loc ().shlib_disabled)
> +	      || (this->has_locations ()
> +		  && this->first_loc ().pspace->executing_startup)
>  	      || enable_state == bp_disabled))
>  	not_found_and_ok = true;
>  
> @@ -13047,7 +13045,7 @@ breakpoint_re_set_thread (struct breakpoint *b)
>  	 selected as current, and unless this was a vfork will have a
>  	 different program space from the original thread.  Reset that
>  	 as well.  */
> -      b->loc->pspace = current_program_space;
> +      b->first_loc ().pspace = current_program_space;
>      }
>  }
>  
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 2a3a5cf57b83..9f49ed62a9ad 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -645,6 +645,20 @@ struct breakpoint
>    bool has_multiple_locations () const
>    { return this->loc != nullptr && this->loc->next != nullptr; }
>  
> +  /* Return a reference to the first location of this breakpoint.  */
> +  bp_location &first_loc ()
> +  {
> +    gdb_assert (this->has_locations ());
> +    return *this->loc;
> +  }
> +
> +  /* Return a reference to the first location of this breakpoint.  */
> +  const bp_location &first_loc () const
> +  {
> +    gdb_assert (this->has_locations ());
> +    return *this->loc;
> +  }
> +
>    /* Reevaluate a breakpoint.  This is necessary after symbols change
>       (e.g., an executable or DSO was loaded, or the inferior just
>       started).  */
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index 7169ae8808a8..2d4f23416245 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -974,7 +974,7 @@ elf_gnu_ifunc_resolver_stop (code_breakpoint *b)
>        gdb_assert (frame_id_p (b_return->frame_id));
>  
>        if (b_return->thread == thread_id
> -	  && b_return->loc->requested_address == prev_pc
> +	  && b_return->first_loc ().requested_address == prev_pc
>  	  && b_return->frame_id == prev_frame_id)
>  	break;
>      }
> @@ -1043,7 +1043,7 @@ elf_gnu_ifunc_resolver_return_stop (code_breakpoint *b)
>  
>    func_func = value::allocate (func_func_type);
>    func_func->set_lval (lval_memory);
> -  func_func->set_address (b->loc->related_address);
> +  func_func->set_address (b->first_loc ().related_address);
>  
>    value = value::allocate (value_type);
>    gdbarch_return_value_as_value (gdbarch, func_func, value_type, regcache,
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index efe2c00c489a..a9e2a2423b17 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -950,14 +950,14 @@ follow_inferior_reset_breakpoints (void)
>    if (tp->control.step_resume_breakpoint)
>      {
>        breakpoint_re_set_thread (tp->control.step_resume_breakpoint);
> -      tp->control.step_resume_breakpoint->loc->enabled = 1;
> +      tp->control.step_resume_breakpoint->first_loc ().enabled = 1;
>      }
>  
>    /* Treat exception_resume breakpoints like step_resume breakpoints.  */
>    if (tp->control.exception_resume_breakpoint)
>      {
>        breakpoint_re_set_thread (tp->control.exception_resume_breakpoint);
> -      tp->control.exception_resume_breakpoint->loc->enabled = 1;
> +      tp->control.exception_resume_breakpoint->first_loc ().enabled = 1;
>      }
>  
>    /* Reinsert all breakpoints in the child.  The user may have set
> @@ -2551,7 +2551,8 @@ resume_1 (enum gdb_signal sig)
>  		 user breakpoints at PC to trigger (again) when this
>  		 hits.  */
>  	      insert_hp_step_resume_breakpoint_at_frame (get_current_frame ());
> -	      gdb_assert (tp->control.step_resume_breakpoint->loc->permanent);
> +	      gdb_assert (tp->control.step_resume_breakpoint->first_loc ()
> +			  .permanent);
>  
>  	      tp->step_after_step_resume_breakpoint = step;
>  	    }
> @@ -7008,9 +7009,9 @@ process_event_stop_test (struct execution_control_state *ecs)
>  	= ecs->event_thread->control.step_resume_breakpoint;
>  
>        if (sr_bp != nullptr
> -	  && sr_bp->loc->permanent
> +	  && sr_bp->first_loc ().permanent
>  	  && sr_bp->type == bp_hp_step_resume
> -	  && sr_bp->loc->address == ecs->event_thread->prev_pc)
> +	  && sr_bp->first_loc ().address == ecs->event_thread->prev_pc)
>  	{
>  	  infrun_debug_printf ("stepped permanent breakpoint, stopped in handler");
>  	  delete_step_resume_breakpoint (ecs->event_thread);
> diff --git a/gdb/tracectf.c b/gdb/tracectf.c
> index ab513b1fa158..16461711da18 100644
> --- a/gdb/tracectf.c
> +++ b/gdb/tracectf.c
> @@ -1536,7 +1536,7 @@ ctf_get_traceframe_address (void)
>  	= get_tracepoint_by_number_on_target (tpnum);
>  
>        if (tp != nullptr && tp->has_locations ())
> -	addr = tp->loc->address;
> +	addr = tp->first_loc ().address;
>      }
>  
>    /* Restore the position.  */
> diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
> index 9c1adee11bc3..9203bcc2e4bb 100644
> --- a/gdb/tracefile-tfile.c
> +++ b/gdb/tracefile-tfile.c
> @@ -668,7 +668,7 @@ tfile_get_traceframe_address (off_t tframe_offset)
>    tp = get_tracepoint_by_number_on_target (tpnum);
>    /* FIXME this is a poor heuristic if multiple locations.  */
>    if (tp != nullptr && tp->has_locations ())
> -    addr = tp->loc->address;
> +    addr = tp->first_loc ().address;
>  
>    /* Restore our seek position.  */
>    cur_offset = saved_offset;
> diff --git a/gdb/tracefile.c b/gdb/tracefile.c
> index 883ce4bf375a..5a2641919f74 100644
> --- a/gdb/tracefile.c
> +++ b/gdb/tracefile.c
> @@ -412,7 +412,7 @@ tracefile_fetch_registers (struct regcache *regcache, int regno)
>  
>    /* Guess what we can from the tracepoint location.  */
>    gdbarch_guess_tracepoint_registers (gdbarch, regcache,
> -				      tp->loc->address);
> +				      tp->first_loc ().address);
>  }
>  
>  /* This is the implementation of target_ops method to_has_all_memory.  */
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 4a9d8491a54b..99d175d32e42 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -2750,7 +2750,7 @@ get_traceframe_location (int *stepping_frame_p)
>    /* If this is a stepping frame, we don't know which location
>       triggered.  The first is as good (or bad) a guess as any...  */
>    *stepping_frame_p = 1;
> -  return t->loc;
> +  return &t->first_loc ();
>  }
>  
>  /* Return the default collect actions of a tracepoint T.  */
> diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
> index 3c4ce501e5ee..3fc38b7be233 100644
> --- a/gdb/tui/tui-winsource.c
> +++ b/gdb/tui/tui-winsource.c
> @@ -640,7 +640,7 @@ tui_source_window_base::update_breakpoint_info
>  		    mode |= TUI_BP_ENABLED;
>  		  if (bp->hit_count)
>  		    mode |= TUI_BP_HIT;
> -		  if (bp->loc->cond)
> +		  if (bp->first_loc ().cond)
>  		    mode |= TUI_BP_CONDITIONAL;
>  		  if (bp->type == bp_hardware_breakpoint)
>  		    mode |= TUI_BP_HARDWARE;
> -- 
> 2.40.1
  

On 5/18/23 08:50, Andrew Burgess via Gdb-patches wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> Add convenience first_loc methods to struct breakpoint (const and
>> non-const overloads).  A subsequent patch changes the list of locations
>> to be an intrusive_list and makes the actual list private, so these
>> spots would need to become something ugly like:
>>
>>   -b->loc
>>   +*b->locations ().begin ()
> 
> It took me longer than I'd like to admit to realise the leading -/+ here
> were indicating that this was an extract from a diff.  Maybe there's
> some way to make it clearer?  Or just drop the first line, I think the
> second line is clear enough (given the description before).

Sorry, I thought I was being smart writing it this way.  I'll change it
to say, these spots would need from:

  b->loc

to something ugly like:

  b->locations ().begin ()

>> @@ -8178,17 +8177,18 @@ momentary_breakpoint_from_master (struct breakpoint *orig,
>>    std::unique_ptr<breakpoint> copy
>>      (new_momentary_breakpoint (orig->gdbarch, type, orig->pspace,
>>  			       orig->frame_id, thread));
>> +  bp_location &orig_loc = orig->first_loc ();
> 
> Maybe 'const bp_location &' ?

Yes, changed.

> Otherwise, looks good.

Thanks,

Simon
  

Simon Marchi <simark@simark.ca> writes:

> On 5/18/23 08:50, Andrew Burgess via Gdb-patches wrote:
>> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>>> Add convenience first_loc methods to struct breakpoint (const and
>>> non-const overloads).  A subsequent patch changes the list of locations
>>> to be an intrusive_list and makes the actual list private, so these
>>> spots would need to become something ugly like:
>>>
>>>   -b->loc
>>>   +*b->locations ().begin ()
>> 
>> It took me longer than I'd like to admit to realise the leading -/+ here
>> were indicating that this was an extract from a diff.  Maybe there's
>> some way to make it clearer?  Or just drop the first line, I think the
>> second line is clear enough (given the description before).
>
> Sorry, I thought I was being smart writing it this way.  I'll change it
> to say, these spots would need from:
>
>   b->loc
>
> to something ugly like:
>
>   b->locations ().begin ()

Looks great.

Thanks,
Andrew

>
>>> @@ -8178,17 +8177,18 @@ momentary_breakpoint_from_master (struct breakpoint *orig,
>>>    std::unique_ptr<breakpoint> copy
>>>      (new_momentary_breakpoint (orig->gdbarch, type, orig->pspace,
>>>  			       orig->frame_id, thread));
>>> +  bp_location &orig_loc = orig->first_loc ();
>> 
>> Maybe 'const bp_location &' ?
>
> Yes, changed.
>
>> Otherwise, looks good.
>
> Thanks,
>
> Simon