[PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Potharla, Rupesh]

[-- Attachment #1: Type: text/plain, Size: 3918 bytes --]

[Public]



While working on the implementation of DW_FORM_strx forms could not print file names even after the implementation of strx forms. I found an issue with adding the file names to the file table with dwarf5 and clang.

With dwarf5 debug line version the file index is starting with zero, but the code is expecting it to be 1 which is the case with other dwarf versions.

From the contents of .debug_line compiled with clang and dwarf5, the file names array index is starting with zero.

standard_opcode_lengths[DW_LNS_set_isa] = 1
include_directories[  0] = "/home/rupesh/addr2line"
file_names[  0]:
           name: "prog1.c"
      dir_index: 0
   md5_checksum: da4ea4c312af96d39b13557acdf23f05

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------


The below line skipping zero entry was added as part of commit 19d80e5fec548e681c453d15b4ae5b49bc080acc is ignoring the file names in the zeroth index. I have no idea why this line was added. Removing the line is working for programs compiled with clang using dwarf5. With my fix, I am not seeing any issues with GCC and dwarf5 moreover currently GCC's debug_line version is 3 even when compiled with dwarf5.

      /* Skip the first "zero entry", which is the compilation dir/file.  */
      if (datai != 0)
        if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
          return false;

Made code changes to fix this issue. Can you review the code changes and send in your comments/suggestions?

Regards,
Rupesh P



From 28e92539dfe5319e7bdfea32c4ee46f55ff51053 Mon Sep 17 00:00:00 2001
From: rupothar rupesh.potharla@amd.com<mailto:rupesh.potharla@amd.com>
Date: Mon, 9 May 2022 12:10:48 +0530
Subject: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

---
bfd/dwarf2.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
index fe6c0b907c4..3233d281275 100644
--- a/bfd/dwarf2.c
+++ b/bfd/dwarf2.c
@@ -1617,6 +1617,7 @@ struct line_info_table
   unsigned int                    num_files;
   unsigned int                    num_dirs;
   unsigned int                    num_sequences;
+  unsigned int                   version;
   char *                   comp_dir;
   char **                  dirs;
   struct fileinfo*        files;
@@ -1837,6 +1838,8 @@ concat_filename (struct line_info_table *table, unsigned int file)
{
   char *filename;

+  if (table->version >= 5)
+    file = file + 1;
   if (table == NULL || file - 1 >= table->num_files)
     {
       /* FILE == 0 means unknown.  */
@@ -2270,10 +2273,8 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,
             }
         }

-      /* Skip the first "zero entry", which is the compilation dir/file.  */
-      if (datai != 0)
-         if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
-           return false;
+      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
+        return false;
     }

   *bufp = buf;
@@ -2438,6 +2439,7 @@ decode_line_info (struct comp_unit *unit)
   table->sequences = NULL;

   table->lcl_head = NULL;
+  table->version = lh.version;

   if (lh.version >= 5)
     {
@@ -2480,7 +2482,12 @@ decode_line_info (struct comp_unit *unit)
       /* State machine registers.  */
       bfd_vma address = 0;
       unsigned char op_index = 0;
-      char * filename = table->num_files ? concat_filename (table, 1) : NULL;
+      char *filename;
+      if (table->version >= 5)
+        filename = table->num_files ? concat_filename (table, 0) : NULL;
+      else
+        filename = table->num_files ? concat_filename (table, 1) : NULL;
+
       unsigned int line = 1;
       unsigned int column = 0;
       unsigned int discriminator = 0;
--
2.17.1


[-- Attachment #2: 0001-bfd-Fix-issues-with-files-in-debug_line-table-with-d.patch --]
[-- Type: application/octet-stream, Size: 2059 bytes --]

From 28e92539dfe5319e7bdfea32c4ee46f55ff51053 Mon Sep 17 00:00:00 2001
From: rupothar <rupesh.potharla@amd.com>
Date: Mon, 9 May 2022 12:10:48 +0530
Subject: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

---
 bfd/dwarf2.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
index fe6c0b907c4..3233d281275 100644
--- a/bfd/dwarf2.c
+++ b/bfd/dwarf2.c
@@ -1617,6 +1617,7 @@ struct line_info_table
   unsigned int		num_files;
   unsigned int		num_dirs;
   unsigned int		num_sequences;
+  unsigned int 		version;
   char *		comp_dir;
   char **		dirs;
   struct fileinfo*	files;
@@ -1837,6 +1838,8 @@ concat_filename (struct line_info_table *table, unsigned int file)
 {
   char *filename;
 
+  if (table->version >= 5)
+    file = file + 1;
   if (table == NULL || file - 1 >= table->num_files)
     {
       /* FILE == 0 means unknown.  */
@@ -2270,10 +2273,8 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,
 	    }
 	}
 
-      /* Skip the first "zero entry", which is the compilation dir/file.  */
-      if (datai != 0)
-	if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
-	  return false;
+      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
+	return false;
     }
 
   *bufp = buf;
@@ -2438,6 +2439,7 @@ decode_line_info (struct comp_unit *unit)
   table->sequences = NULL;
 
   table->lcl_head = NULL;
+  table->version = lh.version;
 
   if (lh.version >= 5)
     {
@@ -2480,7 +2482,12 @@ decode_line_info (struct comp_unit *unit)
       /* State machine registers.  */
       bfd_vma address = 0;
       unsigned char op_index = 0;
-      char * filename = table->num_files ? concat_filename (table, 1) : NULL;
+      char *filename;
+      if (table->version >= 5)
+	filename = table->num_files ? concat_filename (table, 0) : NULL;
+      else
+	filename = table->num_files ? concat_filename (table, 1) : NULL;
+
       unsigned int line = 1;
       unsigned int column = 0;
       unsigned int discriminator = 0;
-- 
2.17.1

Re: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Jan Beulich]

On 09.05.2022 09:03, Potharla, Rupesh via Binutils wrote:
> [Public]
> 
> 
> 
> While working on the implementation of DW_FORM_strx forms could not print file names even after the implementation of strx forms. I found an issue with adding the file names to the file table with dwarf5 and clang.
> 
> With dwarf5 debug line version the file index is starting with zero, but the code is expecting it to be 1 which is the case with other dwarf versions.
> 
> From the contents of .debug_line compiled with clang and dwarf5, the file names array index is starting with zero.
> 
> standard_opcode_lengths[DW_LNS_set_isa] = 1
> include_directories[  0] = "/home/rupesh/addr2line"
> file_names[  0]:
>            name: "prog1.c"
>       dir_index: 0
>    md5_checksum: da4ea4c312af96d39b13557acdf23f05
> 
> Address            Line   Column File   ISA Discriminator Flags
> ------------------ ------ ------ ------ --- ------------- -------------
> 
> 
> The below line skipping zero entry was added as part of commit 19d80e5fec548e681c453d15b4ae5b49bc080acc is ignoring the file names in the zeroth index. I have no idea why this line was added. Removing the line is working for programs compiled with clang using dwarf5. With my fix, I am not seeing any issues with GCC and dwarf5 moreover currently GCC's debug_line version is 3 even when compiled with dwarf5.
> 
>       /* Skip the first "zero entry", which is the compilation dir/file.  */
>       if (datai != 0)
>         if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>           return false;
> 
> Made code changes to fix this issue. Can you review the code changes and send in your comments/suggestions?
> 
> Regards,
> Rupesh P

Much of the above wants to go ...

> From 28e92539dfe5319e7bdfea32c4ee46f55ff51053 Mon Sep 17 00:00:00 2001
> From: rupothar rupesh.potharla@amd.com<mailto:rupesh.potharla@amd.com>
> Date: Mon, 9 May 2022 12:10:48 +0530
> Subject: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.
> 
> ---

... above this marker, to become the actual commit message.

> @@ -2270,10 +2273,8 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,
>              }
>          }
> 
> -      /* Skip the first "zero entry", which is the compilation dir/file.  */
> -      if (datai != 0)
> -         if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
> -           return false;
> +      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
> +        return false;
>      }

How come this change doesn't add a version check?

To help being certain this is the right way of changing things, can you please
add up to two testcases (readelf and/or objdump), one for a version < 5 (unless
one such already exists and hence it would be visible there that you don't
unduly alter handling of those older versions) and one for version 5?

Jan

RE: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Potharla, Rupesh]

[-- Attachment #1: Type: text/plain, Size: 6624 bytes --]

[Public]


Thanks Jan for reviewing the code changes,


>> -      /* Skip the first "zero entry", which is the compilation dir/file.  */
>> -      if (datai != 0)
>> -         if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>> -           return false;
>> +      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>> +        return false;
>>      }
>
>How come this change doesn't add a version check?
>
Since the function read_formatted_entries is only called for version 5 in the file, I thought the version check is not needed. Now I have added the condition and updated the patch. 
 
>To help being certain this is the right way of changing things, can you please
>add up to two testcases (readelf and/or objdump), one for a version < 5
>(unless one such already exists and hence it would be visible there that you
>don't unduly alter handling of those older versions) and one for version 5?
>

Readelf and objdump are not using dwarf2.c file under bfd directory these tools are using dwarf.c under binutils directory.  Currently I am using addr2line to test and validate my code changes manually. There are no testcases for addr2line and I did not find any testcases for bfd library as well.  Please let me know your suggestions .
 
Regards,
Rupesh P

---
 bfd/dwarf2.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
index f6b0183720b..c5b5d14fc9f 100644
--- a/bfd/dwarf2.c
+++ b/bfd/dwarf2.c
@@ -1572,6 +1572,7 @@ struct line_info_table
   unsigned int		num_files;
   unsigned int		num_dirs;
   unsigned int		num_sequences;
+  unsigned int 		version;
   char *		comp_dir;
   char **		dirs;
   struct fileinfo*	files;
@@ -1792,6 +1793,8 @@ concat_filename (struct line_info_table *table, unsigned int file)
 {
   char *filename;
 
+  if (table->version >= 5)
+    file = file + 1;
   if (table == NULL || file - 1 >= table->num_files)
     {
       /* FILE == 0 means unknown.  */
@@ -2414,7 +2417,7 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,
 	}
 
       /* Skip the first "zero entry", which is the compilation dir/file.  */
-      if (datai != 0)
+      if (table->version < 5 && datai != 0)
 	if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
 	  return false;
     }
@@ -2581,6 +2584,7 @@ decode_line_info (struct comp_unit *unit)
   table->sequences = NULL;
 
   table->lcl_head = NULL;
+  table->version = lh.version;
 
   if (lh.version >= 5)
     {
@@ -2623,7 +2627,12 @@ decode_line_info (struct comp_unit *unit)
       /* State machine registers.  */
       bfd_vma address = 0;
       unsigned char op_index = 0;
-      char * filename = table->num_files ? concat_filename (table, 1) : NULL;
+      char *filename;
+      if (table->version >= 5)
+	filename = table->num_files ? concat_filename (table, 0) : NULL;
+      else
+	filename = table->num_files ? concat_filename (table, 1) : NULL;
+
       unsigned int line = 1;
       unsigned int column = 0;
       unsigned int discriminator = 0;
-- 
2.17.1



>-----Original Message-----
>From: Jan Beulich <jbeulich@suse.com>
>Sent: Wednesday, May 18, 2022 6:07 PM
>To: Potharla, Rupesh <Rupesh.Potharla@amd.com>; Potharla, Rupesh via
>Binutils <binutils@sourceware.org>
>Cc: George, Jini Susan <JiniSusan.George@amd.com>; Parasuraman,
>Hariharan <Hariharan.Parasuraman@amd.com>; Natarajan, Kavitha
><Kavitha.Natarajan@amd.com>
>Subject: Re: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.
>
>[CAUTION: External Email]
>
>On 09.05.2022 09:03, Potharla, Rupesh via Binutils wrote:
>> [Public]
>>
>>
>>
>> While working on the implementation of DW_FORM_strx forms could not
>print file names even after the implementation of strx forms. I found an issue
>with adding the file names to the file table with dwarf5 and clang.
>>
>> With dwarf5 debug line version the file index is starting with zero, but the
>code is expecting it to be 1 which is the case with other dwarf versions.
>>
>> From the contents of .debug_line compiled with clang and dwarf5, the file
>names array index is starting with zero.
>>
>> standard_opcode_lengths[DW_LNS_set_isa] = 1 include_directories[  0] =
>> "/home/rupesh/addr2line"
>> file_names[  0]:
>>            name: "prog1.c"
>>       dir_index: 0
>>    md5_checksum: da4ea4c312af96d39b13557acdf23f05
>>
>> Address            Line   Column File   ISA Discriminator Flags
>> ------------------ ------ ------ ------ --- -------------
>> -------------
>>
>>
>> The below line skipping zero entry was added as part of commit
>19d80e5fec548e681c453d15b4ae5b49bc080acc is ignoring the file names in
>the zeroth index. I have no idea why this line was added. Removing the line is
>working for programs compiled with clang using dwarf5. With my fix, I am not
>seeing any issues with GCC and dwarf5 moreover currently GCC's debug_line
>version is 3 even when compiled with dwarf5.
>>
>>       /* Skip the first "zero entry", which is the compilation dir/file.  */
>>       if (datai != 0)
>>         if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>>           return false;
>>
>> Made code changes to fix this issue. Can you review the code changes and
>send in your comments/suggestions?
>>
>> Regards,
>> Rupesh P
>
>Much of the above wants to go ...
>
>> From 28e92539dfe5319e7bdfea32c4ee46f55ff51053 Mon Sep 17 00:00:00
>2001
>> From: rupothar
>rupesh.potharla@amd.com<mailto:rupesh.potharla@amd.com>
>> Date: Mon, 9 May 2022 12:10:48 +0530
>> Subject: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.
>>
>> ---
>
>... above this marker, to become the actual commit message.
>
>> @@ -2270,10 +2273,8 @@ read_formatted_entries (struct comp_unit *unit,
>bfd_byte **bufp,
>>              }
>>          }
>>
>> -      /* Skip the first "zero entry", which is the compilation dir/file.  */
>> -      if (datai != 0)
>> -         if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>> -           return false;
>> +      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>> +        return false;
>>      }
>
>How come this change doesn't add a version check?
>
>To help being certain this is the right way of changing things, can you please
>add up to two testcases (readelf and/or objdump), one for a version < 5
>(unless one such already exists and hence it would be visible there that you
>don't unduly alter handling of those older versions) and one for version 5?
>
>Jan

[-- Attachment #2: 0001-bfd-Fix-issues-with-files-in-debug_line-table-with-d.patch --]
[-- Type: application/octet-stream, Size: 1994 bytes --]

From 1898ad99680a70e81e9cc2ca4aa69285599fea21 Mon Sep 17 00:00:00 2001
From: rupothar <rupesh.potharla@amd.com>
Date: Wed, 25 May 2022 09:22:48 +0530
Subject: [PATCH]  bfd: Fix issues with files in debug_line table with dwarf5.

---
 bfd/dwarf2.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
index f6b0183720b..c5b5d14fc9f 100644
--- a/bfd/dwarf2.c
+++ b/bfd/dwarf2.c
@@ -1572,6 +1572,7 @@ struct line_info_table
   unsigned int		num_files;
   unsigned int		num_dirs;
   unsigned int		num_sequences;
+  unsigned int 		version;
   char *		comp_dir;
   char **		dirs;
   struct fileinfo*	files;
@@ -1792,6 +1793,8 @@ concat_filename (struct line_info_table *table, unsigned int file)
 {
   char *filename;
 
+  if (table->version >= 5)
+    file = file + 1;
   if (table == NULL || file - 1 >= table->num_files)
     {
       /* FILE == 0 means unknown.  */
@@ -2414,7 +2417,7 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,
 	}
 
       /* Skip the first "zero entry", which is the compilation dir/file.  */
-      if (datai != 0)
+      if (table->version < 5 && datai != 0)
 	if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
 	  return false;
     }
@@ -2581,6 +2584,7 @@ decode_line_info (struct comp_unit *unit)
   table->sequences = NULL;
 
   table->lcl_head = NULL;
+  table->version = lh.version;
 
   if (lh.version >= 5)
     {
@@ -2623,7 +2627,12 @@ decode_line_info (struct comp_unit *unit)
       /* State machine registers.  */
       bfd_vma address = 0;
       unsigned char op_index = 0;
-      char * filename = table->num_files ? concat_filename (table, 1) : NULL;
+      char *filename;
+      if (table->version >= 5)
+	filename = table->num_files ? concat_filename (table, 0) : NULL;
+      else
+	filename = table->num_files ? concat_filename (table, 1) : NULL;
+
       unsigned int line = 1;
       unsigned int column = 0;
       unsigned int discriminator = 0;
-- 
2.17.1

Re: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Jan Beulich]

On 25.05.2022 06:20, Potharla, Rupesh wrote:
>>> -      /* Skip the first "zero entry", which is the compilation dir/file.  */
>>> -      if (datai != 0)
>>> -         if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>>> -           return false;
>>> +      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>>> +        return false;
>>>      }
>>
>> How come this change doesn't add a version check?
>>
> Since the function read_formatted_entries is only called for version 5 in the file, I thought the version check is not needed. Now I have added the condition and updated the patch. 

Oh, I'm sorry - I hadn't noticed this aspect. I don't think a version
check is needed then.

>> To help being certain this is the right way of changing things, can you please
>> add up to two testcases (readelf and/or objdump), one for a version < 5
>> (unless one such already exists and hence it would be visible there that you
>> don't unduly alter handling of those older versions) and one for version 5?
>>
> 
> Readelf and objdump are not using dwarf2.c file under bfd directory these tools are using dwarf.c under binutils directory.

Argh, yes - too many dwarf*.c in the tree.

>  Currently I am using addr2line to test and validate my code changes manually. There are no testcases for addr2line and I did not find any testcases for bfd library as well.  Please let me know your suggestions .

Is there a reason speaking against adding an addr2line test? The
generic framework looks to know of addr2line. (Perhaps ideally the
same source would be used for both an addr2line test and a readelf or
objdump one, as to prove that the two forms of Dwarf reading are
actually in sync. But I guess that's asking for too much.)

Jan

RE: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Potharla, Rupesh]

[AMD Official Use Only - General]



>-----Original Message-----
>From: Jan Beulich <jbeulich@suse.com>
>Sent: Wednesday, May 25, 2022 11:49 AM
>To: Potharla, Rupesh <Rupesh.Potharla@amd.com>
>Cc: George, Jini Susan <JiniSusan.George@amd.com>; Parasuraman,
>Hariharan <Hariharan.Parasuraman@amd.com>; Natarajan, Kavitha
><Kavitha.Natarajan@amd.com>; Potharla, Rupesh via Binutils
><binutils@sourceware.org>
>Subject: Re: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.
>
>[CAUTION: External Email]
>
>On 25.05.2022 06:20, Potharla, Rupesh wrote:
>>>> -      /* Skip the first "zero entry", which is the compilation dir/file.  */
>>>> -      if (datai != 0)
>>>> -         if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>>>> -           return false;
>>>> +      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>>>> +        return false;
>>>>      }
>>>
>>> How come this change doesn't add a version check?
>>>
>> Since the function read_formatted_entries is only called for version 5 in the
>file, I thought the version check is not needed. Now I have added the
>condition and updated the patch.
>
>Oh, I'm sorry - I hadn't noticed this aspect. I don't think a version check is
>needed then.
>
>>> To help being certain this is the right way of changing things, can
>>> you please add up to two testcases (readelf and/or objdump), one for
>>> a version < 5 (unless one such already exists and hence it would be
>>> visible there that you don't unduly alter handling of those older versions)
>and one for version 5?
>>>
>>
>> Readelf and objdump are not using dwarf2.c file under bfd directory these
>tools are using dwarf.c under binutils directory.
>
>Argh, yes - too many dwarf*.c in the tree.
>
>>  Currently I am using addr2line to test and validate my code changes
>manually. There are no testcases for addr2line and I did not find any testcases
>for bfd library as well.  Please let me know your suggestions .
>
>Is there a reason speaking against adding an addr2line test? The generic
>framework looks to know of addr2line. (Perhaps ideally the same source
>would be used for both an addr2line test and a readelf or objdump one, as to
>prove that the two forms of Dwarf reading are actually in sync. But I guess
>that's asking for too much.)
>

Regarding test for addr2line, the input address given to addr2line is retrieved from output of nm/objdump tool. The functioning of the addr2line tool depends on the reliability of other tools.

>Jan

RE: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Potharla, Rupesh]

[-- Attachment #1: Type: text/plain, Size: 9770 bytes --]

[AMD Official Use Only - General]


Hi Jan,

As you suggested, added a new test case of addr2line.  Found a minor bug in read_indexed_address function and fixed that as well. Can you review my code changes and let me know your suggestions/comments?


Regards,
Rupesh P


Fixed an issue with the file index for dwarf5.
Added addr2line test case.
read_indexed_address is using offset_size instead of addr_size
while reading addrx forms, fixed that as well.
---
 bfd/dwarf2.c                                  | 21 ++++---
 binutils/testsuite/binutils-all/addr2line.exp | 59 +++++++++++++++++++
 binutils/testsuite/config/default.exp         |  6 ++
 3 files changed, 78 insertions(+), 8 deletions(-)
 create mode 100644 binutils/testsuite/binutils-all/addr2line.exp

diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
index aaa2d84887f..2a992d67bc7 100644
--- a/bfd/dwarf2.c
+++ b/bfd/dwarf2.c
@@ -1369,7 +1369,7 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)
 		     &file->dwarf_addr_buffer, &file->dwarf_addr_size))
     return 0;
 
-  if (_bfd_mul_overflow (idx, unit->offset_size, &offset))
+  if (_bfd_mul_overflow (idx, unit->addr_size, &offset))
     return 0;
 
   offset += unit->dwarf_addr_offset;
@@ -1380,9 +1380,9 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)
 
   info_ptr = file->dwarf_addr_buffer + offset;
 
-  if (unit->offset_size == 4)
+  if (unit->addr_size == 4)
     return bfd_get_32 (unit->abfd, info_ptr);
-  else if (unit->offset_size == 8)
+  else if (unit->addr_size == 8)
     return bfd_get_64 (unit->abfd, info_ptr);
   else
     return 0;
@@ -1731,6 +1731,7 @@ struct line_info_table
   unsigned int		num_files;
   unsigned int		num_dirs;
   unsigned int		num_sequences;
+  unsigned int 		version;
   char *		comp_dir;
   char **		dirs;
   struct fileinfo*	files;
@@ -1951,6 +1952,8 @@ concat_filename (struct line_info_table *table, unsigned int file)
 {
   char *filename;
 
+  if (table->version >= 5)
+    file = file + 1;
   if (table == NULL || file - 1 >= table->num_files)
     {
       /* FILE == 0 means unknown.  */
@@ -2579,10 +2582,8 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,
 	    }
 	}
 
-      /* Skip the first "zero entry", which is the compilation dir/file.  */
-      if (datai != 0)
-	if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
-	  return false;
+      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
+	return false;
     }
 
   *bufp = buf;
@@ -2747,6 +2748,7 @@ decode_line_info (struct comp_unit *unit)
   table->sequences = NULL;
 
   table->lcl_head = NULL;
+  table->version = lh.version;
 
   if (lh.version >= 5)
     {
@@ -2789,13 +2791,16 @@ decode_line_info (struct comp_unit *unit)
       /* State machine registers.  */
       bfd_vma address = 0;
       unsigned char op_index = 0;
-      char * filename = table->num_files ? concat_filename (table, 1) : NULL;
+      char *filename;
+      int index = table->version >= 5 ? 0 : 1;
       unsigned int line = 1;
       unsigned int column = 0;
       unsigned int discriminator = 0;
       int is_stmt = lh.default_is_stmt;
       int end_sequence = 0;
       unsigned int dir, xtime, size;
+
+      filename = table->num_files ? concat_filename (table, index) : NULL;
       /* eraxxon@alumni.rice.edu: Against the DWARF2 specs, some
 	 compilers generate address sequences that are wildly out of
 	 order using DW_LNE_set_address (e.g. Intel C++ 6.0 compiler
diff --git a/binutils/testsuite/binutils-all/addr2line.exp b/binutils/testsuite/binutils-all/addr2line.exp
new file mode 100644
index 00000000000..153e83c2ace
--- /dev/null
+++ b/binutils/testsuite/binutils-all/addr2line.exp
@@ -0,0 +1,59 @@
+#   Copyright (C) 2018-2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
+
+    global $NM
+    global $ADDR2LINE
+
+    set testname "addr2line"
+    if { [target_compile $srcdir/$subdir/testprog.c tmpdir/testprog executable debug] != "" } {
+        verbose "Unable to compile test file."
+        untested "addr2line"
+        return
+    }
+
+    #testcase for default option.
+    set output [binutils_run $NM "tmpdir/testprog"]
+    regexp -line {^[0-9]+\s+[A-Z]\s+main} $output contents
+    set list [regexp -inline -all -- {\S+} $contents]
+
+    set got [binutils_run $ADDR2LINE "-e tmpdir/testprog  [lindex $list 0]"]
+    set want "$srcdir/$subdir/testprog.c:\[0-9\]+"
+    if ![regexp $want $got] then {
+        fail "$testname $got\n"
+    } else {
+        pass "$testname"
+    }
+
+    #testcase for -f option.
+    regexp -line {^[0-9]+\s+[A-Z]\s+fn} $output contents
+    set list [regexp -inline -all -- {\S+} $contents]
+
+    set got [binutils_run $ADDR2LINE "-f -e tmpdir/testprog  [lindex $list 0]"]
+    set want "fn\n$srcdir/$subdir/testprog.c:\[0-9\]+"
+    if ![regexp $want $got] then {
+        fail "$testname -f option $got\n"
+    } else {
+        pass "$testname -f option"
+    }
+
+    #testcase for -s option.
+    set got [binutils_run $ADDR2LINE "-s -e tmpdir/testprog  [lindex $list 0]"]
+    set want "testprog.c:\[0-9\]+"
+    if ![regexp $want $got] then {
+        fail "$testname -s option $got\n"
+    } else {
+        pass "$testname -s option"
+    }
diff --git a/binutils/testsuite/config/default.exp b/binutils/testsuite/config/default.exp
index c654bd4081c..7192c929a83 100644
--- a/binutils/testsuite/config/default.exp
+++ b/binutils/testsuite/config/default.exp
@@ -40,6 +40,12 @@ if ![info exists NM] then {
 if ![info exists NMFLAGS] then {
     set NMFLAGS ""
 }
+if ![info exists ADDR2LINE] then {
+    set ADDR2LINE [findfile $base_dir/addr2line $base_dir/addr2line [transform nm]]
+}
+if ![info exists ADDR2LINEFLAGS] then {
+    set ADDR2LINEFLAGS ""
+}
 if ![info exists SIZE] then {
     set SIZE [findfile $base_dir/size]
 }
-- 
2.17.1



>-----Original Message-----
>From: Potharla, Rupesh
>Sent: Tuesday, May 31, 2022 11:38 PM
>To: Jan Beulich <jbeulich@suse.com>
>Cc: George, Jini Susan <JiniSusan.George@amd.com>; Parasuraman,
>Hariharan <Hariharan.Parasuraman@amd.com>; Natarajan, Kavitha
><Kavitha.Natarajan@amd.com>; Potharla, Rupesh via Binutils
><binutils@sourceware.org>
>Subject: RE: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.
>
>[AMD Official Use Only - General]
>
>
>
>>-----Original Message-----
>>From: Jan Beulich <jbeulich@suse.com>
>>Sent: Wednesday, May 25, 2022 11:49 AM
>>To: Potharla, Rupesh <Rupesh.Potharla@amd.com>
>>Cc: George, Jini Susan <JiniSusan.George@amd.com>; Parasuraman,
>>Hariharan <Hariharan.Parasuraman@amd.com>; Natarajan, Kavitha
>><Kavitha.Natarajan@amd.com>; Potharla, Rupesh via Binutils
>><binutils@sourceware.org>
>>Subject: Re: [PATCH] bfd: Fix issues with files in debug_line table with
>dwarf5.
>>
>>[CAUTION: External Email]
>>
>>On 25.05.2022 06:20, Potharla, Rupesh wrote:
>>>>> -      /* Skip the first "zero entry", which is the compilation dir/file.  */
>>>>> -      if (datai != 0)
>>>>> -         if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>>>>> -           return false;
>>>>> +      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
>>>>> +        return false;
>>>>>      }
>>>>
>>>> How come this change doesn't add a version check?
>>>>
>>> Since the function read_formatted_entries is only called for version
>>> 5 in the
>>file, I thought the version check is not needed. Now I have added the
>>condition and updated the patch.
>>
>>Oh, I'm sorry - I hadn't noticed this aspect. I don't think a version
>>check is needed then.
>>
>>>> To help being certain this is the right way of changing things, can
>>>> you please add up to two testcases (readelf and/or objdump), one for
>>>> a version < 5 (unless one such already exists and hence it would be
>>>> visible there that you don't unduly alter handling of those older
>>>> versions)
>>and one for version 5?
>>>>
>>>
>>> Readelf and objdump are not using dwarf2.c file under bfd directory
>>> these
>>tools are using dwarf.c under binutils directory.
>>
>>Argh, yes - too many dwarf*.c in the tree.
>>
>>>  Currently I am using addr2line to test and validate my code changes
>>manually. There are no testcases for addr2line and I did not find any
>>testcases for bfd library as well.  Please let me know your suggestions .
>>
>>Is there a reason speaking against adding an addr2line test? The
>>generic framework looks to know of addr2line. (Perhaps ideally the same
>>source would be used for both an addr2line test and a readelf or
>>objdump one, as to prove that the two forms of Dwarf reading are
>>actually in sync. But I guess that's asking for too much.)
>>
>
>Regarding test for addr2line, the input address given to addr2line is retrieved
>from output of nm/objdump tool. The functioning of the addr2line tool
>depends on the reliability of other tools.
>
>>Jan

[-- Attachment #2: 0001-bfd-Fix-issues-with-files-in-debug_line-table-with-d.docx --]
[-- Type: application/vnd.openxmlformats-officedocument.wordprocessingml.document, Size: 17590 bytes --]

Re: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Jan Beulich]

On 05.07.2022 07:05, Potharla, Rupesh wrote:
> As you suggested, added a new test case of addr2line.  Found a minor bug in read_indexed_address function and fixed that as well. Can you review my code changes and let me know your suggestions/comments?

Please can you submit your change as an ordinary patch, _To_ the list?

Jan

Re: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Jan Beulich]

On 05.07.2022 08:46, Potharla, Rupesh via Binutils wrote:
> Patch Inline:

Please note how the inlined version has extra blank lines inserted
between any two actual lines, whereas the attached variant is a
*.docx rather than a *.patch. I'll use the inlined version for the
few comments / questions I have, but you will want to sort your mail
setup.

> --- a/bfd/dwarf2.c
> 
> +++ b/bfd/dwarf2.c
> 
> @@ -1369,7 +1369,7 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)
> 
>                             &file->dwarf_addr_buffer, &file->dwarf_addr_size))
> 
>      return 0;
> 
> -  if (_bfd_mul_overflow (idx, unit->offset_size, &offset))
> 
> +  if (_bfd_mul_overflow (idx, unit->addr_size, &offset))
> 
>      return 0;
> 
>    offset += unit->dwarf_addr_offset;
> 
> @@ -1380,9 +1380,9 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)
> 
>    info_ptr = file->dwarf_addr_buffer + offset;
> 
> -  if (unit->offset_size == 4)
> 
> +  if (unit->addr_size == 4)
> 
>      return bfd_get_32 (unit->abfd, info_ptr);
> 
> -  else if (unit->offset_size == 8)
> 
> +  else if (unit->addr_size == 8)
> 
>      return bfd_get_64 (unit->abfd, info_ptr);
> 
>    else
> 
>      return 0;
> 

I have to admit that I don't see the (direct) connection to the
patch title here - are these changes perhaps better to form a
standalone change?

> @@ -1951,6 +1952,8 @@ concat_filename (struct line_info_table *table, unsigned int file)
> 
> {
> 
>    char *filename;
> 
> +  if (table->version >= 5)
> 
> +    file = file + 1;
> 

Don't you need either this ...

>    if (table == NULL || file - 1 >= table->num_files)
> 
>      {
> 
>        /* FILE == 0 means unknown.  */
> 
> @@ -2579,10 +2582,8 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,
> 
>                }
> 
>            }
> 
> -      /* Skip the first "zero entry", which is the compilation dir/file.  */
> 
> -      if (datai != 0)
> 
> -           if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
> 
> -             return false;
> 
> +      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
> 
> +          return false;
> 
>      }

... or this adjustment, but not both at the same time?

> --- /dev/null
> 
> +++ b/binutils/testsuite/binutils-all/addr2line.exp
> 
> @@ -0,0 +1,59 @@
> 
> +#   Copyright (C) 2018-2022 Free Software Foundation, Inc.
> 
> +
> 
> +# This program is free software; you can redistribute it and/or modify
> 
> +# it under the terms of the GNU General Public License as published by
> 
> +# the Free Software Foundation; either version 3 of the License, or
> 
> +# (at your option) any later version.
> 
> +#
> 
> +# This program is distributed in the hope that it will be useful,
> 
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> 
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> 
> +# GNU General Public License for more details.
> 
> +#
> 
> +# You should have received a copy of the GNU General Public License
> 
> +# along with this program; if not, write to the Free Software
> 
> +# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.
> 
> +
> 
> +    global $NM
> 
> +    global $ADDR2LINE
> 
> +
> 
> +    set testname "addr2line"
> 
> +    if { [target_compile $srcdir/$subdir/testprog.c tmpdir/testprog executable debug] != "" } {

Could you help me spotting where the Dwarf version to be used it coming
from? (Ideally the new test would be run once for v5 and once with an
earlier version; yet more ideally once per version. Yet I'm not sure
how easy this would be to achieve.)

> +        verbose "Unable to compile test file."
> 
> +        untested "addr2line"
> 
> +        return
> 
> +    }
> 
> +
> 
> +    #testcase for default option.
> 
> +    set output [binutils_run $NM "tmpdir/testprog"]
> 
> +    regexp -line {^[0-9]+\s+[A-Z]\s+main} $output contents
> 
> +    set list [regexp -inline -all -- {\S+} $contents]
> 
> +
> 
> +    set got [binutils_run $ADDR2LINE "-e tmpdir/testprog  [lindex $list 0]"]
> 
> +    set want "$srcdir/$subdir/testprog.c:\[0-9\]+"
> 
> +    if ![regexp $want $got] then {
> 
> +        fail "$testname $got\n"
> 
> +    } else {
> 
> +        pass "$testname"
> 
> +    }

I have to admit that I can't really spot what you're actually looking
for, and hence whether that's sufficient for a test. Rather than all of
this open-coding, did you consider using the pre-cooked run_dump_test?

Jan

Re: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Jan Beulich]

On 20.07.2022 07:03, Potharla, Rupesh via Binutils wrote:
> Can you review my code changes and send in your comments ?

I'm sorry, I've been meaning to get to it, but so far didn't due
to lack of time. It is on my list of things to look at.

Jan

RE: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Potharla, Rupesh]

[Public]

Hi,

Can you review my code changes and send in your comments ?

Regards,
Rupesh P
From: Potharla, Rupesh
Sent: Tuesday, July 5, 2022 12:16 PM
To: Potharla, Rupesh via Binutils <binutils@sourceware.org>
Cc: Parasuraman, Hariharan <Hariharan.Parasuraman@amd.com>; George, Jini Susan <JiniSusan.George@amd.com>; Natarajan, Kavitha <Kavitha.Natarajan@amd.com>
Subject: [PATCH] bfd: Fix issues with files in debug_line table with dwarf5.


[Public]




Hi,


While working on the implementation of DW_FORM_strx forms, addr2line tool could not print file names even after the implementation of strx forms. I found an issue with adding the file names to the file table with dwarf5 and clang.


With dwarf5 debug line version, the file index is starting with zero, but the code is expecting it to be 1 which is the case with other dwarf versions.

From the contents of .debug_line compiled with clang and dwarf5, the file names array index is starting with zero.

standard_opcode_lengths[DW_LNS_set_isa] = 1
include_directories[  0] = "/home/rupesh/addr2line"
file_names[  0]:
           name: "prog1.c"
      dir_index: 0
   md5_checksum: da4ea4c312af96d39b13557acdf23f05

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------


The below line skipping zero entry was added as part of commit 19d80e5fec548e681c453d15b4ae5b49bc080acc is ignoring the file names in the zeroth index. I have no idea why this line was added. Removing the line is working for programs compiled with clang using dwarf5. With my fix, I am not seeing any issues with GCC and dwarf5 because currently GCC's debug_line version is 3 even when compiled with dwarf5.

      /* Skip the first "zero entry", which is the compilation dir/file.  */
      if (datai != 0)
        if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
          return false;

Made code changes to fix this issue. Added a new test case for addr2line to test the changes. And fixed a minor bug in read_indexed_address where in offset_size is used instead of addr_size.  Can you review the code changes and send in your comments/suggestions?



Regards,

Rupesh P



Patch Inline:



Fixed an issue with the file index for dwarf5.

Added addr2line test case.

read_indexed_address is using offset_size instead of addr_size

while reading addrx forms, fixed that as well.

---

bfd/dwarf2.c                                  | 21 ++++---

binutils/testsuite/binutils-all/addr2line.exp | 59 +++++++++++++++++++

binutils/testsuite/config/default.exp         |  6 ++

3 files changed, 78 insertions(+), 8 deletions(-)

create mode 100644 binutils/testsuite/binutils-all/addr2line.exp

diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c

index aaa2d84887f..2a992d67bc7 100644

--- a/bfd/dwarf2.c

+++ b/bfd/dwarf2.c

@@ -1369,7 +1369,7 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)

                            &file->dwarf_addr_buffer, &file->dwarf_addr_size))

     return 0;



-  if (_bfd_mul_overflow (idx, unit->offset_size, &offset))

+  if (_bfd_mul_overflow (idx, unit->addr_size, &offset))

     return 0;



   offset += unit->dwarf_addr_offset;

@@ -1380,9 +1380,9 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)



   info_ptr = file->dwarf_addr_buffer + offset;



-  if (unit->offset_size == 4)

+  if (unit->addr_size == 4)

     return bfd_get_32 (unit->abfd, info_ptr);

-  else if (unit->offset_size == 8)

+  else if (unit->addr_size == 8)

     return bfd_get_64 (unit->abfd, info_ptr);

   else

     return 0;

@@ -1731,6 +1731,7 @@ struct line_info_table

   unsigned int              num_files;

   unsigned int              num_dirs;

   unsigned int              num_sequences;

+  unsigned int             version;

   char *                        comp_dir;

   char **                       dirs;

   struct fileinfo* files;

@@ -1951,6 +1952,8 @@ concat_filename (struct line_info_table *table, unsigned int file)

{

   char *filename;



+  if (table->version >= 5)

+    file = file + 1;

   if (table == NULL || file - 1 >= table->num_files)

     {

       /* FILE == 0 means unknown.  */

@@ -2579,10 +2582,8 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,

               }

           }



-      /* Skip the first "zero entry", which is the compilation dir/file.  */

-      if (datai != 0)

-           if (!callback (table, fe.name, fe.dir, fe.time, fe.size))

-             return false;

+      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))

+          return false;

     }



   *bufp = buf;

@@ -2747,6 +2748,7 @@ decode_line_info (struct comp_unit *unit)

   table->sequences = NULL;



   table->lcl_head = NULL;

+  table->version = lh.version;



   if (lh.version >= 5)

     {

@@ -2789,13 +2791,16 @@ decode_line_info (struct comp_unit *unit)

       /* State machine registers.  */

       bfd_vma address = 0;

       unsigned char op_index = 0;

-      char * filename = table->num_files ? concat_filename (table, 1) : NULL;

+      char *filename;

+      int index = table->version >= 5 ? 0 : 1;

       unsigned int line = 1;

       unsigned int column = 0;

       unsigned int discriminator = 0;

       int is_stmt = lh.default_is_stmt;

       int end_sequence = 0;

       unsigned int dir, xtime, size;

+

+      filename = table->num_files ? concat_filename (table, index) : NULL;

       /* eraxxon@alumni.rice.edu<mailto:eraxxon@alumni.rice.edu>: Against the DWARF2 specs, some

            compilers generate address sequences that are wildly out of

            order using DW_LNE_set_address (e.g. Intel C++ 6.0 compiler

diff --git a/binutils/testsuite/binutils-all/addr2line.exp b/binutils/testsuite/binutils-all/addr2line.exp

new file mode 100644

index 00000000000..153e83c2ace

--- /dev/null

+++ b/binutils/testsuite/binutils-all/addr2line.exp

@@ -0,0 +1,59 @@

+#   Copyright (C) 2018-2022 Free Software Foundation, Inc.

+

+# This program is free software; you can redistribute it and/or modify

+# it under the terms of the GNU General Public License as published by

+# the Free Software Foundation; either version 3 of the License, or

+# (at your option) any later version.

+#

+# This program is distributed in the hope that it will be useful,

+# but WITHOUT ANY WARRANTY; without even the implied warranty of

+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

+# GNU General Public License for more details.

+#

+# You should have received a copy of the GNU General Public License

+# along with this program; if not, write to the Free Software

+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.

+

+    global $NM

+    global $ADDR2LINE

+

+    set testname "addr2line"

+    if { [target_compile $srcdir/$subdir/testprog.c tmpdir/testprog executable debug] != "" } {

+        verbose "Unable to compile test file."

+        untested "addr2line"

+        return

+    }

+

+    #testcase for default option.

+    set output [binutils_run $NM "tmpdir/testprog"]

+    regexp -line {^[0-9]+\s+[A-Z]\s+main} $output contents

+    set list [regexp -inline -all -- {\S+} $contents]

+

+    set got [binutils_run $ADDR2LINE "-e tmpdir/testprog  [lindex $list 0]"]

+    set want "$srcdir/$subdir/testprog.c:\[0-9\]+"

+    if ![regexp $want $got] then {

+        fail "$testname $got\n"

+    } else {

+        pass "$testname"

+    }

+

+    #testcase for -f option.

+    regexp -line {^[0-9]+\s+[A-Z]\s+fn} $output contents

+    set list [regexp -inline -all -- {\S+} $contents]

+

+    set got [binutils_run $ADDR2LINE "-f -e tmpdir/testprog  [lindex $list 0]"]

+    set want "fn\n$srcdir/$subdir/testprog.c:\[0-9\]+"

+    if ![regexp $want $got] then {

+        fail "$testname -f option $got\n"

+    } else {

+        pass "$testname -f option"

+    }

+

+    #testcase for -s option.

+    set got [binutils_run $ADDR2LINE "-s -e tmpdir/testprog  [lindex $list 0]"]

+    set want "testprog.c:\[0-9\]+"

+    if ![regexp $want $got] then {

+        fail "$testname -s option $got\n"

+    } else {

+        pass "$testname -s option"

+    }

diff --git a/binutils/testsuite/config/default.exp b/binutils/testsuite/config/default.exp

index c654bd4081c..7192c929a83 100644

--- a/binutils/testsuite/config/default.exp

+++ b/binutils/testsuite/config/default.exp

@@ -40,6 +40,12 @@ if ![info exists NM] then {

if ![info exists NMFLAGS] then {

     set NMFLAGS ""

}

+if ![info exists ADDR2LINE] then {

+    set ADDR2LINE [findfile $base_dir/addr2line $base_dir/addr2line [transform nm]]

+}

+if ![info exists ADDR2LINEFLAGS] then {

+    set ADDR2LINEFLAGS ""

+}

if ![info exists SIZE] then {

     set SIZE [findfile $base_dir/size]

}

--

2.17.1

[PATCH] bfd: Fix issues with files in debug_line table with dwarf5.

[Potharla, Rupesh]

[-- Attachment #1: Type: text/plain, Size: 8731 bytes --]

[Public]




Hi,


While working on the implementation of DW_FORM_strx forms, addr2line tool could not print file names even after the implementation of strx forms. I found an issue with adding the file names to the file table with dwarf5 and clang.


With dwarf5 debug line version, the file index is starting with zero, but the code is expecting it to be 1 which is the case with other dwarf versions.

From the contents of .debug_line compiled with clang and dwarf5, the file names array index is starting with zero.

standard_opcode_lengths[DW_LNS_set_isa] = 1
include_directories[  0] = "/home/rupesh/addr2line"
file_names[  0]:
           name: "prog1.c"
      dir_index: 0
   md5_checksum: da4ea4c312af96d39b13557acdf23f05

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------


The below line skipping zero entry was added as part of commit 19d80e5fec548e681c453d15b4ae5b49bc080acc is ignoring the file names in the zeroth index. I have no idea why this line was added. Removing the line is working for programs compiled with clang using dwarf5. With my fix, I am not seeing any issues with GCC and dwarf5 because currently GCC's debug_line version is 3 even when compiled with dwarf5.

      /* Skip the first "zero entry", which is the compilation dir/file.  */
      if (datai != 0)
        if (!callback (table, fe.name, fe.dir, fe.time, fe.size))
          return false;

Made code changes to fix this issue. Added a new test case for addr2line to test the changes. And fixed a minor bug in read_indexed_address where in offset_size is used instead of addr_size.  Can you review the code changes and send in your comments/suggestions?



Regards,

Rupesh P



Patch Inline:



Fixed an issue with the file index for dwarf5.

Added addr2line test case.

read_indexed_address is using offset_size instead of addr_size

while reading addrx forms, fixed that as well.

---

bfd/dwarf2.c                                  | 21 ++++---

binutils/testsuite/binutils-all/addr2line.exp | 59 +++++++++++++++++++

binutils/testsuite/config/default.exp         |  6 ++

3 files changed, 78 insertions(+), 8 deletions(-)

create mode 100644 binutils/testsuite/binutils-all/addr2line.exp

diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c

index aaa2d84887f..2a992d67bc7 100644

--- a/bfd/dwarf2.c

+++ b/bfd/dwarf2.c

@@ -1369,7 +1369,7 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)

                            &file->dwarf_addr_buffer, &file->dwarf_addr_size))

     return 0;

-  if (_bfd_mul_overflow (idx, unit->offset_size, &offset))

+  if (_bfd_mul_overflow (idx, unit->addr_size, &offset))

     return 0;

   offset += unit->dwarf_addr_offset;

@@ -1380,9 +1380,9 @@ read_indexed_address (uint64_t idx, struct comp_unit *unit)

   info_ptr = file->dwarf_addr_buffer + offset;

-  if (unit->offset_size == 4)

+  if (unit->addr_size == 4)

     return bfd_get_32 (unit->abfd, info_ptr);

-  else if (unit->offset_size == 8)

+  else if (unit->addr_size == 8)

     return bfd_get_64 (unit->abfd, info_ptr);

   else

     return 0;

@@ -1731,6 +1731,7 @@ struct line_info_table

   unsigned int              num_files;

   unsigned int              num_dirs;

   unsigned int              num_sequences;

+  unsigned int             version;

   char *                        comp_dir;

   char **                       dirs;

   struct fileinfo* files;

@@ -1951,6 +1952,8 @@ concat_filename (struct line_info_table *table, unsigned int file)

{

   char *filename;

+  if (table->version >= 5)

+    file = file + 1;

   if (table == NULL || file - 1 >= table->num_files)

     {

       /* FILE == 0 means unknown.  */

@@ -2579,10 +2582,8 @@ read_formatted_entries (struct comp_unit *unit, bfd_byte **bufp,

               }

           }

-      /* Skip the first "zero entry", which is the compilation dir/file.  */

-      if (datai != 0)

-           if (!callback (table, fe.name, fe.dir, fe.time, fe.size))

-             return false;

+      if (!callback (table, fe.name, fe.dir, fe.time, fe.size))

+          return false;

     }

   *bufp = buf;

@@ -2747,6 +2748,7 @@ decode_line_info (struct comp_unit *unit)

   table->sequences = NULL;

   table->lcl_head = NULL;

+  table->version = lh.version;

   if (lh.version >= 5)

     {

@@ -2789,13 +2791,16 @@ decode_line_info (struct comp_unit *unit)

       /* State machine registers.  */

       bfd_vma address = 0;

       unsigned char op_index = 0;

-      char * filename = table->num_files ? concat_filename (table, 1) : NULL;

+      char *filename;

+      int index = table->version >= 5 ? 0 : 1;

       unsigned int line = 1;

       unsigned int column = 0;

       unsigned int discriminator = 0;

       int is_stmt = lh.default_is_stmt;

       int end_sequence = 0;

       unsigned int dir, xtime, size;

+

+      filename = table->num_files ? concat_filename (table, index) : NULL;

       /* eraxxon@alumni.rice.edu<mailto:eraxxon@alumni.rice.edu>: Against the DWARF2 specs, some

            compilers generate address sequences that are wildly out of

            order using DW_LNE_set_address (e.g. Intel C++ 6.0 compiler

diff --git a/binutils/testsuite/binutils-all/addr2line.exp b/binutils/testsuite/binutils-all/addr2line.exp

new file mode 100644

index 00000000000..153e83c2ace

--- /dev/null

+++ b/binutils/testsuite/binutils-all/addr2line.exp

@@ -0,0 +1,59 @@

+#   Copyright (C) 2018-2022 Free Software Foundation, Inc.

+

+# This program is free software; you can redistribute it and/or modify

+# it under the terms of the GNU General Public License as published by

+# the Free Software Foundation; either version 3 of the License, or

+# (at your option) any later version.

+#

+# This program is distributed in the hope that it will be useful,

+# but WITHOUT ANY WARRANTY; without even the implied warranty of

+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

+# GNU General Public License for more details.

+#

+# You should have received a copy of the GNU General Public License

+# along with this program; if not, write to the Free Software

+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.

+

+    global $NM

+    global $ADDR2LINE

+

+    set testname "addr2line"

+    if { [target_compile $srcdir/$subdir/testprog.c tmpdir/testprog executable debug] != "" } {

+        verbose "Unable to compile test file."

+        untested "addr2line"

+        return

+    }

+

+    #testcase for default option.

+    set output [binutils_run $NM "tmpdir/testprog"]

+    regexp -line {^[0-9]+\s+[A-Z]\s+main} $output contents

+    set list [regexp -inline -all -- {\S+} $contents]

+

+    set got [binutils_run $ADDR2LINE "-e tmpdir/testprog  [lindex $list 0]"]

+    set want "$srcdir/$subdir/testprog.c:\[0-9\]+"

+    if ![regexp $want $got] then {

+        fail "$testname $got\n"

+    } else {

+        pass "$testname"

+    }

+

+    #testcase for -f option.

+    regexp -line {^[0-9]+\s+[A-Z]\s+fn} $output contents

+    set list [regexp -inline -all -- {\S+} $contents]

+

+    set got [binutils_run $ADDR2LINE "-f -e tmpdir/testprog  [lindex $list 0]"]

+    set want "fn\n$srcdir/$subdir/testprog.c:\[0-9\]+"

+    if ![regexp $want $got] then {

+        fail "$testname -f option $got\n"

+    } else {

+        pass "$testname -f option"

+    }

+

+    #testcase for -s option.

+    set got [binutils_run $ADDR2LINE "-s -e tmpdir/testprog  [lindex $list 0]"]

+    set want "testprog.c:\[0-9\]+"

+    if ![regexp $want $got] then {

+        fail "$testname -s option $got\n"

+    } else {

+        pass "$testname -s option"

+    }

diff --git a/binutils/testsuite/config/default.exp b/binutils/testsuite/config/default.exp

index c654bd4081c..7192c929a83 100644

--- a/binutils/testsuite/config/default.exp

+++ b/binutils/testsuite/config/default.exp

@@ -40,6 +40,12 @@ if ![info exists NM] then {

if ![info exists NMFLAGS] then {

     set NMFLAGS ""

}

+if ![info exists ADDR2LINE] then {

+    set ADDR2LINE [findfile $base_dir/addr2line $base_dir/addr2line [transform nm]]

+}

+if ![info exists ADDR2LINEFLAGS] then {

+    set ADDR2LINEFLAGS ""

+}

if ![info exists SIZE] then {

     set SIZE [findfile $base_dir/size]

}

--

2.17.1




[-- Attachment #2: 0001-bfd-Fix-issues-with-files-in-debug_line-table-with-d.docx --]
[-- Type: application/vnd.openxmlformats-officedocument.wordprocessingml.document, Size: 17590 bytes --]