Re: [PATCH] Disassemble over end of line table sequence.

["Andrew Burgess" <aburgess at broadcom dot com>]

On 14/01/2011 16:09, Joel Brobecker wrote:
> Andrew,
> 
> Note that ever line in the CL entry needs to be aligned - I can't remember
> if it was you already told about this. I may have, this patch is from
> 4 weeks ago...

Sorry about that, I made this patch before I was told about this, I should have refreshed the patch before ping-ing it.

> 
>> gdb/testsuite/
>>
>> 2010-12-10  Andrew Burgess<aburgess@broadcom.com>
>>
>>            * gdb.disasm/disasm-end-cu-1.c, gdb.disasm/disasm-end-cu-2.c,
>>              gdb.disasm/disasm-end-cu.exp: New test for disassembling
>>              over the boundary between two compilation units.
> 
> I think you got mislead a bit by the directory name when choosing
> the directory where to put your testcase. This directory contains
> testcase that use files with assembly code, and thus limited to
> selected architectures.  Yours uses plain C files, which can be compiled
> on all hosts.  So, let's move your testcase to gdb.base.

Moved.

>> +  /* Work with end of sequence markers
>> +     where the line number is set to 0 */
> 
> The lines are too short - please join them into 1 (actually, the guideline
> is 70 characters, and that means it doesn't fit, but let's make the first
> line a little longer, which is a more natural length). Also, the GNU Coding
> Standards (GCS) require that we put a period at the end of every sentence,
> and periods are followed by 2 spaces. Thus:
> 
>    /* Work with end of sequence markers where the line number is set
>       to 0. */

Comment now says something a little better, and is wrapped correctly.
 
>> +  if ( mle1->line == 0 || mle2->line == 0 )
> 
> Formatting, no space after '(' and before ')':
> 
>    if (mle1->line == 0 || mle2->line == 0)
> 
>> +      if ( val == 0 )
> 
> Same here.

Fixed.

> 
>> +++ b/gdb/testsuite/gdb.disasm/disasm-end-cu-1.c
>> @@ -0,0 +1,14 @@
>> +#include<stdio.h>
> 
> This file needs a copyright header.
> 
>> +++ b/gdb/testsuite/gdb.disasm/disasm-end-cu-2.c
>> @@ -0,0 +1,13 @@
>> +#include<stdio.h>
> 
> Same here.
> 
>> +++ b/gdb/testsuite/gdb.disasm/disasm-end-cu.exp
>> @@ -0,0 +1,70 @@
>> +# Copyright 2010 Free Software Foundation, Inc.
> 
> Can you add 2011? (And a `(C)' - this is not strictly mandated by the FSF,
> but the script I'm planning on using to perform yearly updates requires it).

All fixed.

> 
>> +# This test can only be run on targets which support DWARF-2 and use gas.
>> +# For now pick a sampling of likely targets.
>> +if {![istarget *-*-linux*]
>> +&&  ![istarget *-*-gnu*]
>> +&&  ![istarget *-*-elf*]
>> +&&  ![istarget *-*-openbsd*]
>> +&&  ![istarget arm-*-eabi*]
>> +&&  ![istarget powerpc-*-eabi*]} {
>> +    return 0
> 
> This is not necessary in your case.
> 
>> +send_gdb "disassemble /m ${main_addr},${dummy_3_addr}\n"
>> +gdb_expect {
>> +    -re "Dump of assembler code from ${main_addr} to
> 
> We should not be using gdb_send/gdb_expect.  Can you use gdb_test_multiple
> instead?
> 
>> +gdb_stop_suppressing_tests;
> 
> You can remove this line.

... and fixed.

Thanks for the review.

Andrew


gdb/ChangeLog

2011-01-18  Andrew Burgess  <aburgess@broadcom.com>

	* disasm.c (compare_lines): Handle the end of sequence markers
	within the line table to better support disassembling over
	compilation unit boundaries.

gdb/testsuite/ChangeLog

2011-01-18  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.base/disasm-end-cu-1.c, gdb.base/disasm-end-cu-2.c,
	gdb.base/disasm-end-cu.exp: New test for disassembling over the
	boundary between two compilation units.

diff --git a/gdb/disasm.c b/gdb/disasm.c
index c51f0bf..f2428b5 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -77,12 +77,22 @@ compare_lines (const void *mle1p, const void *mle2p)
   mle1 = (struct dis_line_entry *) mle1p;
   mle2 = (struct dis_line_entry *) mle2p;
 
-  val = mle1->line - mle2->line;
-
-  if (val != 0)
-    return val;
+  /* End of sequence markers have a line number of 0 but don't want to
+     be sorted to the head of the list, instead sort by PC.  */
+  if (mle1->line == 0 || mle2->line == 0)
+    {
+      val = mle1->start_pc - mle2->start_pc;
+      if (val == 0)
+        val = mle1->line - mle2->line;
+    }
+  else
+    {
+      val = mle1->line - mle2->line;
+      if (val == 0)
+        val = mle1->start_pc - mle2->start_pc;
+    }
 
-  return mle1->start_pc - mle2->start_pc;
+  return val;
 }
 
 static int
diff --git a/gdb/testsuite/gdb.base/disasm-end-cu-1.c b/gdb/testsuite/gdb.base/disasm-end-cu-1.c
new file mode 100644
index 0000000..ea7b1a3
--- /dev/null
+++ b/gdb/testsuite/gdb.base/disasm-end-cu-1.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2010, 2011 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, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+
+void
+dummy_1 ()
+{
+  printf ("In dummy_1 ()\n");
+}
+
+int
+main ()
+{
+  printf ("Hello World!\n");
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/disasm-end-cu-2.c b/gdb/testsuite/gdb.base/disasm-end-cu-2.c
new file mode 100644
index 0000000..f4b6152
--- /dev/null
+++ b/gdb/testsuite/gdb.base/disasm-end-cu-2.c
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2010, 2011 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, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+
+void
+dummy_2 ()
+{
+  printf ("In dummy_2 ()\n");
+}
+
+void
+dummy_3 ()
+{
+  printf ("In dummy_3 ()\n");
+}
diff --git a/gdb/testsuite/gdb.base/disasm-end-cu.exp b/gdb/testsuite/gdb.base/disasm-end-cu.exp
new file mode 100644
index 0000000..4a145f8
--- /dev/null
+++ b/gdb/testsuite/gdb.base/disasm-end-cu.exp
@@ -0,0 +1,52 @@
+# Copyright (C) 2010, 2011 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, see <http://www.gnu.org/licenses/>.
+
+# This test tries to disassemble over the boundary between two compilation
+# units displaying source lines. This checks that the disassemble routine
+# can handle our use of line number 0 to mark the end of sequence.
+
+if { [prepare_for_testing disasm-end-cu.exp "disasm-end-cu" {disasm-end-cu-1.c disasm-end-cu-2.c} {debug}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+set main_addr [get_hexadecimal_valueof "&main" "0"]
+set dummy_3_addr [get_hexadecimal_valueof "&dummy_3" "0"]
+
+if {$main_addr == 0 || $dummy_3_addr == 0 || $dummy_3_addr <= $main_addr} {
+    fail "Unable to extract required addresses, or addresses out of order"
+    return -1
+}
+
+gdb_test_multiple "disassemble /m ${main_addr},${dummy_3_addr}" "Disassemble address range with source" {
+    -re "Dump of assembler code from ${main_addr} to ${dummy_3_addr}:\r\nEnd of assembler dump\." {
+        fail "No output from the disassemble command"
+    }
+    -re "Line number 0 out of range;.* has $decimal lines\." {
+        fail "The disassemble command failed"
+    }
+    -re "Dump of assembler code from ${main_addr} to ${dummy_3_addr}:\r\n.*main.*End of assembler dump\." {
+        pass "disassemble command returned some output"
+    }
+    -re ".*$gdb_prompt $" {
+        fail "Unexpected output from disassemble command"
+    }
+    timeout {
+        fail "(timeout) waiting for output of disassemble command"
+    }
+}