| Message ID | 20180813170446.0905cd4c@pinnacle.lan |
|---|---|
| State | New |
| Headers | show |
On 2018-08-13 08:04 PM, Kevin Buettner wrote: > 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 regards setting *ADDRESS and > *ENDADDR. There are three possibilities which might make sense: > > 1) *ADDRESS and *ENDADDR represent the lowest and highest address > of the function. > 2) *ADDRESS and *ENDADDR are set to the start and end address of > the range containing the entry pc. > 3) *ADDRESS and *ENDADDR are set to the start and end address of > the range in which PC is found. > > An earlier version of this patch implemented option #1. I found out > that it's not very useful though and, in fact, returns results that > are incorrect when used in the context of determining the start and > end of the function for doing prologue analysis. While debugging a > function in which the entry pc was in the second range (of a function > containing two non-contiguous ranges), I noticed that > amd64_skip_prologue called find_pc_partial_function - the returned > start address was set to the beginning of the first range. This is > incorrect for this function. What was also interesting was that this > first invocation of find_pc_partial_function correctly set the cache > for the PC on which it had been invoked, but a slightly later call > from skip_prologue_using_sal could not use this cached value because > it was now being used to lookup the very lowest address of the > function - which is in a range not containing the entry pc. > > Option #2 is attractive as it would provide a desirable result > when used in the context of prologue analysis. However, many callers, > including some which do prologue analysis want the condition > *ADDRESS <= PC < *ENDADDR to hold. This will not be the case when > find_pc_partial_function is called on a PC that's in a non-entry-pc > range. A later patch to this series adds find_pc_partial_entry_range > as a wrapper of find_pc_partial_function. > > Option #3 causes the *ADDRESS <= PC < *ENDADDR property to hold. If > find_pc_partial_function is called with a PC that's within entry pc's > range, then it will correctly return the limits of that range. So, if > the result of a minsym search is passed to find_pc_partial_function > to find the limits, then correct results will be achieved. Returned > limits (for prologue analysis) won't be correct when PC is within some > other (non-entry-pc) range. I don't yet know how big of a problem > this might be; I'm guessing that it won't be a serious problem - if a > compiler generates functions which have non-contiguous ranges, then it > also probably generates DWARF2 CFI which makes a lot of the old > prologue analysis moot. > > I've implemented option #3 for this version of the patch. I don't see > any regressions for x86-64. Moreover, I don't expect to see > regressions for other targets either simply because > find_pc_partial_function behaves the same as it did before for the > contiguous address range case. That said, there may be some > adjustments needed if GDB encounters a function with requires prologue > analysis which also occupies non-contiguous ranges. LGTM, just some nits. As I mentioned previously, cache_pc_function_{low,high} could be renamed to reflect that they now represent the low/high addresses of the range. If you think it's not necessary, it's also fine, but I just want to make sure the comment didn't just fall through the cracks. > @@ -228,17 +219,64 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address, > if (compunit_symtab != NULL) > { > /* Checking whether the msymbol has a larger value is for the > - "pathological" case mentioned in print_frame_info. */ > + "pathological" case mentioned in stack.c:find_frame_funname. > + > + We use BLOCK_ENTRY_PC instead of BLOCK_START_PC for this > + comparison because the minimal symbol should refer to the > + function's entry pc which is not necessarily the lowest > + address of the function. This will happen when the function > + has more than one range and the entry pc is not within the > + lowest range of addresses. */ > 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)) > >= 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); > + > + cache_pc_function_low = BLOCK_START (b); > + cache_pc_function_high = BLOCK_END (b); I think these last two lines are unnecessary, because we are guaranteed to set them lower in any case. > @@ -292,12 +331,15 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address, > then add one to that. */ > > *endaddr = 1 + overlay_unmapped_address (cache_pc_function_high - 1, > - section); > + section); Spurious change here. Simon
On Tue, 21 Aug 2018 11:56:52 -0400 Simon Marchi <simon.marchi@ericsson.com> wrote: > As I mentioned previously, cache_pc_function_{low,high} could be renamed > to reflect that they now represent the low/high addresses of the range. > If you think it's not necessary, it's also fine, but I just want to make > sure the comment didn't just fall through the cracks. Once I realized that find_pc_partial_function no longer needed to track both the minimum/maximum function addresses AND the range in which PC is found, I decided to leave the name alone. (We only track the latter now.) For a while, I had rewritten it as you had suggested in your earlier review. It definitely made sense to do this when find_pc_partial_function returned values for *ADDRESS and *ENDADDR which referred to the min/max addresses of the function. I'll take another look at it though. If I decide to leave these names unchanged, I'll extend the comment in the declaration for cache_pc_function_{low,high}. Kevin
diff --git a/gdb/blockframe.c b/gdb/blockframe.c index b3c9aa3..85be327 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; } -/* 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; @@ -228,17 +219,64 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address, if (compunit_symtab != NULL) { /* Checking whether the msymbol has a larger value is for the - "pathological" case mentioned in print_frame_info. */ + "pathological" case mentioned in stack.c:find_frame_funname. + + We use BLOCK_ENTRY_PC instead of BLOCK_START_PC for this + comparison because the minimal symbol should refer to the + function's entry pc which is not necessarily the lowest + address of the function. This will happen when the function + has more than one range and the entry pc is not within the + lowest range of addresses. */ 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)) >= 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); + + cache_pc_function_low = BLOCK_START (b); + cache_pc_function_high = BLOCK_END (b); cache_pc_function_name = SYMBOL_LINKAGE_NAME (f); cache_pc_function_section = section; + cache_pc_function_block = b; + + /* For blocks occupying contiguous addresses (i.e. no gaps), + the low and high cache addresses are simply the start + and end of the block. + + For blocks with non-contiguous ranges, we have to search + for the range containing mapped_pc and then use the start + and end of that range. + + This causes the returned *ADDRESS and *ENDADDR values to + be limited to the range in which mapped_pc is found. See + comment preceding declaration of find_pc_partial_function + in symtab.h for more information. */ + + 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)); + } + + goto return_cached_value; } } @@ -268,6 +306,7 @@ 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: @@ -292,12 +331,15 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address, then add one to that. */ *endaddr = 1 + overlay_unmapped_address (cache_pc_function_high - 1, - section); + section); } else *endaddr = cache_pc_function_high; } + if (block != nullptr) + *block = cache_pc_function_block; + return 1; } diff --git a/gdb/symtab.h b/gdb/symtab.h index 84fc897..e518603 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -1683,10 +1683,42 @@ extern struct symbol *find_pc_sect_function (CORE_ADDR, struct obj_section *); extern struct symbol *find_symbol_at_address (CORE_ADDR); -/* lookup function from address, return name, start addr and end addr. */ - -extern int find_pc_partial_function (CORE_ADDR, const char **, CORE_ADDR *, - CORE_ADDR *); +/* 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). If the optional parameter BLOCK is non-null, + then set *BLOCK to the address of the block corresponding to the + function symbol, if such a symbol could be found during the lookup; + nullptr is used as a return value for *BLOCK if no block is found. + 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. + + If the function in question occupies non-contiguous ranges, + *ADDRESS and *ENDADDR are (subject to the conditions noted above) set + to the start and end of the range in which PC is found. Thus + *ADDRESS <= PC < *ENDADDR with no intervening gaps (in which ranges + from other functions might be found). + + This property allows find_pc_partial_function to be used (as it had + been prior to the introduction of non-contiguous range support) by + various tdep files for finding a start address and limit address + for prologue analysis. This still isn't ideal, however, because we + probably shouldn't be doing prologue analysis (in which + instructions are scanned to determine frame size and stack layout) + for any range that doesn't contain the entry pc. Moreover, a good + argument can be made that prologue analysis ought to be performed + starting from the entry pc even when PC is within some other range. + 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. */ + +extern int find_pc_partial_function (CORE_ADDR pc, const char **name, + CORE_ADDR *address, CORE_ADDR *endaddr, + const struct block **block = nullptr); /* Return the type of a function with its first instruction exactly at the PC address. Return NULL otherwise. */
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 regards setting *ADDRESS and *ENDADDR. There are three possibilities which might make sense: 1) *ADDRESS and *ENDADDR represent the lowest and highest address of the function. 2) *ADDRESS and *ENDADDR are set to the start and end address of the range containing the entry pc. 3) *ADDRESS and *ENDADDR are set to the start and end address of the range in which PC is found. An earlier version of this patch implemented option #1. I found out that it's not very useful though and, in fact, returns results that are incorrect when used in the context of determining the start and end of the function for doing prologue analysis. While debugging a function in which the entry pc was in the second range (of a function containing two non-contiguous ranges), I noticed that amd64_skip_prologue called find_pc_partial_function - the returned start address was set to the beginning of the first range. This is incorrect for this function. What was also interesting was that this first invocation of find_pc_partial_function correctly set the cache for the PC on which it had been invoked, but a slightly later call from skip_prologue_using_sal could not use this cached value because it was now being used to lookup the very lowest address of the function - which is in a range not containing the entry pc. Option #2 is attractive as it would provide a desirable result when used in the context of prologue analysis. However, many callers, including some which do prologue analysis want the condition *ADDRESS <= PC < *ENDADDR to hold. This will not be the case when find_pc_partial_function is called on a PC that's in a non-entry-pc range. A later patch to this series adds find_pc_partial_entry_range as a wrapper of find_pc_partial_function. Option #3 causes the *ADDRESS <= PC < *ENDADDR property to hold. If find_pc_partial_function is called with a PC that's within entry pc's range, then it will correctly return the limits of that range. So, if the result of a minsym search is passed to find_pc_partial_function to find the limits, then correct results will be achieved. Returned limits (for prologue analysis) won't be correct when PC is within some other (non-entry-pc) range. I don't yet know how big of a problem this might be; I'm guessing that it won't be a serious problem - if a compiler generates functions which have non-contiguous ranges, then it also probably generates DWARF2 CFI which makes a lot of the old prologue analysis moot. I've implemented option #3 for this version of the patch. I don't see any regressions for x86-64. Moreover, I don't expect to see regressions for other targets either simply because find_pc_partial_function behaves the same as it did before for the contiguous address range case. That said, there may be some adjustments needed if GDB encounters a function with requires prologue analysis which also occupies non-contiguous ranges. gdb/ChangeLog: * symtab.h (find_pc_partial_function): Add new parameter `block'. * blockframe.c (cache_pc_function_block): New static global. (clear_pc_function_cache): Clear cache_pc_function_block. (find_pc_partial_function): Move comment to symtab.h. Add support for non-contiguous blocks. --- gdb/blockframe.c | 78 +++++++++++++++++++++++++++++++++++++++++++------------- gdb/symtab.h | 40 ++++++++++++++++++++++++++--- 2 files changed, 96 insertions(+), 22 deletions(-)