Re: [PATCH 3/8] Add support for non-contiguous blocks to find_pc_partial_function

Simon Marchi
2018-08-01T02:01:16Z

On 2018-07-19 02:52 PM, Kevin Buettner wrote:
> The description and patch below are intended as a replacement for my
> original patch. It uses the approach outlined by Simon Marchi for
> checking the find_pc_partial_function cache.
>
> - - - -
>
> This change adds an optional output parameter BLOCK to
> find_pc_partial_function. If BLOCK is non-null, then *BLOCK will be
> set to the address of the block corresponding to the function symbol
> if such a symbol was found during lookup. Otherwise it's set to a
> NULL value. Callers may wish to use the block information to
> determine whether the block contains any non-contiguous ranges. The
> caller may also iterate over or examine those ranges.
>
> When I first started looking at the broken stepping behavior associated
> with functions w/ non-contiguous ranges, I found that I could "fix"
> the problem by disabling the find_pc_partial_function cache. It would
> sometimes happen that the PC passed in would be between the low and
> high cache values, but would be in some other function that happens to
> be placed in between the ranges for the cached function. This caused
> incorrect values to be returned.
>
> So dealing with this cache turns out to be very important for fixing
> this problem. I explored three different ways of dealing with the cache.
>
> My first approach was to clear the cache when a block was encountered
> with more than one range. This would cause the non-cache pathway to
> be executed on the next call to find_pc_partial_function.
>
> Another approach, which I suspect is slightly faster, checks to see
> whether the PC is within one of the ranges associated with the cached
> block. If so, then the cached values can be used. It falls back to
> the original behavior if there is no cached block.
>
> The current approach, suggested by Simon Marchi, is to restrict the
> low/high pc values recorded for the cache to the beginning and end of
> the range containing the PC value under consideration. This allows us
> to retain the simple (and fast) test for determining whether the
> memoized (cached) values apply to the PC passed to
> find_pc_partial_function.
>
> Another choice that had to be made was whether to have ADDRESS
> continue to represent the lowest address associated with the function
> or with the entry pc associated with the function. For the moment,
> I've decided to keep the current behavior of having it represent the
> lowest address. In cases where the entry pc is needed, this can be
> found by passing a non-NULL value for BLOCK which will cause the block
> (pointer) associated with the function to be returned. BLOCK_ENTRY_PC
> can then be used on that block to obtain the entry pc.

This LGTM overall, just a few nits/suggestions/questions.

> diff --git a/gdb/blockframe.c b/gdb/blockframe.c
> index b3c9aa3..a3b2a11 100644
> --- a/gdb/blockframe.c
> +++ b/gdb/blockframe.c
> @@ -159,6 +159,7 @@ static CORE_ADDR cache_pc_function_low = 0;
> static CORE_ADDR cache_pc_function_high = 0;
> static const char *cache_pc_function_name = 0;
> static struct obj_section *cache_pc_function_section = NULL;
> +static const struct block *cache_pc_function_block = nullptr;
>
> /* Clear cache, e.g. when symbol table is discarded. */
>
> @@ -169,24 +170,14 @@ clear_pc_function_cache (void)
> cache_pc_function_high = 0;
> cache_pc_function_name = (char *) 0;
> cache_pc_function_section = NULL;
> + cache_pc_function_block = nullptr;

We might want to rename cache_pc_function_low and cache_pc_function_high to
reflect their new usage (since they don't represent the low/high bounds of
the function anymore, but the range).

Alternatively, it might make things simpler to keep cache_pc_function_low
and cache_pc_function_high as they are right now, and introduce two
new variables for the bounds of the matched range.

> }
>
> -/* Finds the "function" (text symbol) that is smaller than PC but
> - greatest of all of the potential text symbols in SECTION. Sets
> - *NAME and/or *ADDRESS conditionally if that pointer is non-null.
> - If ENDADDR is non-null, then set *ENDADDR to be the end of the
> - function (exclusive), but passing ENDADDR as non-null means that
> - the function might cause symbols to be read. This function either
> - succeeds or fails (not halfway succeeds). If it succeeds, it sets
> - *NAME, *ADDRESS, and *ENDADDR to real information and returns 1.
> - If it fails, it sets *NAME, *ADDRESS and *ENDADDR to zero and
> - returns 0. */
> -
> -/* Backward compatibility, no section argument. */
> +/* See symtab.h. */
>
> int
> find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> - CORE_ADDR *endaddr)
> + CORE_ADDR *endaddr, const struct block **block)
> {
> struct obj_section *section;
> struct symbol *f;
> @@ -232,13 +223,37 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> f = find_pc_sect_function (mapped_pc, section);
> if (f != NULL
> && (msymbol.minsym == NULL
> - || (BLOCK_START (SYMBOL_BLOCK_VALUE (f))
> + || (BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (f))

I don't understand this change, can you explain it briefly?

> >= BMSYMBOL_VALUE_ADDRESS (msymbol))))
> {
> - cache_pc_function_low = BLOCK_START (SYMBOL_BLOCK_VALUE (f));
> - cache_pc_function_high = BLOCK_END (SYMBOL_BLOCK_VALUE (f));
> + const struct block *b = SYMBOL_BLOCK_VALUE (f);
> +
> + if (BLOCK_CONTIGUOUS_P (b))
> + {
> + cache_pc_function_low = BLOCK_START (b);
> + cache_pc_function_high = BLOCK_END (b);
> + }
> + else
> + {
> + int i;
> + for (i = 0; i < BLOCK_NRANGES (b); i++)
> + {
> + if (BLOCK_RANGE_START (b, i) <= mapped_pc
> + && mapped_pc < BLOCK_RANGE_END (b, i))
> + {
> + cache_pc_function_low = BLOCK_RANGE_START (b, i);
> + cache_pc_function_high = BLOCK_RANGE_END (b, i);
> + break;
> + }
> + }
> + /* Above loop should exit via the break. */
> + gdb_assert (i < BLOCK_NRANGES (b));
> + }
> +
> cache_pc_function_name = SYMBOL_LINKAGE_NAME (f);
> cache_pc_function_section = section;
> + cache_pc_function_block = b;
> +
> goto return_cached_value;
> }
> }
> @@ -268,15 +283,33 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> cache_pc_function_name = MSYMBOL_LINKAGE_NAME (msymbol.minsym);
> cache_pc_function_section = section;
> cache_pc_function_high = minimal_symbol_upper_bound (msymbol);
> + cache_pc_function_block = nullptr;
>
> return_cached_value:
>
> + CORE_ADDR f_low, f_high;
> +
> + /* The low and high addresses for the cache do not necessarily
> + correspond to the low and high addresses for the function.
> + Extract the function low/high addresses from the cached block
> + if there is one; otherwise use the cached low & high values. */
> + if (cache_pc_function_block)

if (cache_pc_function_block != nullptr)

> + {
> + f_low = BLOCK_START (cache_pc_function_block);
> + f_high = BLOCK_END (cache_pc_function_block);
> + }
> + else
> + {
> + f_low = cache_pc_function_low;
> + f_high = cache_pc_function_high;
> + }

This, for example, could probably be kept simple if new variables were
introduced for the matched block range, and cache_pc_function_{low,high}
stayed as-is. I haven't actually tried, so it may not be a good idea at
all.

> +
> if (address)
> {
> if (pc_in_unmapped_range (pc, section))
> - *address = overlay_unmapped_address (cache_pc_function_low, section);
> + *address = overlay_unmapped_address (f_low, section);
> else
> - *address = cache_pc_function_low;
> + *address = f_low;
> }
>
> if (name)
> @@ -291,13 +324,15 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> the overlay), we must actually convert (high - 1) and
> then add one to that. */
>
> - *endaddr = 1 + overlay_unmapped_address (cache_pc_function_high - 1,
> - section);
> + *endaddr = 1 + overlay_unmapped_address (f_high - 1, section);
> }
> else
> - *endaddr = cache_pc_function_high;
> + *endaddr = f_high;
> }
>
> + if (block)

if (block != nullptr)

Thanks,

Simon