Re: [PATCH 09/23] init_breakpoint_sal -> base_breakpoint::base_breakpoint

[Pedro Alves <pedro@palves.net>]

On 2022-05-20 14:40, Lancelot SIX wrote:
> Hi,
> 
> Some styling remarks below.
> 
> On Mon, May 16, 2022 at 07:40:16PM +0100, Pedro Alves wrote:
>> This converts init_breakpoint_sal to a base_breakpoint constructor.
>>
>> It removes a use of init_raw_breakpoint.
>>
>> To avoid manually adding a bunch of parameters to
>> new_breakpoint_from_type, and manually passing them down to the
>> constructors of a number of different base_breakpoint subclasses, make
>> new_breakpoint_from_type a variable template function.
>>
>> Change-Id: I4cc24133ac4c292f547289ec782fc78e5bbe2510
>> ---
>>  gdb/breakpoint.c | 239 +++++++++++++++++++++++------------------------
>>  gdb/breakpoint.h |  16 ++++
>>  2 files changed, 133 insertions(+), 122 deletions(-)
>> @@ -8326,81 +8331,68 @@ init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch,
>>  
>>    gdb_assert (!sals.empty ());
>>  
>> -  for (const auto &sal : sals)
>> -    {
>> -      struct bp_location *loc;
>> +  thread = thread_;
>> +  task = task_;
>>  
>> -      if (from_tty)
>> -	{
>> -	  struct gdbarch *loc_gdbarch = get_sal_arch (sal);
>> -	  if (!loc_gdbarch)
>> -	    loc_gdbarch = gdbarch;
>> +  cond_string = std::move (cond_string_);
>> +  extra_string = std::move (extra_string_);
>> +  ignore_count = ignore_count_;
>> +  enable_state = enabled_ ? bp_enabled : bp_disabled;
>> +  disposition = disposition_;
>>  
>> -	  describe_other_breakpoints (loc_gdbarch,
>> -				      sal.pspace, sal.pc, sal.section, thread);
>> -	}
>> +  if (type == bp_static_tracepoint
>> +      || type == bp_static_marker_tracepoint)
>> +    {
>> +      auto *t = static_cast <struct tracepoint *> (this);
> 
> I think the space before "<" should be removed (I think I saw this in
> one of the previous patches too).

Thanks, I fixed it in the patch that introduced it (patch #6).

I then did:

 $ git diff upstream/master... | grep static_cast
 +      auto *t = static_cast<struct tracepoint *> (this);
 +    = static_cast<momentary_breakpoint *> (tp->control.single_step_breakpoints);

with the whole series applied, so it looks like this was the only case.

> 
>> +      struct static_tracepoint_marker marker;
>>  
>> -      if (&sal == &sals[0])
>> +      if (strace_marker_p (this))
>>  	{
>> -	  init_raw_breakpoint (b, sal, type);
>> -	  b->thread = thread;
>> -	  b->task = task;
>> +	  /* We already know the marker exists, otherwise, we
>> +	     wouldn't see a sal for it.  */
>> +	  const char *p
>> +	    = &event_location_to_string (location_.get ())[3];
> 
> Looks like that the statement can fit on one line now.

Done.

> 
>> +	  const char *endp;
>>  
>> -	  b->cond_string = std::move (cond_string);
>> -	  b->extra_string = std::move (extra_string);
>> -	  b->ignore_count = ignore_count;
>> -	  b->enable_state = enabled ? bp_enabled : bp_disabled;
>> -	  b->disposition = disposition;
>> +	  p = skip_spaces (p);
>>  
>> -	  if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
>> -	    b->loc->inserted = 1;
>> +	  endp = skip_to_space (p);
>>  
>> -	  if (type == bp_static_tracepoint
>> -	      || type == bp_static_marker_tracepoint)
>> -	    {
>> -	      auto *t = static_cast <struct tracepoint *> (b);
>> -	      struct static_tracepoint_marker marker;
>> -
>> -	      if (strace_marker_p (b))
>> -		{
>> -		  /* We already know the marker exists, otherwise, we
>> -		     wouldn't see a sal for it.  */
>> -		  const char *p
>> -		    = &event_location_to_string (b->location.get ())[3];
>> -		  const char *endp;
>> -
>> -		  p = skip_spaces (p);
>> -
>> -		  endp = skip_to_space (p);
>> -
>> -		  t->static_trace_marker_id.assign (p, endp - p);
>> -
>> -		  gdb_printf (_("Probed static tracepoint "
>> -				"marker \"%s\"\n"),
>> -			      t->static_trace_marker_id.c_str ());
>> -		}
>> -	      else if (target_static_tracepoint_marker_at (sal.pc, &marker))
>> -		{
>> -		  t->static_trace_marker_id = std::move (marker.str_id);
>> +	  t->static_trace_marker_id.assign (p, endp - p);
>>  
>> -		  gdb_printf (_("Probed static tracepoint "
>> -				"marker \"%s\"\n"),
>> -			      t->static_trace_marker_id.c_str ());
>> -		}
>> -	      else
>> -		warning (_("Couldn't determine the static "
>> -			   "tracepoint marker to probe"));
>> -	    }
>> +	  gdb_printf (_("Probed static tracepoint "
>> +			"marker \"%s\"\n"),
> 
> This literal string can also fit into one line, no need to split it.

Done.

> 
>> +		      t->static_trace_marker_id.c_str ());
>> +	}
>> +      else if (target_static_tracepoint_marker_at (sals[0].pc, &marker))
>> +	{
>> +	  t->static_trace_marker_id = std::move (marker.str_id);
>>  
>> -	  loc = b->loc;
>> +	  gdb_printf (_("Probed static tracepoint "
>> +			"marker \"%s\"\n"),
> 
> Same here.

Done.

> 
>> +		      t->static_trace_marker_id.c_str ());
>>  	}
>>        else
>> +	warning (_("Couldn't determine the static "
>> +		   "tracepoint marker to probe"));
> 
> And probably here too.

Done.

> 
>> +    }
>> +
>> +  for (const auto &sal : sals)
>> +    {
>> +      if (from_tty)
>>  	{
>> -	  loc = b->add_location (sal);
>> -	  if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
>> -	    loc->inserted = 1;
>> +	  struct gdbarch *loc_gdbarch = get_sal_arch (sal);
>> +	  if (!loc_gdbarch)
> 
> Should be "loc_gdbarch == nullptr"
> 
>> +	    loc_gdbarch = gdbarch;

Done.

>> +
>> +	  describe_other_breakpoints (loc_gdbarch,
>> +				      sal.pspace, sal.pc, sal.section, thread);
>>  	}
>>  
>> +      bp_location *new_loc = add_location (sal);
>> +      if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
>> +	new_loc->inserted = 1;
>> +
>>        /* Do not set breakpoint locations conditions yet.  As locations
>>  	 are inserted, they get sorted based on their addresses.  Let
>>  	 the list stabilize to have reliable location numbers.  */
> 
> Also, down the line, there are some leftover NULL which could become
> nullptr.  This is from code which just god re-indented, but while at
> updating it, it could be nice to fix this too.

All done locally.

Thanks!