[RFC][readelf] Fix end_seq entry in -wL

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFC][readelf] Fix end_seq entry in -wL

Tom de Vries
Hi,

this patch is missing updates for the test-suite, without those we have:
...
FAIL: objdump -WL
FAIL: readelf -wiaoRlL dw5
...
but I'd rather update those once the new format for the end_seq lines
has been agreed upon.

Any comments?

Thanks,
- Tom

[readelf] Fix end_seq entry in -wL

For a hello world with debug info, we have with readelf -wl:
...
  [0x000002a3]  Extended opcode 2: set Address to 0x400598
  [0x000002ae]  Advance Line by 43 to 44
  [0x000002b0]  Copy
  [0x000002b1]  Special opcode 62: advance Address by 4 to 0x40059c \
    and Line by 1 to 45
  [0x000002b2]  Advance PC by 1 to 0x40059d
  [0x000002b4]  Extended opcode 1: End of Sequence
...
and with readelf -wL (dropping the File name part):
...
Line number    Starting address    View    Stmt
         44            0x400598               x
         45            0x40059c               x
         45            0x40059d               x
...

The "End of Sequence" is DW_LNE_end_sequence, which:
...
sets the end_sequence register of the state machine to “true” and appends a
row to the matrix using the current values of the state-machine registers.
...
where the end_sequence register has the semantics:
...
A boolean indicating that the current address is that of the first byte after
the end of a sequence of target machine instructions. end_sequence
terminates a sequence of lines; therefore other information in the same
row is not meaningful.
...

So, the last entry in the -wL bit shows us a line number which is not
meaningful, which is confusing.

Fix this by instead emitting:
...
Line number    Starting address    View    Stmt
         44            0x400598               x
         45            0x40059c               x
          -            0x40059d
...

binutils/ChangeLog:

2020-07-03  Tom de Vries  <[hidden email]>

        * dwarf.c (display_debug_lines_decoded): Don't emit meaningless
        information in the end_sequence row.

---
 binutils/dwarf.c | 62 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 16 deletions(-)

diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index bdabd9c4ba..189f0f5f71 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -4991,34 +4991,64 @@ display_debug_lines_decoded (struct dwarf_section *  section,
       if (!do_wide || (fileNameLength <= MAX_FILENAME_LENGTH))
  {
   if (linfo.li_max_ops_per_insn == 1)
-    printf ("%-35s  %11d  %#18" DWARF_VMA_FMT "x",
-    newFileName, state_machine_regs.line,
-    state_machine_regs.address);
+    {
+      if (xop == -DW_LNE_end_sequence)
+ printf ("%-35s  %11s  %#18" DWARF_VMA_FMT "x",
+ newFileName, "-",
+ state_machine_regs.address);
+      else
+ printf ("%-35s  %11d  %#18" DWARF_VMA_FMT "x",
+ newFileName, state_machine_regs.line,
+ state_machine_regs.address);
+    }
   else
-    printf ("%-35s  %11d  %#18" DWARF_VMA_FMT "x[%d]",
-    newFileName, state_machine_regs.line,
-    state_machine_regs.address,
-    state_machine_regs.op_index);
+    {
+      if (xop == -DW_LNE_end_sequence)
+ printf ("%-35s  %11s  %#18" DWARF_VMA_FMT "x[%d]",
+ newFileName, "-",
+ state_machine_regs.address,
+ state_machine_regs.op_index);
+      else
+ printf ("%-35s  %11d  %#18" DWARF_VMA_FMT "x[%d]",
+ newFileName, state_machine_regs.line,
+ state_machine_regs.address,
+ state_machine_regs.op_index);
+    }
  }
       else
  {
   if (linfo.li_max_ops_per_insn == 1)
-    printf ("%s  %11d  %#18" DWARF_VMA_FMT "x",
-    newFileName, state_machine_regs.line,
-    state_machine_regs.address);
+    {
+      if (xop == -DW_LNE_end_sequence)
+ printf ("%s  %11s  %#18" DWARF_VMA_FMT "x",
+ newFileName, "-",
+ state_machine_regs.address);
+      else
+ printf ("%s  %11d  %#18" DWARF_VMA_FMT "x",
+ newFileName, state_machine_regs.line,
+ state_machine_regs.address);
+    }
   else
-    printf ("%s  %11d  %#18" DWARF_VMA_FMT "x[%d]",
-    newFileName, state_machine_regs.line,
-    state_machine_regs.address,
-    state_machine_regs.op_index);
+    {
+      if (xop == -DW_LNE_end_sequence)
+ printf ("%s  %11s  %#18" DWARF_VMA_FMT "x[%d]",
+ newFileName, "-",
+ state_machine_regs.address,
+ state_machine_regs.op_index);
+      else
+ printf ("%s  %11d  %#18" DWARF_VMA_FMT "x[%d]",
+ newFileName, state_machine_regs.line,
+ state_machine_regs.address,
+ state_machine_regs.op_index);
+    }
  }
 
-      if (state_machine_regs.view)
+      if (state_machine_regs.view && (xop != -DW_LNE_end_sequence))
  printf ("  %6u", state_machine_regs.view);
       else
  printf ("        ");
 
-      if (state_machine_regs.is_stmt)
+      if (state_machine_regs.is_stmt && (xop != -DW_LNE_end_sequence))
  printf ("       x");
 
       putchar ('\n');
Reply | Threaded
Open this post in threaded view
|

Re: [RFC][readelf] Fix end_seq entry in -wL

Sourceware - binutils list mailing list
Hi Tom,

> Any comments?

The new patch looks fine to me, but I think that it would be useful
to include a comment in the code itself explaining why the line number
is not displayed.

Cheers
  Nick


Reply | Threaded
Open this post in threaded view
|

[PATCH][readelf] Fix end_seq entry in -wL

Tom de Vries
[ was: [RFC][readelf] Fix end_seq entry in -wL ]

On 7/9/20 3:31 PM, Nick Clifton wrote:
> Hi Tom,
>
>> Any comments?
>
> The new patch looks fine to me, but I think that it would be useful
> to include a comment in the code itself explaining why the line number
> is not displayed.

Thanks for the review.

I've:
- added the comment
- updated the two failing test-cases mentioned before
- fixed the fallout from that (trailing whitespace after the starting
  address)

OK for trunk?

Thanks,
- Tom

[readelf] Fix end_seq entry in -wL

For a hello world with debug info, we have with readelf -wl:
...
  [0x000002a3]  Extended opcode 2: set Address to 0x400598
  [0x000002ae]  Advance Line by 43 to 44
  [0x000002b0]  Copy
  [0x000002b1]  Special opcode 62: advance Address by 4 to 0x40059c \
    and Line by 1 to 45
  [0x000002b2]  Advance PC by 1 to 0x40059d
  [0x000002b4]  Extended opcode 1: End of Sequence
...
and with readelf -wL (dropping the File name part):
...
Line number    Starting address    View    Stmt
         44            0x400598               x
         45            0x40059c               x
         45            0x40059d               x
...

The "End of Sequence" is DW_LNE_end_sequence, which:
...
sets the end_sequence register of the state machine to “true” and appends a
row to the matrix using the current values of the state-machine registers.
...
where the end_sequence register has the semantics:
...
A boolean indicating that the current address is that of the first byte after
the end of a sequence of target machine instructions. end_sequence
terminates a sequence of lines; therefore other information in the same
row is not meaningful.
...

So, the last entry in the -wL bit shows us a line number which is not
meaningful, which is confusing.

Fix this by instead emitting:
...
Line number    Starting address    View    Stmt
         44            0x400598               x
         45            0x40059c               x
          -            0x40059d
...

Tested on x86_64-linux.

binutils/ChangeLog:

2020-07-03  Tom de Vries  <[hidden email]>

        * dwarf.c (display_debug_lines_decoded): Don't emit meaningless
        information in the end_sequence row.
        * testsuite/binutils-all/dw5.W: Update.
        * testsuite/binutils-all/objdump.WL: Update.

---
 binutils/dwarf.c                           | 76 ++++++++++++++++++++++--------
 binutils/testsuite/binutils-all/dw5.W      |  4 +-
 binutils/testsuite/binutils-all/objdump.WL |  2 +-
 3 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index bdabd9c4ba..42afe59320 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -4988,38 +4988,74 @@ display_debug_lines_decoded (struct dwarf_section *  section,
   strncpy (newFileName, fileName, fileNameLength + 1);
  }
 
+      /* A row with end_seq set to true has a meaning address, but the
+ other information in the same row is not meaningful.  In such
+ a row, print line as "-", and don't print view/is_stmt.  */
       if (!do_wide || (fileNameLength <= MAX_FILENAME_LENGTH))
  {
   if (linfo.li_max_ops_per_insn == 1)
-    printf ("%-35s  %11d  %#18" DWARF_VMA_FMT "x",
-    newFileName, state_machine_regs.line,
-    state_machine_regs.address);
+    {
+      if (xop == -DW_LNE_end_sequence)
+ printf ("%-35s  %11s  %#18" DWARF_VMA_FMT "x",
+ newFileName, "-",
+ state_machine_regs.address);
+      else
+ printf ("%-35s  %11d  %#18" DWARF_VMA_FMT "x",
+ newFileName, state_machine_regs.line,
+ state_machine_regs.address);
+    }
   else
-    printf ("%-35s  %11d  %#18" DWARF_VMA_FMT "x[%d]",
-    newFileName, state_machine_regs.line,
-    state_machine_regs.address,
-    state_machine_regs.op_index);
+    {
+      if (xop == -DW_LNE_end_sequence)
+ printf ("%-35s  %11s  %#18" DWARF_VMA_FMT "x[%d]",
+ newFileName, "-",
+ state_machine_regs.address,
+ state_machine_regs.op_index);
+      else
+ printf ("%-35s  %11d  %#18" DWARF_VMA_FMT "x[%d]",
+ newFileName, state_machine_regs.line,
+ state_machine_regs.address,
+ state_machine_regs.op_index);
+    }
  }
       else
  {
   if (linfo.li_max_ops_per_insn == 1)
-    printf ("%s  %11d  %#18" DWARF_VMA_FMT "x",
-    newFileName, state_machine_regs.line,
-    state_machine_regs.address);
+    {
+      if (xop == -DW_LNE_end_sequence)
+ printf ("%s  %11s  %#18" DWARF_VMA_FMT "x",
+ newFileName, "-",
+ state_machine_regs.address);
+      else
+ printf ("%s  %11d  %#18" DWARF_VMA_FMT "x",
+ newFileName, state_machine_regs.line,
+ state_machine_regs.address);
+    }
   else
-    printf ("%s  %11d  %#18" DWARF_VMA_FMT "x[%d]",
-    newFileName, state_machine_regs.line,
-    state_machine_regs.address,
-    state_machine_regs.op_index);
+    {
+      if (xop == -DW_LNE_end_sequence)
+ printf ("%s  %11s  %#18" DWARF_VMA_FMT "x[%d]",
+ newFileName, "-",
+ state_machine_regs.address,
+ state_machine_regs.op_index);
+      else
+ printf ("%s  %11d  %#18" DWARF_VMA_FMT "x[%d]",
+ newFileName, state_machine_regs.line,
+ state_machine_regs.address,
+ state_machine_regs.op_index);
+    }
  }
 
-      if (state_machine_regs.view)
- printf ("  %6u", state_machine_regs.view);
-      else
- printf ("        ");
+      if (xop != -DW_LNE_end_sequence)
+ {
+  if (state_machine_regs.view)
+    printf ("  %6u", state_machine_regs.view);
+  else
+    printf ("        ");
 
-      if (state_machine_regs.is_stmt)
- printf ("       x");
+  if (state_machine_regs.is_stmt)
+    printf ("       x");
+ }
 
       putchar ('\n');
       state_machine_regs.view++;
diff --git a/binutils/testsuite/binutils-all/dw5.W b/binutils/testsuite/binutils-all/dw5.W
index 2eccb03c5a..ebeda6575d 100644
--- a/binutils/testsuite/binutils-all/dw5.W
+++ b/binutils/testsuite/binutils-all/dw5.W
@@ -350,10 +350,10 @@ CU: ./main.c:
 File name                            Line number    Starting address    View    Stmt
 main.c                                         6              0x1234               x
 main.c                                         6             0x12346               x
-main.c                                         6              0x1234               x
+main.c                                         -              0x1234
 
 main.c                                         5              0x1234               x
 main.c                                         5              0x1234               x
-main.c                                         5              0x1234               x
+main.c                                         -              0x1234
 
 
diff --git a/binutils/testsuite/binutils-all/objdump.WL b/binutils/testsuite/binutils-all/objdump.WL
index 954fb3e67f..e16d5bad43 100644
--- a/binutils/testsuite/binutils-all/objdump.WL
+++ b/binutils/testsuite/binutils-all/objdump.WL
@@ -12,5 +12,5 @@ file1\.c                                        1 .*
 
 \./dw2-decodedline\.c:\[\+\+\]
 dw2-decodedline\.c                              2 .*
-dw2-decodedline\.c                              2 .*
+dw2-decodedline\.c                              - .*
 
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][readelf] Fix end_seq entry in -wL

Sourceware - binutils list mailing list
Hi Tom,

> I've:
> - added the comment
> - updated the two failing test-cases mentioned before
> - fixed the fallout from that (trailing whitespace after the starting
>   address)
>
> OK for trunk?

Yes - although you missed one set of tests - the assembler also includes
some checks of DWARF line number table generation, and these needed to be
updated too.  I went ahead and fixed them myself, since it was easy.

I also tweaked the comment slightly as I think that there was a small typo
in the version you had.  I hope that you do not mind.  

I have checked in your patch with the above mentioned changes.

Cheers
  Nick


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][readelf] Fix end_seq entry in -wL

Tom de Vries
On 7/10/20 12:29 PM, Nick Clifton wrote:

> Hi Tom,
>
>> I've:
>> - added the comment
>> - updated the two failing test-cases mentioned before
>> - fixed the fallout from that (trailing whitespace after the starting
>>   address)
>>
>> OK for trunk?
>
> Yes - although you missed one set of tests - the assembler also includes
> some checks of DWARF line number table generation, and these needed to be
> updated too.  I went ahead and fixed them myself, since it was easy.
>

Ah, I didn't realize that, thanks.

> I also tweaked the comment slightly as I think that there was a small typo
> in the version you had.  I hope that you do not mind.  
>

Not at all :)

> I have checked in your patch with the above mentioned changes.

Thanks,
- Tom