Re: [PATCH 1/8] Add block range data structure for blocks with non-contiguous address ranges

[Simon Marchi <simon.marchi@ericsson.com>]

On 2018-06-26 02:42 AM, Kevin Buettner wrote:
> This patch does the following:
> 
> - Introduces a block range data structure which is accessed via
>   a new field in struct block.
> - Defines several macros for accessing block ranges.
> - Defines a new function, make_blockrange, which is responsible for
>   creating the new data structure.
> - Defines a predicate (function) which checks to see if an address
>   is present in a block.
> 
> It should be noted that some support for non-contiguous ranges already
> existed in GDB in the form of blockvector addrmaps.  This support
> allowed GDB to quickly find a block containing a particular address
> even when the block consists of non-contiguous addresses.  See
> find_block_in_blockvector() in block.c, dwarf2_record_block_ranges()
> in dwarf2read.c, and record_block_range() in buildsym.c.
> 
> Addrmaps do not provide a convenient way to examine address ranges
> associated with a particular block.  This data structure (and its
> interface) is set up for quickly finding the value (which in this case
> is a block) associated with a particular address.  The interface
> does not include a method for doing a reverse mapping from blocks to
> addresses.  A linear time mapping might be attempted via use of the
> addrmap's foreach method, but this is not as straightforward as it
> might first appear due to the fact that blocks corresponding to inline
> function instances and lexical blocks w/ variables end up getting
> interspersed in in the set of transitions.
> 
> Note:  If this approach is deemed to be too expensive in terms of
> space, an alternate approach might be to attempt the linear time
> mapping noted above.  find_pc_partial_function() needs to be able to
> quickly know whether there are discontiguous ranges (for determining
> cache validity), so a flag for this property would have to be added to
> struct block.  Also integral to this set of changes is the concept of
> an "entry pc" which might be different than the block's start address.
> An entry_pc field would also need to be added to struct block.  This
> does not result in any space savings in struct block though since the
> space for the flag and entry_pc use more space than the blockranges
> struct pointer that I've added.  There would, however, be some space
> savings due to the fact that the new data structures that I've added
> for this patch would not need to be allocated.  (I happen to like the
> approach I've come up with, but I wanted to mention another possibility
> just in case someone does not.)

I assume the impact won't be too big (there probably isn't a ton of ranges
per block), but some actual data would be interesting.

Just two nits below, otherwise it LGTM.

> @@ -807,3 +822,27 @@ block_find_non_opaque_type_preferred (struct symbol *sym, void *data)
>    *best = sym;
>    return 0;
>  }
> +
> +/* See block.h.  */
> +
> +struct blockranges *
> +make_blockranges (struct objfile * objfile,

Spurious space after the *.

> @@ -322,4 +401,9 @@ extern int block_find_non_opaque_type_preferred (struct symbol *sym,
>         (sym) != NULL;							\
>         (sym) = block_iter_match_next ((name), &(iter)))
>  
> +/* Given a vector of pairs, allocate and build an obstack allocated
> +   blockranges struct for a block.  */
> +struct blockranges *make_blockranges (struct objfile *objfile,
> +                                      std::vector<std::pair<CORE_ADDR, CORE_ADDR>> &);

Can you please name the second parameter, and make it const?

Thanks,

Simon