Re: [PATCH v3 6/8] Introduce find_pc_partial_entry_range and use it in infrun.c

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 2018-08-20 06:45 PM, Kevin Buettner wrote:
> An earlier version of this patch used the returned block in conjunction
> with BLOCK_ENTRY_PC to set stop_func_start in fill_in_stop_func() in
> infrun.c.  While I think this was the correct thing to do, changes
> to find_inferior_partial_function could potentially end up with
> stop_func_end < stop_func_start, which is definitely wrong.  For
> this case, we want to set both stop_func_start and stop_func_end
> to the start and end of the range containing the function's entry
> pc.
> 
> I think that this functionality will be useful in many other places
> too - it probably ought to be used in all of the various prologue
> analyzers in GDB.
> 
> The change to infrun.c was simple: the call to find_pc_partial_function
> was replaced with a call to find_pc_partial_entry_range.  The difference
> between these two functions is that find_pc_partial_entry_function
> will (potentially) return the start and end address corresponding to
> the range in which PC is found, but find_pc_partial_entry_range
> will (again, potentially) return the start and end address of the
> range containing the entry pc.  find_pc_partial_function has the
> property that *ADDRESS <= PC < *ENDADDR.  This condition does not
> necessarily hold for the outputs of find_pc_partial_entry_range.
> 
> It should be noted that for functions which contain only a single
> range, the outputs of find_pc_partial_{function,entry_range} are
> identical.
> 
> I think it might happen that find_pc_partial_entry_range will come
> to be used in place of many of the calls to find_pc_partial_function
> within GDB.  Care must be taken in making this change, however, since
> some of this code depends on the *ADDRESS <= PC < *ENDADDR property.

This LGTM.

I just find the naming of all these finc_pc_* funtions confusing, like
what does the "partial" mean in "find_pc_partial_function"?  Why does
the new function name not include "function"?  I don't really have a
better alternative to suggest, but if there's a logic behind the new name
I'd like to know it so I understand the naming scheme better.

> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index b25d745..fb72880 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -4285,11 +4285,12 @@ fill_in_stop_func (struct gdbarch *gdbarch,
>  {
>    if (!ecs->stop_func_filled_in)
>      {
> +

This looks like an unwanted change.

Simon