[PATCH] Fix two buglets in .debug_names dumping

[PATCH] Fix two buglets in .debug_names dumping

[Tom Tromey]

While working on gdb's .debug_names writer, I found a couple of small
bugs in binutils .debug_names dumping.

First, the DWARF spec (section 6.1.1.4.6 Name Table) says:

    These two arrays are indexed starting at 1, [...]

I think it is clearer for binutils to follow this, particularly
because DW_IDX_parent refers to this number.

Second, I think the handling of an empty hash table is slightly wrong.
Currently the dumping code assumes there is always an array of hashes.
However, section 6.1.1.4.5 Hash Lookup Table says:

    The optional hash lookup table immediately follows the list of
    type signatures.

and then:

    The hash lookup table is actually two separate arrays: an array of
    buckets, followed immediately by an array of hashes.

My reading of this is that the hash table as a whole is optional, and
so the hashes will not exist in this case.  (This also makes sense
because the hashes are not useful without the buckets anyway.)

This patch fixes both of these problems.  FWIW I have some gdb patches
in progress that change gdb both to omit the hash table and to use
DW_IDX_parent.

binutils/ChangeLog
2023-12-04  Tom Tromey  <tom@tromey.com>

	* dwarf.c (display_debug_names): Handle empty .debug_names hash
	table.  Name entries start at 1.
---
 binutils/ChangeLog |  5 +++++
 binutils/dwarf.c   | 11 ++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index 9e7d8753148..47eadfd1c67 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -10353,7 +10353,8 @@ display_debug_names (struct dwarf_section *section, void *file)
       const uint32_t *const hash_table_buckets = (uint32_t *) hdrptr;
       hdrptr += bucket_count * sizeof (uint32_t);
       const uint32_t *const hash_table_hashes = (uint32_t *) hdrptr;
-      hdrptr += name_count * sizeof (uint32_t);
+      if (bucket_count != 0)
+	hdrptr += name_count * sizeof (uint32_t);
       unsigned char *const name_table_string_offsets = hdrptr;
       hdrptr += name_count * offset_size;
       unsigned char *const name_table_entry_offsets = hdrptr;
@@ -10478,8 +10479,12 @@ display_debug_names (struct dwarf_section *section, void *file)
 	  p = name_table_entry_offsets + namei * offset_size;
 	  SAFE_BYTE_GET (entry_offset, p, offset_size, unit_end);
 
-	  printf ("[%3u] #%08x %s:", namei, hash_table_hashes[namei],
-		  fetch_indirect_string (string_offset));
+	  /* The name table is indexed starting at 1 according to
+	     DWARF, so be sure to use the DWARF numbering here.  */
+	  printf ("[%3u] ", namei + 1);
+	  if (bucket_count != 0)
+	    printf ("#%08x ", hash_table_hashes[namei]);
+	  printf ("%s:", fetch_indirect_string (string_offset));
 
 	  unsigned char *entryptr = entry_pool + entry_offset;
 
-- 
2.43.0

Re: [PATCH] Fix two buglets in .debug_names dumping

[Jan Beulich]

On 04.12.2023 16:12, Tom Tromey wrote:
> While working on gdb's .debug_names writer, I found a couple of small
> bugs in binutils .debug_names dumping.
> 
> First, the DWARF spec (section 6.1.1.4.6 Name Table) says:
> 
>     These two arrays are indexed starting at 1, [...]
> 
> I think it is clearer for binutils to follow this, particularly
> because DW_IDX_parent refers to this number.
> 
> Second, I think the handling of an empty hash table is slightly wrong.
> Currently the dumping code assumes there is always an array of hashes.
> However, section 6.1.1.4.5 Hash Lookup Table says:
> 
>     The optional hash lookup table immediately follows the list of
>     type signatures.
> 
> and then:
> 
>     The hash lookup table is actually two separate arrays: an array of
>     buckets, followed immediately by an array of hashes.
> 
> My reading of this is that the hash table as a whole is optional, and
> so the hashes will not exist in this case.  (This also makes sense
> because the hashes are not useful without the buckets anyway.)
> 
> This patch fixes both of these problems.  FWIW I have some gdb patches
> in progress that change gdb both to omit the hash table and to use
> DW_IDX_parent.
> 
> binutils/ChangeLog
> 2023-12-04  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf.c (display_debug_names): Handle empty .debug_names hash
> 	table.  Name entries start at 1.

Looks good to me, feel free to put in.

Jan