[v3,6/8] Introduce find_pc_partial_entry_range and use it in infrun.c
Commit Message
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.
gdb/ChangeLog:
* infrun.c (fill_in_stop_func): Use find_pc_partial_entry_range
in place of find_pc_partial_function.
* blockframe.c (find_pc_partial_entry_range): New function.
* symtab.h (find_pc_partial_entry_range): Declare and document.
---
gdb/blockframe.c | 37 +++++++++++++++++++++++++++++++++++++
gdb/infrun.c | 7 ++++---
gdb/symtab.h | 20 +++++++++++++++++++-
3 files changed, 60 insertions(+), 4 deletions(-)
Comments
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
On Tue, 21 Aug 2018 12:19:13 -0400
Simon Marchi <simon.marchi@ericsson.com> wrote:
> 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.
I (also) don't understand the original intent/meaning behind the use of
"partial" in the name.
As noted elsewhere, I've made a change to what the return values
*ADDRESS and *ENDADDR refer to: the start and end address now refer
only to the part (range) of the function in which PC is found. So
that's what "partial" might now refer to, though I'll admit that I
hadn't put much thought into it until just now.
With regard to "find_pc_partial_entry_range", it seems to me that
"find_pc_partial_function_entry_range" is overly long. Maybe
"find_pc_function_entry_range" is better?
Kevin
On 2018-08-21 13:49, Kevin Buettner wrote:
> With regard to "find_pc_partial_entry_range", it seems to me that
> "find_pc_partial_function_entry_range" is overly long. Maybe
> "find_pc_function_entry_range" is better?
Either way is fine with me. In the end I'd like to rename everything in
the form "find_X_from_Y", which in this case could be
"find_function_entry_range_from_pc". If you like it we can use that,
although it would make it inconsistent with what we have at the moment.
Simon
On Tue, 21 Aug 2018 14:23:36 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:
> On 2018-08-21 13:49, Kevin Buettner wrote:
> > With regard to "find_pc_partial_entry_range", it seems to me that
> > "find_pc_partial_function_entry_range" is overly long. Maybe
> > "find_pc_function_entry_range" is better?
>
> Either way is fine with me. In the end I'd like to rename everything in
> the form "find_X_from_Y", which in this case could be
> "find_function_entry_range_from_pc". If you like it we can use that,
> although it would make it inconsistent with what we have at the moment.
I like find_function_entry_range_from_pc. I'll use that. Hopefully, at some
point, it will no longer be inconsistent...
Thanks,
Kevin
@@ -358,6 +358,43 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
/* See symtab.h. */
+bool
+find_pc_partial_entry_range (CORE_ADDR pc, const char **name,
+ CORE_ADDR *address, CORE_ADDR *endaddr)
+{
+ const struct block *block;
+ bool status = find_pc_partial_function (pc, name, address, endaddr, &block);
+
+ if (status && block != nullptr && !BLOCK_CONTIGUOUS_P (block))
+ {
+ CORE_ADDR entry_pc = BLOCK_ENTRY_PC (block);
+
+ for (int i = 0; i < BLOCK_NRANGES (block); i++)
+ {
+ if (BLOCK_RANGE_START (block, i) <= entry_pc
+ && entry_pc < BLOCK_RANGE_END (block, i))
+ {
+ if (address != nullptr)
+ *address = BLOCK_RANGE_START (block, i);
+
+ if (endaddr != nullptr)
+ *endaddr = BLOCK_RANGE_END (block, i);
+
+ return status;
+ }
+ }
+
+ /* It's an internal error if we exit the above loop without finding
+ the range. */
+ internal_error (__FILE__, __LINE__,
+ _("Entry block not found in find_pc_partial_entry_range"));
+ }
+
+ return status;
+}
+
+/* See symtab.h. */
+
struct type *
find_function_type (CORE_ADDR pc)
{
@@ -4285,11 +4285,12 @@ fill_in_stop_func (struct gdbarch *gdbarch,
{
if (!ecs->stop_func_filled_in)
{
+
/* Don't care about return value; stop_func_start and stop_func_name
will both be 0 if it doesn't work. */
- find_pc_partial_function (ecs->event_thread->suspend.stop_pc,
- &ecs->stop_func_name,
- &ecs->stop_func_start, &ecs->stop_func_end);
+ find_pc_partial_entry_range (ecs->event_thread->suspend.stop_pc,
+ &ecs->stop_func_name,
+ &ecs->stop_func_start, &ecs->stop_func_end);
ecs->stop_func_start
+= gdbarch_deprecated_function_start_offset (gdbarch);
@@ -1725,12 +1725,30 @@ extern struct symbol *find_symbol_at_address (CORE_ADDR);
This might suggest that *ADDRESS and *ENDADDR ought to be set to the
limits of the entry pc range, but that will cause the
*ADDRESS <= PC < *ENDADDR condition to be violated; many of the
- callers of find_pc_partial_function expect this condition to hold. */
+ callers of find_pc_partial_function expect this condition to hold.
+
+ Callers which require the start and/or end addresses for the range
+ containing the entry pc should instead call
+ find_pc_partial_entry_range. */
extern int find_pc_partial_function (CORE_ADDR pc, const char **name,
CORE_ADDR *address, CORE_ADDR *endaddr,
const struct block **block = nullptr);
+/* Like find_pc_partial_function, above, but *ADDRESS and *ENDADDR are
+ set to start and end addresses of the range containing the entry pc.
+
+ Note that it is not necessarily the case that (for non-NULL ADDRESS
+ and ENDADDR arguments) the *ADDRESS <= PC < *ENDADDR condition will
+ hold.
+
+ See comment for find_pc_partial_function, above, for further
+ explanation. */
+
+extern bool find_pc_partial_entry_range (CORE_ADDR pc, const char **name,
+ CORE_ADDR *address,
+ CORE_ADDR *endaddr);
+
/* Return the type of a function with its first instruction exactly at
the PC address. Return NULL otherwise. */