[PATCH v2 0/5] [gdb/symtab] Handle invalid .gdb_index better

[PATCH v2 0/5] [gdb/symtab] Handle invalid .gdb_index better

[Tom de Vries]

I ran into a failure with test-case gdb.tui/tui-missing-src.exp and target
board gold-gdb-index on aarch64-linux.

Investigation showed that this was due to an incorrect address table entry in
the .gdb_index section, with the start of the entry positioned in a hole
between two sections.

The fourth patch detects such entries.

The third patch detects overlapping entries, and the second patch adds some
infrastructure for that patch.

The first patch rejects any .gdb_index with an incorrect adress table.

The last patch changes the related complaints into warnings, making sure that
rejecting the .gdb_index is reported to the user.

The first and fourth patch fix aforementioned test-case.

A v1 was submitted here [1].

New in v2:
- moved the approved patch to be the first patch
- added approved-by tag in first patch
- rewrite commit messages to not refer to other patches, to better allow
  committing patches independently
- add two patches for detecting overlapping entries
- add missing function comment update for create_addrmap_from_gdb_index
- fixed fourth patch to use relocated addresses, after review comment by Simon

[1] https://sourceware.org/pipermail/gdb-patches/2025-August/220087.html

Tom de Vries (5):
  [gdb/symtab] Bail out of create_addrmap_from_gdb_index on error
  [gdb] Make addrmap_mutable::insert_empty return bool
  [gdb/symtab] Detect overlapping ranges in
    create_addrmap_from_gdb_index
  [gdb/symtab] Improve invalid range check in
    create_addrmap_from_gdb_index
  [gdb/symtab] Turn complaints in create_addrmap_from_gdb_index into
    warnings

 gdb/addrmap.c               | 23 +++++++++++----
 gdb/addrmap.h               |  4 +--
 gdb/dwarf2/read-gdb-index.c | 57 +++++++++++++++++++++++++++++--------
 3 files changed, 65 insertions(+), 19 deletions(-)


base-commit: 8186f0d31ddf339ba6803cde97ef6a6310ca2587
-- 
2.43.0

[PATCH v2 1/5] [gdb/symtab] Bail out of create_addrmap_from_gdb_index on error

[Tom de Vries]

Currently, in create_addrmap_from_gdb_index, when finding an incorrect entry
in the address table of a .gdb_index section:
- a (by default silent) complaint is made,
- the entry is skipped, and
- the rest of the entries is processed.

This is the use-what-you-can approach, which make sense in general.

But in the case that the .gdb_index section is incorrect while the other debug
info is correct, this approach prevents gdb from building a correct cooked
index (assuming there's no bug in gdb that would cause an incorrect index to
be generated).

Instead, bail out of create_addrmap_from_gdb_index on finding errors in the
address table.

I wonder about the following potential drawback of this approach: in the case
that the .gdb_index section is incorrect because the debug info is incorrect,
this approach rejects the .gdb_index section and spents time rebuilding a
likewise incorrect index.  But I'm not sure if this is a real problem.
Perhaps gdb will refuse to generate such an index, in which case this is a
non-issue.

Tested on aarch64-linux.

Approved-By: Simon Marchi <simon.marchi@efficios.com>
---
 gdb/dwarf2/read-gdb-index.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
index fc7b654be1a..7db2834f4d3 100644
--- a/gdb/dwarf2/read-gdb-index.c
+++ b/gdb/dwarf2/read-gdb-index.c
@@ -1395,9 +1395,10 @@ create_signatured_type_table_from_gdb_index
   per_bfd->signatured_types = std::move (sig_types_hash);
 }
 
-/* Read the address map data from the mapped GDB index.  */
+/* Read the address map data from the mapped GDB index.  Return true if no
+   errors were found, otherwise return false.  */
 
-static void
+static bool
 create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
 			       mapped_gdb_index *index)
 {
@@ -1423,14 +1424,14 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
 	{
 	  complaint (_(".gdb_index address table has invalid range (%s - %s)"),
 		     hex_string (lo), hex_string (hi));
-	  continue;
+	  return false;
 	}
 
       if (cu_index >= index->units.size ())
 	{
 	  complaint (_(".gdb_index address table has invalid CU number %u"),
 		     (unsigned) cu_index);
-	  continue;
+	  return false;
 	}
 
       mutable_map.set_empty (lo, hi - 1, index->units[cu_index]);
@@ -1438,6 +1439,8 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
 
   index->index_addrmap
     = new (&per_bfd->obstack) addrmap_fixed (&per_bfd->obstack, &mutable_map);
+
+  return true;
 }
 
 /* Sets the name and language of the main function from the shortcut table.  */
@@ -1559,7 +1562,8 @@ dwarf2_read_gdb_index
 
   finalize_all_units (per_bfd);
 
-  create_addrmap_from_gdb_index (per_objfile, map.get ());
+  if (!create_addrmap_from_gdb_index (per_objfile, map.get ()))
+    return false;
 
   set_main_name_from_gdb_index (per_objfile, map.get ());
 
-- 
2.43.0

[PATCH v2 2/5] [gdb] Make addrmap_mutable::insert_empty return bool

[Tom de Vries]

Function addrmap_mutable::set_empty has the follow behavior (shortened
comment):
...
/* 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.  */
  void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
		  void *obj);
...

Change the return type to bool, and return true if the full range
[START, END_INCLUSIVE] is mapped to OBJ.

Tested on x86_64-linux.
---
 gdb/addrmap.c | 23 ++++++++++++++++++-----
 gdb/addrmap.h |  4 ++--
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/gdb/addrmap.c b/gdb/addrmap.c
index 45493509f21..5fddc3c32b7 100644
--- a/gdb/addrmap.c
+++ b/gdb/addrmap.c
@@ -201,10 +201,11 @@ xfree_wrapper (splay_tree_key key)
   xfree ((void *) key);
 }
 
-void
+bool
 addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
 			    void *obj)
 {
+  bool full_range = true;
   splay_tree_node n, next;
   void *prior_value;
 
@@ -234,8 +235,14 @@ addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
        n && addrmap_node_key (n) <= end_inclusive;
        n = splay_tree_successor (addrmap_node_key (n)))
     {
-      if (! addrmap_node_value (n))
-	addrmap_node_set_value (n, obj);
+      if (addrmap_node_value (n))
+	{
+	  /* Already mapped.  */
+	  full_range = false;
+	  continue;
+	}
+
+      addrmap_node_set_value (n, obj);
     }
 
   /* Walk the area again, removing transitions from any value to
@@ -254,6 +261,8 @@ addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
       else
 	prior_value = addrmap_node_value (n);
     }
+
+  return full_range;
 }
 
 
@@ -433,7 +442,9 @@ test_addrmap ()
   check_addrmap_find (map, array, 0, 19, nullptr);
 
   /* Insert address range into mutable addrmap.  */
-  map.set_empty (core_addr (&array[10]), core_addr (&array[12]), val1);
+  bool full_range_p
+    = map.set_empty (core_addr (&array[10]), core_addr (&array[12]), val1);
+  SELF_CHECK (full_range_p);
   check_addrmap_find (map, array, 0, 9, nullptr);
   check_addrmap_find (map, array, 10, 12, val1);
   check_addrmap_find (map, array, 13, 19, nullptr);
@@ -469,7 +480,9 @@ test_addrmap ()
   check_addrmap_find (*map2, array, 14, 19, nullptr);
 
   /* Insert partially overlapping address range into mutable addrmap.  */
-  map.set_empty (core_addr (&array[11]), core_addr (&array[13]), val2);
+  full_range_p
+    = map.set_empty (core_addr (&array[11]), core_addr (&array[13]), val2);
+  SELF_CHECK (!full_range_p);
   check_addrmap_find (map, array, 0, 9, nullptr);
   check_addrmap_find (map, array, 10, 12, val1);
   check_addrmap_find (map, array, 13, 13, val2);
diff --git a/gdb/addrmap.h b/gdb/addrmap.h
index 179e1f86668..398bdd84215 100644
--- a/gdb/addrmap.h
+++ b/gdb/addrmap.h
@@ -152,7 +152,7 @@ struct addrmap_mutable final : public addrmap
   /* 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.
+     unchanged.  Return true if the full range is mapped to OBJ.
 
      As the name suggests, END_INCLUSIVE is also mapped to OBJ.  This
      convention is unusual, but it allows callers to accurately specify
@@ -186,7 +186,7 @@ struct addrmap_mutable final : public addrmap
      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,
+  bool set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
 		  void *obj);
 
   /* Clear this addrmap.  */
-- 
2.43.0

[PATCH v2 3/5] [gdb/symtab] Detect overlapping ranges in create_addrmap_from_gdb_index

[Tom de Vries]

In create_addrmap_from_gdb_index, use the return value of
addrmap_mutable::insert_empty to detect overlapping ranges.

Tested on x86_64-linux.
---
 gdb/dwarf2/read-gdb-index.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
index 7db2834f4d3..79d19a3abaa 100644
--- a/gdb/dwarf2/read-gdb-index.c
+++ b/gdb/dwarf2/read-gdb-index.c
@@ -1434,7 +1434,15 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
 	  return false;
 	}
 
-      mutable_map.set_empty (lo, hi - 1, index->units[cu_index]);
+      bool full_range_p
+	= mutable_map.set_empty (lo, hi - 1, index->units[cu_index]);
+      if (!full_range_p)
+	{
+	  complaint (_(".gdb_index address table has a range (%s - %s) that"
+		       " overlaps with an earlier range"),
+		     hex_string (lo), hex_string (hi));
+	  return false;
+	}
     }
 
   index->index_addrmap
-- 
2.43.0

[PATCH v2 4/5] [gdb/symtab] Improve invalid range check in create_addrmap_from_gdb_index

[Tom de Vries]

When running test-case gdb.tui/tui-missing-src.exp with target board
gold-gdb-index (and likewise fission and fission-dwp) on aarch64-linux, I run
into:
...
FAIL: gdb.tui/tui-missing-src.exp: checking if inside f2 ()
...

Looking at the gold-gdb-index case, the problem is caused by the address table
of the .gdb_index section:
...
Address table:
000000000040066c 0000000000400694 0
000000000040053f 0000000000400563 1
...

The address range for f2 is [0x400694, 0x4006b8), but the address table says
it's [0x40053f, 0x400563).

The address 0x40053f is not even in a section:
...
  [Nr] Name    Type            Address          Off    Size   ES Flg Lk Inf Al
  ...
  [12] .plt    PROGBITS        00000000004004b8 0004b8 000050 10  AX  0   0  8
  [13] .text   PROGBITS        0000000000400540 000540 000178 00  AX  0   0 64
...
but part of the hole [0x400508, 0x400540) in between .plt and .text.

Detect this in the invalid range check in create_addrmap_from_gdb_index.

Tested on aarch64-linux.
---
 gdb/dwarf2/read-gdb-index.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
index 79d19a3abaa..df20b20e081 100644
--- a/gdb/dwarf2/read-gdb-index.c
+++ b/gdb/dwarf2/read-gdb-index.c
@@ -1420,14 +1420,33 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
       cu_index = extract_unsigned_integer (iter, 4, BFD_ENDIAN_LITTLE);
       iter += 4;
 
-      if (lo >= hi)
+      bool valid_range_p = lo < hi;
+      bool valid_index_p = cu_index < index->units.size ();
+
+      /* Variable hi is the exclusive upper bound, get the inclusive one.  */
+      CORE_ADDR hi_m1 = (valid_range_p
+			 ? hi - 1
+			 : 0);
+
+      if (valid_range_p)
+	{
+	  CORE_ADDR relocated_lo
+	    = per_objfile->relocate (unrelocated_addr (lo));
+	  CORE_ADDR relocated_hi_m1
+	    = per_objfile->relocate (unrelocated_addr (hi_m1));
+	  struct obj_section *lo_sect = find_pc_section (relocated_lo);
+	  struct obj_section *hi_sect = find_pc_section (relocated_hi_m1);
+	  valid_range_p = lo_sect != nullptr && hi_sect != nullptr;
+	}
+
+      if (!valid_range_p)
 	{
 	  complaint (_(".gdb_index address table has invalid range (%s - %s)"),
 		     hex_string (lo), hex_string (hi));
 	  return false;
 	}
 
-      if (cu_index >= index->units.size ())
+      if (!valid_index_p)
 	{
 	  complaint (_(".gdb_index address table has invalid CU number %u"),
 		     (unsigned) cu_index);
@@ -1435,7 +1454,7 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
 	}
 
       bool full_range_p
-	= mutable_map.set_empty (lo, hi - 1, index->units[cu_index]);
+	= mutable_map.set_empty (lo, hi_m1, index->units[cu_index]);
       if (!full_range_p)
 	{
 	  complaint (_(".gdb_index address table has a range (%s - %s) that"
-- 
2.43.0

[PATCH v2 5/5] [gdb/symtab] Turn complaints in create_addrmap_from_gdb_index into warnings

[Tom de Vries]

Rather than issuing a complaint, which is off by default, warn when returning
false in create_addrmap_from_gdb_index, informing the user that the .gdb_index
was ignored, and why.

Tested on aarch64-linux.
---
 gdb/dwarf2/read-gdb-index.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
index df20b20e081..5e18c21fb93 100644
--- a/gdb/dwarf2/read-gdb-index.c
+++ b/gdb/dwarf2/read-gdb-index.c
@@ -1441,15 +1441,17 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
 
       if (!valid_range_p)
 	{
-	  complaint (_(".gdb_index address table has invalid range (%s - %s)"),
-		     hex_string (lo), hex_string (hi));
+	  warning (_(".gdb_index address table has invalid range (%s - %s),"
+		     " ignoring .gdb_index"),
+		   hex_string (lo), hex_string (hi));
 	  return false;
 	}
 
       if (!valid_index_p)
 	{
-	  complaint (_(".gdb_index address table has invalid CU number %u"),
-		     (unsigned) cu_index);
+	  warning (_(".gdb_index address table has invalid CU number %u,"
+		     " ignoring .gdb_index"),
+		   (unsigned) cu_index);
 	  return false;
 	}
 
@@ -1457,8 +1459,8 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
 	= mutable_map.set_empty (lo, hi_m1, index->units[cu_index]);
       if (!full_range_p)
 	{
-	  complaint (_(".gdb_index address table has a range (%s - %s) that"
-		       " overlaps with an earlier range"),
+	  warning (_(".gdb_index address table has a range (%s - %s) that"
+		     " overlaps with an earlier range, ignoring .gdb_index"),
 		     hex_string (lo), hex_string (hi));
 	  return false;
 	}
-- 
2.43.0

Re: [PATCH v2 2/5] [gdb] Make addrmap_mutable::insert_empty return bool

[Simon Marchi]

insert_empty -> set_empty in the subject.

Otherwise, LGTM.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH v2 4/5] [gdb/symtab] Improve invalid range check in create_addrmap_from_gdb_index

[Tom de Vries]

On 8/21/25 15:31, Tom de Vries wrote:
> +      if (valid_range_p)
> +	{
> +	  CORE_ADDR relocated_lo
> +	    = per_objfile->relocate (unrelocated_addr (lo));
> +	  CORE_ADDR relocated_hi_m1
> +	    = per_objfile->relocate (unrelocated_addr (hi_m1));
> +	  struct obj_section *lo_sect = find_pc_section (relocated_lo);
> +	  struct obj_section *hi_sect = find_pc_section (relocated_hi_m1);
> +	  valid_range_p = lo_sect != nullptr && hi_sect != nullptr;
> +	}

It turns out that this doesn't always work.

I ran the test-case gdb.base/code_elim.exp (which uses add-symbol-file) 
with target board cc-with-gdb-index, and ran into the following problem.

Function addr_info_make_relative gets called, which puts the section 
table into some relative address state.  Consequently, the relocated 
address are also relative, and aren't found using find_pc_section.

I'll submit a v3 once I've fixed this.

Thanks,
- Tom

Re: [PATCH v2 3/5] [gdb/symtab] Detect overlapping ranges in create_addrmap_from_gdb_index

[Simon Marchi]

On 8/21/25 9:31 AM, Tom de Vries wrote:
> In create_addrmap_from_gdb_index, use the return value of
> addrmap_mutable::insert_empty to detect overlapping ranges.
> 
> Tested on x86_64-linux.
> ---
>  gdb/dwarf2/read-gdb-index.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
> index 7db2834f4d3..79d19a3abaa 100644
> --- a/gdb/dwarf2/read-gdb-index.c
> +++ b/gdb/dwarf2/read-gdb-index.c
> @@ -1434,7 +1434,15 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
>  	  return false;
>  	}
>  
> -      mutable_map.set_empty (lo, hi - 1, index->units[cu_index]);
> +      bool full_range_p
> +	= mutable_map.set_empty (lo, hi - 1, index->units[cu_index]);
> +      if (!full_range_p)
> +	{
> +	  complaint (_(".gdb_index address table has a range (%s - %s) that"
> +		       " overlaps with an earlier range"),
> +		     hex_string (lo), hex_string (hi));
> +	  return false;
> +	}
>      }
>  
>    index->index_addrmap
> -- 
> 2.43.0
> 

LGTM.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH v2 4/5] [gdb/symtab] Improve invalid range check in create_addrmap_from_gdb_index

[Simon Marchi]

On 8/21/25 9:31 AM, Tom de Vries wrote:
> When running test-case gdb.tui/tui-missing-src.exp with target board
> gold-gdb-index (and likewise fission and fission-dwp) on aarch64-linux, I run
> into:
> ...
> FAIL: gdb.tui/tui-missing-src.exp: checking if inside f2 ()
> ...
> 
> Looking at the gold-gdb-index case, the problem is caused by the address table
> of the .gdb_index section:
> ...
> Address table:
> 000000000040066c 0000000000400694 0
> 000000000040053f 0000000000400563 1
> ...
> 
> The address range for f2 is [0x400694, 0x4006b8), but the address table says
> it's [0x40053f, 0x400563).
> 
> The address 0x40053f is not even in a section:
> ...
>   [Nr] Name    Type            Address          Off    Size   ES Flg Lk Inf Al
>   ...
>   [12] .plt    PROGBITS        00000000004004b8 0004b8 000050 10  AX  0   0  8
>   [13] .text   PROGBITS        0000000000400540 000540 000178 00  AX  0   0 64
> ...
> but part of the hole [0x400508, 0x400540) in between .plt and .text.
> 
> Detect this in the invalid range check in create_addrmap_from_gdb_index.
> 
> Tested on aarch64-linux.
> ---
>  gdb/dwarf2/read-gdb-index.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
> index 79d19a3abaa..df20b20e081 100644
> --- a/gdb/dwarf2/read-gdb-index.c
> +++ b/gdb/dwarf2/read-gdb-index.c
> @@ -1420,14 +1420,33 @@ create_addrmap_from_gdb_index (dwarf2_per_objfile *per_objfile,
>        cu_index = extract_unsigned_integer (iter, 4, BFD_ENDIAN_LITTLE);
>        iter += 4;
>  
> -      if (lo >= hi)
> +      bool valid_range_p = lo < hi;
> +      bool valid_index_p = cu_index < index->units.size ();

For readability, I would prefer kepeing each check independent.  There
will be less conditionals below.  To avoid duplicating the warning, you
can use a lambda or free function:

  if (lo >= hi)
    return invalid_range ();

with, for instance:

  auto invalid_range = [&] ()
    {
      complaint (_(".gdb_index address table has invalid range (%s - %s)"),
		 hex_string (lo), hex_string (hi));
      return false;
    };

> +
> +      /* Variable hi is the exclusive upper bound, get the inclusive one.  */
> +      CORE_ADDR hi_m1 = (valid_range_p
> +			 ? hi - 1
> +			 : 0);
> +
> +      if (valid_range_p)
> +	{
> +	  CORE_ADDR relocated_lo
> +	    = per_objfile->relocate (unrelocated_addr (lo));
> +	  CORE_ADDR relocated_hi_m1
> +	    = per_objfile->relocate (unrelocated_addr (hi_m1));
> +	  struct obj_section *lo_sect = find_pc_section (relocated_lo);
> +	  struct obj_section *hi_sect = find_pc_section (relocated_hi_m1);
> +	  valid_range_p = lo_sect != nullptr && hi_sect != nullptr;

It only concerns MIPS, so perhaps not super important (I guess MIPS
programs tend to be not too big), but per_objfile->relocate can be
somewhat heavy, as mips_adjust_dwarf2_addr does one minimal symbol
lookup for each address.  It might not be a problem in practice, but it
would seem much more logical to me if this work was done completely in
the unrelocated domain.  It's an analysis that can be done with the
object file in isolation, independent of whether or where it's loaded in
the inferior.

Simon

Re: [PATCH v2 2/5] [gdb] Make addrmap_mutable::insert_empty return bool

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> @@ -234,8 +235,14 @@ addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
Tom>         n && addrmap_node_key (n) <= end_inclusive;
Tom>         n = splay_tree_successor (addrmap_node_key (n)))
Tom>      {
Tom> -      if (! addrmap_node_value (n))
Tom> -	addrmap_node_set_value (n, obj);
Tom> +      if (addrmap_node_value (n))
Tom> +	{
Tom> +	  /* Already mapped.  */
Tom> +	  full_range = false;
Tom> +	  continue;
Tom> +	}
Tom> +
Tom> +      addrmap_node_set_value (n, obj);

I think using 'else' here would be better than 'continue', since loop
short-circuits are harder to understand.

Tom

Re: [PATCH v2 4/5] [gdb/symtab] Improve invalid range check in create_addrmap_from_gdb_index

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +	  struct obj_section *lo_sect = find_pc_section (relocated_lo);

find_pc_section searches all the objfiles but it seems better to
restrict the search to just this objfile (or related non-debug objfile).

Using the BFD's sections would mean not having to deal with the runtime
offsets as well.

Tom

Re: [PATCH v2 2/5] [gdb] Make addrmap_mutable::insert_empty return bool

[Tom de Vries]

On 8/22/25 20:51, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> @@ -234,8 +235,14 @@ addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
> Tom>         n && addrmap_node_key (n) <= end_inclusive;
> Tom>         n = splay_tree_successor (addrmap_node_key (n)))
> Tom>      {
> Tom> -      if (! addrmap_node_value (n))
> Tom> -	addrmap_node_set_value (n, obj);
> Tom> +      if (addrmap_node_value (n))
> Tom> +	{
> Tom> +	  /* Already mapped.  */
> Tom> +	  full_range = false;
> Tom> +	  continue;
> Tom> +	}
> Tom> +
> Tom> +      addrmap_node_set_value (n, obj);
> 
> I think using 'else' here would be better than 'continue', since loop
> short-circuits are harder to understand.

Hi Tom,

thanks for the review.

I've pushed this using an if/else.

FWIW, I still think that they way I wrote is is much better.  My opinion 
is based on this coding standard rule ( 
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code 
).

Thanks,
- Tom

Re: [PATCH v2 4/5] [gdb/symtab] Improve invalid range check in create_addrmap_from_gdb_index

[Tom de Vries]

On 8/22/25 20:53, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +	  struct obj_section *lo_sect = find_pc_section (relocated_lo);
> 
> find_pc_section searches all the objfiles but it seems better to
> restrict the search to just this objfile (or related non-debug objfile).
> 
> Using the BFD's sections would mean not having to deal with the runtime
> offsets as well.

Hi,

Agreed.

My rationale for using this approach was to reuse code as much as 
possible, but as mentioned here ( 
https://sourceware.org/pipermail/gdb-patches/2025-August/220136.html ), 
it made things more complicated, and in v3 I'm no longer using 
find_pc_section.

Thanks,
- Tom

Re: [PATCH v2 2/5] [gdb] Make addrmap_mutable::insert_empty return bool

[Simon Marchi]

On 2025-08-23 00:20, Tom de Vries wrote:
> On 8/22/25 20:51, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> @@ -234,8 +235,14 @@ addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
>> Tom>         n && addrmap_node_key (n) <= end_inclusive;
>> Tom>         n = splay_tree_successor (addrmap_node_key (n)))
>> Tom>      {
>> Tom> -      if (! addrmap_node_value (n))
>> Tom> -    addrmap_node_set_value (n, obj);
>> Tom> +      if (addrmap_node_value (n))
>> Tom> +    {
>> Tom> +      /* Already mapped.  */
>> Tom> +      full_range = false;
>> Tom> +      continue;
>> Tom> +    }
>> Tom> +
>> Tom> +      addrmap_node_set_value (n, obj);
>>
>> I think using 'else' here would be better than 'continue', since loop
>> short-circuits are harder to understand.
> 
> Hi Tom,
> 
> thanks for the review.
> 
> I've pushed this using an if/else.
> 
> FWIW, I still think that they way I wrote is is much better.  My opinion is based on this coding standard rule ( https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code ).

I also like early returns and early continue in loops.  It tells me: you
don't need to think about that case for the rest of the function loop.

Simon

Re: [PATCH v2 2/5] [gdb] Make addrmap_mutable::insert_empty return bool

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Tom> +      if (addrmap_node_value (n))
Tom> +    {
Tom> +      /* Already mapped.  */
Tom> +      full_range = false;
Tom> +      continue;
Tom> +    }
Tom> +
Tom> +      addrmap_node_set_value (n, obj);

>> FWIW, I still think that they way I wrote is is much better.  My
>> opinion is based on this coding standard rule (
>> https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
>> ).

I don't think we should use LLVM's coding standards.  Particularly so
now that I've worked on LLVM this last year.

Simon> I also like early returns and early continue in loops.  It tells me: you
Simon> don't need to think about that case for the rest of the function loop.

In this particular case the two branches are single lines.
Using a continue here seems more confusing for that reason -- it's just
more to process here.

I get it for longer loops, at least situationally.

Tom

Re: [PATCH v2 2/5] [gdb] Make addrmap_mutable::insert_empty return bool

[Tom de Vries]

On 8/29/25 02:20, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Tom> +      if (addrmap_node_value (n))
> Tom> +    {
> Tom> +      /* Already mapped.  */
> Tom> +      full_range = false;
> Tom> +      continue;
> Tom> +    }
> Tom> +
> Tom> +      addrmap_node_set_value (n, obj);
> 
>>> FWIW, I still think that they way I wrote is is much better.  My
>>> opinion is based on this coding standard rule (
>>> https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
>>> ).
> 
> I don't think we should use LLVM's coding standards. 

Hi Tom,

I realize I'm probably not going to convince you, so all I'm trying to 
do here is to better make my argument, since I have not done a good job 
sofar.

I'm not advocating that we should use LLVM's coding standards, regardless.

I'm saying that we could use some of them if they make sense and don't 
conflict with the existing coding standard.

Reading the rule was a light-bulb moment for me, and has shaped my 
opinion on how to write code.

Agreed, it can be argued that the rule doesn't apply here because the 
if-else branches are too small.

It also says "Aim to reduce indentation where possible when it doesn’t 
make it more difficult to understand the code".

Of course it's highly debatable how to fill this in.

For me, it makes sense to look at the impact of the branches.

To illustrate my point, I think it makes sense to do:
...
if (...)
   a = b;
else
   c = d;
...
but not
...
if (...)
   a = b;
else
   exec ("rm -Rf /")
...

In the latter case, I'd rather write:
...
if (!...)
   {
      a = b;
      continue;
   }

   exec ("rm -Rf /");
...
to have the most impactful statement stand out both by less indentation, 
and by having it standalone rather than as part of an if/else.

And I feel the same applies to:
...
if (...)
   { /* Don't do something and make a note of it.  */ }
else
   { /* Do something. */ }
...

> Particularly so
> now that I've worked on LLVM this last year.
> 
> Simon> I also like early returns and early continue in loops.  It tells me: you
> Simon> don't need to think about that case for the rest of the function loop.
> 
> In this particular case the two branches are single lines.

FWIW, one branch is one line, the other 4 lines:
...
       if (addrmap_node_value (n))
         {
           /* Already mapped.  */
           full_range = false;
         }
       else
         addrmap_node_set_value (n, obj);
...

Thanks,
- Tom

> Using a continue here seems more confusing for that reason -- it's just
> more to process here.
> 
> I get it for longer loops, at least situationally.
> 
> Tom