[PATCH][gdb/symtab] Fix parsing of .debug_str_offsets header

[PATCH][gdb/symtab] Fix parsing of .debug_str_offsets header

[Tom de Vries]

Hi,

When running test-case gdb.dwarf2/fission-mix.exp with target board dwarf64
and gcc-12 (defaulting to DWARF5), I run into:
...
(gdb) break func2^M
Offset from DW_FORM_GNU_str_index or DW_FORM_strx pointing outside of \
  .debug_str.dwo section in CU at offset 0x0 [in module fission-mix]^M
(gdb) FAIL: gdb.dwarf2/fission-mix.exp: break func2
...

The .debug_str_offsets section has version 5, so as per the standard it has
it's own header, with initial length and version:
...
Contents of the .debug_str_offsets.dwo section (loaded from fission-mix2.dwo):

    Length: 0x1c
    Version: 0x5
       Index   Offset [String]
           0        0 build/gdb/testsuite
           1       33 GNU C17
           2       8f src/gdb/testsuite/gdb.dwarf2/fission-mix-2.c
...

But when trying to read the string offset at index 0 in the table (which
is 0), we start reading at offset 8, which points in the header, at the last
4 bytes of the initial length (it's 12 bytes because of 64-bit dwarf), as well
at the 2-byte version field and 2 bytes of padding, so we get:
...
(gdb) p /x str_offset
$1 = 0x500000000
...
which indeed is an offset that doesn't fit in the .debug_str section.

The offset 8 is based on reader->cu->header.addr_size:
...
static const char *
read_dwo_str_index (const struct die_reader_specs *reader, ULONGEST str_index)
{
 ULONGEST str_offsets_base = reader->cu->header.version >= 5
                             ? reader->cu->header.addr_size : 0;
...
which doesn't in look in agreement with the standard.

Note that this happens to give the right answer for 32-bit dwarf and
addr_size == 8, because then we have header size ==
(initial length (4) + version (2) + padding (2)) == 8.

Conversely, for 32-bit dwarf and addr_size == 4 (target board unix/-m32)
we run into a similar problem.  It just happens to not trigger the warning,
instead we get the wrong strings, like "func2" for DW_AT_producer and
"build/gdb/testsuite" for DW_AT_name of the DW_TAG_compile_unit DIE.

Fix this by parsing the .debug_str_offsets header in read_dwo_str_index.

Add a FIXME that we should not parse this for every call.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/symtab] Fix parsing of .debug_str_offsets header

---
 gdb/dwarf2/read.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 7 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 848fd5627b8..73889fe7eba 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -19378,7 +19378,8 @@ static const char *
 read_str_index (struct dwarf2_cu *cu,
 		struct dwarf2_section_info *str_section,
 		struct dwarf2_section_info *str_offsets_section,
-		ULONGEST str_offsets_base, ULONGEST str_index)
+		ULONGEST str_offsets_base, ULONGEST str_index,
+		unsigned offset_size)
 {
   dwarf2_per_objfile *per_objfile = cu->per_objfile;
   struct objfile *objfile = per_objfile->objfile;
@@ -19402,8 +19403,8 @@ read_str_index (struct dwarf2_cu *cu,
 	   sect_offset_str (cu->header.sect_off), objf_name);
   info_ptr = (str_offsets_section->buffer
 	      + str_offsets_base
-	      + str_index * cu->header.offset_size);
-  if (cu->header.offset_size == 4)
+	      + str_index * offset_size);
+  if (offset_size == 4)
     str_offset = bfd_get_32 (abfd, info_ptr);
   else
     str_offset = bfd_get_64 (abfd, info_ptr);
@@ -19419,12 +19420,60 @@ read_str_index (struct dwarf2_cu *cu,
 static const char *
 read_dwo_str_index (const struct die_reader_specs *reader, ULONGEST str_index)
 {
-  ULONGEST str_offsets_base = reader->cu->header.version >= 5
-			      ? reader->cu->header.addr_size : 0;
+  unsigned offset_size;
+  ULONGEST str_offsets_base;
+  if (reader->cu->header.version >= 5)
+    {
+      /* We have a DWARF5 CU with a reference to a .debug_str_offsets section,
+         so assume the .debug_str_offsets section is DWARF5 as well, and
+	 parse the header.  FIXME: Parse the header only once.  */
+      unsigned int bytes_read = 0;
+      bfd *abfd = reader->dwo_file->sections.str_offsets.get_bfd_owner ();
+      const gdb_byte *p = reader->dwo_file->sections.str_offsets.buffer;
+
+      /* Header: Initial length.  */
+      read_initial_length (abfd, p + bytes_read, &bytes_read);
+
+      /* Determine offset_size based on the .debug_str_offsets header.  */
+      const bool dwarf5_is_dwarf64 = bytes_read != 4;
+      offset_size = dwarf5_is_dwarf64 ? 8 : 4;
+
+      /* Header: Version.  */
+      unsigned version = read_2_bytes (abfd, p + bytes_read);
+      bytes_read += 2;
+
+      if (version <= 4)
+	{
+	  /* We'd like one warning here about ignoring the section, but
+	     because we parse the header more than once (see FIXME above)
+	     we'd have many warnings, so use a complaint instead, which at
+	     least has a limit. */
+	  complaint (_("Section .debug_str_offsets in %s has unsupported"
+		       " version %d, use empty string."),
+		   reader->dwo_file->dwo_name, version);
+	  return "";
+	}
+
+      /* Header: Padding.  */
+      bytes_read += 2;
+
+      str_offsets_base = bytes_read;
+    }
+  else
+    {
+      /* We have a pre-DWARF5 CU with a reference to a .debug_str_offsets
+	 section, assume the .debug_str_offsets section is pre-DWARF5 as
+	 well, which doesn't have a header.  */
+      str_offsets_base = 0;
+
+      /* Determine offset_size based on the .debug_info header.  */
+      offset_size = reader->cu->header.offset_size;
+  }
+
   return read_str_index (reader->cu,
 			 &reader->dwo_file->sections.str,
 			 &reader->dwo_file->sections.str_offsets,
-			 str_offsets_base, str_index);
+			 str_offsets_base, str_index, offset_size);
 }
 
 /* Given a DW_FORM_GNU_str_index from a Fission stub, fetch the string.  */
@@ -19446,7 +19495,8 @@ read_stub_str_index (struct dwarf2_cu *cu, ULONGEST str_index)
   return read_str_index (cu,
 			 &cu->per_objfile->per_bfd->str,
 			 &cu->per_objfile->per_bfd->str_offsets,
-			 *cu->str_offsets_base, str_index);
+			 *cu->str_offsets_base, str_index,
+			 cu->header.offset_size);
 }
 
 /* Return the length of an LEB128 number in BUF.  */

[committed][gdb/symtab] Fix parsing of .debug_str_offsets header

[Tom de Vries]

On 6/1/22 18:35, Tom de Vries wrote:
> Hi,
> 
> When running test-case gdb.dwarf2/fission-mix.exp with target board dwarf64
> and gcc-12 (defaulting to DWARF5), I run into:
> ...
> (gdb) break func2^M
> Offset from DW_FORM_GNU_str_index or DW_FORM_strx pointing outside of \
>    .debug_str.dwo section in CU at offset 0x0 [in module fission-mix]^M
> (gdb) FAIL: gdb.dwarf2/fission-mix.exp: break func2
> ...
> 
> The .debug_str_offsets section has version 5, so as per the standard it has
> it's own header, with initial length and version:
> ...
> Contents of the .debug_str_offsets.dwo section (loaded from fission-mix2.dwo):
> 
>      Length: 0x1c
>      Version: 0x5
>         Index   Offset [String]
>             0        0 build/gdb/testsuite
>             1       33 GNU C17
>             2       8f src/gdb/testsuite/gdb.dwarf2/fission-mix-2.c
> ...
> 
> But when trying to read the string offset at index 0 in the table (which
> is 0), we start reading at offset 8, which points in the header, at the last
> 4 bytes of the initial length (it's 12 bytes because of 64-bit dwarf), as well
> at the 2-byte version field and 2 bytes of padding, so we get:
> ...
> (gdb) p /x str_offset
> $1 = 0x500000000
> ...
> which indeed is an offset that doesn't fit in the .debug_str section.
> 
> The offset 8 is based on reader->cu->header.addr_size:
> ...
> static const char *
> read_dwo_str_index (const struct die_reader_specs *reader, ULONGEST str_index)
> {
>   ULONGEST str_offsets_base = reader->cu->header.version >= 5
>                               ? reader->cu->header.addr_size : 0;
> ...
> which doesn't in look in agreement with the standard.
> 
> Note that this happens to give the right answer for 32-bit dwarf and
> addr_size == 8, because then we have header size ==
> (initial length (4) + version (2) + padding (2)) == 8.
> 
> Conversely, for 32-bit dwarf and addr_size == 4 (target board unix/-m32)
> we run into a similar problem.  It just happens to not trigger the warning,
> instead we get the wrong strings, like "func2" for DW_AT_producer and
> "build/gdb/testsuite" for DW_AT_name of the DW_TAG_compile_unit DIE.
> 
> Fix this by parsing the .debug_str_offsets header in read_dwo_str_index.
> 
> Add a FIXME that we should not parse this for every call.
> 
> Tested on x86_64-linux.
> 
> Any comments?

Committed.

Thanks,
- Tom

> [gdb/symtab] Fix parsing of .debug_str_offsets header
> 
> ---
>   gdb/dwarf2/read.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 57 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 848fd5627b8..73889fe7eba 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -19378,7 +19378,8 @@ static const char *
>   read_str_index (struct dwarf2_cu *cu,
>   		struct dwarf2_section_info *str_section,
>   		struct dwarf2_section_info *str_offsets_section,
> -		ULONGEST str_offsets_base, ULONGEST str_index)
> +		ULONGEST str_offsets_base, ULONGEST str_index,
> +		unsigned offset_size)
>   {
>     dwarf2_per_objfile *per_objfile = cu->per_objfile;
>     struct objfile *objfile = per_objfile->objfile;
> @@ -19402,8 +19403,8 @@ read_str_index (struct dwarf2_cu *cu,
>   	   sect_offset_str (cu->header.sect_off), objf_name);
>     info_ptr = (str_offsets_section->buffer
>   	      + str_offsets_base
> -	      + str_index * cu->header.offset_size);
> -  if (cu->header.offset_size == 4)
> +	      + str_index * offset_size);
> +  if (offset_size == 4)
>       str_offset = bfd_get_32 (abfd, info_ptr);
>     else
>       str_offset = bfd_get_64 (abfd, info_ptr);
> @@ -19419,12 +19420,60 @@ read_str_index (struct dwarf2_cu *cu,
>   static const char *
>   read_dwo_str_index (const struct die_reader_specs *reader, ULONGEST str_index)
>   {
> -  ULONGEST str_offsets_base = reader->cu->header.version >= 5
> -			      ? reader->cu->header.addr_size : 0;
> +  unsigned offset_size;
> +  ULONGEST str_offsets_base;
> +  if (reader->cu->header.version >= 5)
> +    {
> +      /* We have a DWARF5 CU with a reference to a .debug_str_offsets section,
> +         so assume the .debug_str_offsets section is DWARF5 as well, and
> +	 parse the header.  FIXME: Parse the header only once.  */
> +      unsigned int bytes_read = 0;
> +      bfd *abfd = reader->dwo_file->sections.str_offsets.get_bfd_owner ();
> +      const gdb_byte *p = reader->dwo_file->sections.str_offsets.buffer;
> +
> +      /* Header: Initial length.  */
> +      read_initial_length (abfd, p + bytes_read, &bytes_read);
> +
> +      /* Determine offset_size based on the .debug_str_offsets header.  */
> +      const bool dwarf5_is_dwarf64 = bytes_read != 4;
> +      offset_size = dwarf5_is_dwarf64 ? 8 : 4;
> +
> +      /* Header: Version.  */
> +      unsigned version = read_2_bytes (abfd, p + bytes_read);
> +      bytes_read += 2;
> +
> +      if (version <= 4)
> +	{
> +	  /* We'd like one warning here about ignoring the section, but
> +	     because we parse the header more than once (see FIXME above)
> +	     we'd have many warnings, so use a complaint instead, which at
> +	     least has a limit. */
> +	  complaint (_("Section .debug_str_offsets in %s has unsupported"
> +		       " version %d, use empty string."),
> +		   reader->dwo_file->dwo_name, version);
> +	  return "";
> +	}
> +
> +      /* Header: Padding.  */
> +      bytes_read += 2;
> +
> +      str_offsets_base = bytes_read;
> +    }
> +  else
> +    {
> +      /* We have a pre-DWARF5 CU with a reference to a .debug_str_offsets
> +	 section, assume the .debug_str_offsets section is pre-DWARF5 as
> +	 well, which doesn't have a header.  */
> +      str_offsets_base = 0;
> +
> +      /* Determine offset_size based on the .debug_info header.  */
> +      offset_size = reader->cu->header.offset_size;
> +  }
> +
>     return read_str_index (reader->cu,
>   			 &reader->dwo_file->sections.str,
>   			 &reader->dwo_file->sections.str_offsets,
> -			 str_offsets_base, str_index);
> +			 str_offsets_base, str_index, offset_size);
>   }
>   
>   /* Given a DW_FORM_GNU_str_index from a Fission stub, fetch the string.  */
> @@ -19446,7 +19495,8 @@ read_stub_str_index (struct dwarf2_cu *cu, ULONGEST str_index)
>     return read_str_index (cu,
>   			 &cu->per_objfile->per_bfd->str,
>   			 &cu->per_objfile->per_bfd->str_offsets,
> -			 *cu->str_offsets_base, str_index);
> +			 *cu->str_offsets_base, str_index,
> +			 cu->header.offset_size);
>   }
>   
>   /* Return the length of an LEB128 number in BUF.  */