[readelf] Fix end_seq entry in -wL

Message ID 647bc2fb-2755-48e6-e8cb-1e451dc6f3de@suse.de
State New
Headers show
Series
  • [readelf] Fix end_seq entry in -wL
Related show

Commit Message

Tom de Vries July 10, 2020, 7:48 a.m.
[ 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

Comments

David Faust via Binutils July 10, 2020, 10:29 a.m. | #1
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
Tom de Vries July 10, 2020, 11:29 a.m. | #2
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

Patch

[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  <tdevries@suse.de>

	* 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                              - .*