Remove addrmap_fixed::set_entry

  It occurred to me that there is no reason for addrmap_fixed::set_entry
to exist.  This patch removes it and removes the abstract virtual
function from the base class.  This then required a few minor changes
in the DWARF reader.  I consider this a type-safety improvement.

Tested by rebuilding.
---
 gdb/addrmap.c        |  9 -----
 gdb/addrmap.h        | 81 +++++++++++++++++++++-----------------------
 gdb/dwarf2/aranges.c |  2 +-
 gdb/dwarf2/aranges.h |  4 +--
 gdb/dwarf2/read.c    |  8 ++---
 5 files changed, 45 insertions(+), 59 deletions(-)
  

On 1/14/24 19:30, Tom Tromey wrote:
> It occurred to me that there is no reason for addrmap_fixed::set_entry
> to exist.  This patch removes it and removes the abstract virtual
> function from the base class.  This then required a few minor changes
> in the DWARF reader.  I consider this a type-safety improvement.
> 

LGTM.

Reviewed-By: Tom de Vries <tdevries@suse.de>

Thanks,
- Tom

> Tested by rebuilding.
> ---
>   gdb/addrmap.c        |  9 -----
>   gdb/addrmap.h        | 81 +++++++++++++++++++++-----------------------
>   gdb/dwarf2/aranges.c |  2 +-
>   gdb/dwarf2/aranges.h |  4 +--
>   gdb/dwarf2/read.c    |  8 ++---
>   5 files changed, 45 insertions(+), 59 deletions(-)
> 
> diff --git a/gdb/addrmap.c b/gdb/addrmap.c
> index 2753f85e0a6..162ab84763b 100644
> --- a/gdb/addrmap.c
> +++ b/gdb/addrmap.c
> @@ -30,15 +30,6 @@ static_assert (sizeof (splay_tree_value) >= sizeof (void *));
>   
>   /* Fixed address maps.  */
>   
> -void
> -addrmap_fixed::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
> -			  void *obj)
> -{
> -  internal_error ("addrmap_fixed_set_empty: "
> -		  "fixed addrmaps can't be changed\n");
> -}
> -
> -
>   void *
>   addrmap_fixed::do_find (CORE_ADDR addr) const
>   {
> diff --git a/gdb/addrmap.h b/gdb/addrmap.h
> index c6417169443..ba83607ad8c 100644
> --- a/gdb/addrmap.h
> +++ b/gdb/addrmap.h
> @@ -46,46 +46,6 @@ struct addrmap
>   {
>     virtual ~addrmap () = default;
>   
> -  /* In the mutable address map MAP, associate the addresses from START
> -     to END_INCLUSIVE that are currently associated with NULL with OBJ
> -     instead.  Addresses mapped to an object other than NULL are left
> -     unchanged.
> -
> -     As the name suggests, END_INCLUSIVE is also mapped to OBJ.  This
> -     convention is unusual, but it allows callers to accurately specify
> -     ranges that abut the top of the address space, and ranges that
> -     cover the entire address space.
> -
> -     This operation seems a bit complicated for a primitive: if it's
> -     needed, why not just have a simpler primitive operation that sets a
> -     range to a value, wiping out whatever was there before, and then
> -     let the caller construct more complicated operations from that,
> -     along with some others for traversal?
> -
> -     It turns out this is the mutation operation we want to use all the
> -     time, at least for now.  Our immediate use for address maps is to
> -     represent lexical blocks whose address ranges are not contiguous.
> -     We walk the tree of lexical blocks present in the debug info, and
> -     only create 'struct block' objects after we've traversed all a
> -     block's children.  If a lexical block declares no local variables
> -     (and isn't the lexical block for a function's body), we omit it
> -     from GDB's data structures entirely.
> -
> -     However, this means that we don't decide to create a block (and
> -     thus record it in the address map) until after we've traversed its
> -     children.  If we do decide to create the block, we do so at a time
> -     when all its children have already been recorded in the map.  So
> -     this operation --- change only those addresses left unset --- is
> -     actually the operation we want to use every time.
> -
> -     It seems simpler to let the code which operates on the
> -     representation directly deal with the hair of implementing these
> -     semantics than to provide an interface which allows it to be
> -     implemented efficiently, but doesn't reveal too much of the
> -     representation.  */
> -  virtual void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
> -			  void *obj) = 0;
> -
>     /* Return the object associated with ADDR in MAP.  */
>     const void *find (CORE_ADDR addr) const
>     { return this->do_find (addr); }
> @@ -127,8 +87,6 @@ struct addrmap_fixed : public addrmap,
>     addrmap_fixed (struct obstack *obstack, addrmap_mutable *mut);
>     DISABLE_COPY_AND_ASSIGN (addrmap_fixed);
>   
> -  void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
> -		  void *obj) override;
>     void relocate (CORE_ADDR offset) override;
>   
>   private:
> @@ -165,8 +123,45 @@ struct addrmap_mutable : public addrmap
>     ~addrmap_mutable ();
>     DISABLE_COPY_AND_ASSIGN (addrmap_mutable);
>   
> +  /* In the mutable address map MAP, associate the addresses from START
> +     to END_INCLUSIVE that are currently associated with NULL with OBJ
> +     instead.  Addresses mapped to an object other than NULL are left
> +     unchanged.
> +
> +     As the name suggests, END_INCLUSIVE is also mapped to OBJ.  This
> +     convention is unusual, but it allows callers to accurately specify
> +     ranges that abut the top of the address space, and ranges that
> +     cover the entire address space.
> +
> +     This operation seems a bit complicated for a primitive: if it's
> +     needed, why not just have a simpler primitive operation that sets a
> +     range to a value, wiping out whatever was there before, and then
> +     let the caller construct more complicated operations from that,
> +     along with some others for traversal?
> +
> +     It turns out this is the mutation operation we want to use all the
> +     time, at least for now.  Our immediate use for address maps is to
> +     represent lexical blocks whose address ranges are not contiguous.
> +     We walk the tree of lexical blocks present in the debug info, and
> +     only create 'struct block' objects after we've traversed all a
> +     block's children.  If a lexical block declares no local variables
> +     (and isn't the lexical block for a function's body), we omit it
> +     from GDB's data structures entirely.
> +
> +     However, this means that we don't decide to create a block (and
> +     thus record it in the address map) until after we've traversed its
> +     children.  If we do decide to create the block, we do so at a time
> +     when all its children have already been recorded in the map.  So
> +     this operation --- change only those addresses left unset --- is
> +     actually the operation we want to use every time.
> +
> +     It seems simpler to let the code which operates on the
> +     representation directly deal with the hair of implementing these
> +     semantics than to provide an interface which allows it to be
> +     implemented efficiently, but doesn't reveal too much of the
> +     representation.  */
>     void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
> -		  void *obj) override;
> +		  void *obj);
>     void relocate (CORE_ADDR offset) override;
>   
>   private:
> diff --git a/gdb/dwarf2/aranges.c b/gdb/dwarf2/aranges.c
> index 0a4ae63801d..4287a5a884b 100644
> --- a/gdb/dwarf2/aranges.c
> +++ b/gdb/dwarf2/aranges.c
> @@ -26,7 +26,7 @@
>   bool
>   read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
>   			   dwarf2_section_info *section,
> -			   addrmap *mutable_map,
> +			   addrmap_mutable *mutable_map,
>   			   deferred_warnings *warn)
>   {
>     /* Caller must ensure that the section has already been read.  */
> diff --git a/gdb/dwarf2/aranges.h b/gdb/dwarf2/aranges.h
> index d923e2d4905..2ba8a05620d 100644
> --- a/gdb/dwarf2/aranges.h
> +++ b/gdb/dwarf2/aranges.h
> @@ -22,7 +22,7 @@
>   
>   class dwarf2_per_objfile;
>   class dwarf2_section_info;
> -class addrmap;
> +class addrmap_mutable;
>   
>   /* Read the address map data from DWARF-5 .debug_aranges, and use it
>      to populate given addrmap.  Returns true on success, false on
> @@ -30,7 +30,7 @@ class addrmap;
>   
>   extern bool read_addrmap_from_aranges (dwarf2_per_objfile *per_objfile,
>   				       dwarf2_section_info *section,
> -				       addrmap *mutable_map,
> +				       addrmap_mutable *mutable_map,
>   				       deferred_warnings *warn);
>   
>   #endif /* GDB_DWARF2_ARANGES_H */
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index a50248c4d56..1b5e566bb01 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -907,7 +907,7 @@ static enum pc_bounds_kind dwarf2_get_pc_bounds (struct die_info *,
>   						 unrelocated_addr *,
>   						 unrelocated_addr *,
>   						 struct dwarf2_cu *,
> -						 addrmap *,
> +						 addrmap_mutable *,
>   						 void *);
>   
>   static void get_scope_pc_bounds (struct die_info *,
> @@ -11013,7 +11013,7 @@ dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu, dwarf_tag tag,
>   static int
>   dwarf2_ranges_read (unsigned offset, unrelocated_addr *low_return,
>   		    unrelocated_addr *high_return, struct dwarf2_cu *cu,
> -		    addrmap *map, void *datum, dwarf_tag tag)
> +		    addrmap_mutable *map, void *datum, dwarf_tag tag)
>   {
>     dwarf2_per_objfile *per_objfile = cu->per_objfile;
>     int low_set = 0;
> @@ -11123,7 +11123,7 @@ dwarf2_get_pc_bounds_entry_point (die_info *die, unrelocated_addr *low,
>   static pc_bounds_kind
>   dwarf_get_pc_bounds_ranges_or_highlow_pc (die_info *die, unrelocated_addr *low,
>   					  unrelocated_addr *high, dwarf2_cu *cu,
> -					  addrmap *map, void *datum)
> +					  addrmap_mutable *map, void *datum)
>   {
>     gdb_assert (low != nullptr);
>     gdb_assert (high != nullptr);
> @@ -11192,7 +11192,7 @@ dwarf_get_pc_bounds_ranges_or_highlow_pc (die_info *die, unrelocated_addr *low,
>   static enum pc_bounds_kind
>   dwarf2_get_pc_bounds (struct die_info *die, unrelocated_addr *lowpc,
>   		      unrelocated_addr *highpc, struct dwarf2_cu *cu,
> -		      addrmap *map, void *datum)
> +		      addrmap_mutable *map, void *datum)
>   {
>     dwarf2_per_objfile *per_objfile = cu->per_objfile;
>