[PATCH 0/2] disassembly over compilation unit boundaries

[PATCH 0/2] disassembly over compilation unit boundaries

[Andrew Burgess]

Patch #2 in this series attempts to improve gdb's ability to
disassembly over compilation unit boundaries.  The thinking is that if
a user tries to disassemble the address range from START to END like
this:

    (gdb) disassemble /m START,END

Then even though they asked for disassembly with source code, it is
better to provide raw assembly if there's no debug information
available, and so raw assembly is the best they'll ever get.

At the same time I address the issue where the address range crosses
from one compilation unit with debug information into a second
compilation unit also with debug information.  Currently gdb will
again stop at the boundary of the first compilation unit, after this
patch the disassembly will continue.

Patch #1 is a small clean up in the disassembly code.

Thanks,

Andrew


Andrew Burgess (2):
  gdb: Move common MI code to outer function.
  gdb: Ensure disassembler covers requested address range.

 gdb/ChangeLog                                      |  18 +++
 gdb/disasm.c                                       | 141 +++++++++++++++------
 gdb/testsuite/ChangeLog                            |  11 ++
 gdb/testsuite/gdb.base/disasm-end-cu.exp           |  49 -------
 .../{disasm-end-cu-1.c => disasm-multi-cu-1.c}     |  10 +-
 .../{disasm-end-cu-2.c => disasm-multi-cu-2.c}     |  14 +-
 gdb/testsuite/gdb.base/disasm-multi-cu-3.c         |  30 +++++
 gdb/testsuite/gdb.base/disasm-multi-cu.exp         | 111 ++++++++++++++++
 gdb/testsuite/gdb.base/disasm-multi-cu.h           |  25 ++++
 9 files changed, 304 insertions(+), 105 deletions(-)
 delete mode 100644 gdb/testsuite/gdb.base/disasm-end-cu.exp
 rename gdb/testsuite/gdb.base/{disasm-end-cu-1.c => disasm-multi-cu-1.c} (86%)
 rename gdb/testsuite/gdb.base/{disasm-end-cu-2.c => disasm-multi-cu-2.c} (80%)
 create mode 100644 gdb/testsuite/gdb.base/disasm-multi-cu-3.c
 create mode 100644 gdb/testsuite/gdb.base/disasm-multi-cu.exp
 create mode 100644 gdb/testsuite/gdb.base/disasm-multi-cu.h

-- 
2.5.1

[PATCH 2/2] gdb: Ensure disassembler covers requested address range.

[Andrew Burgess]

If gdb is asked to disassemble a particular address range, but that
address range spans the end of a symbol table, then currently gdb will
exit the disassembler early.

This commit makes the disassembler handle passing over the boundary
between different symbol tables, and even over regions where there is no
symbol table.

The only reason that the disassembler will now not disassemble a
complete address range as asked is if the inferior memory can't be
read within that region.

The existing test for disassembling over compilation unit boundary is
not sufficient, in fact the behaviour has regressed since the test was
first added in 2011 (commit 9011945e46bf8b) and the test failed to
highlight this regression.

This commit removes the old test in this area, and adds new, more
comprehensive tests that cover the following features:

 1. Disassembly over the end of a compilation unit and into the next
    compilation unit should work.

 2. When different compilation units do, or do not, have debug
    information, we should still get some disassembly output, even when
    asking for disassembly with source code.

 3. When asking for disassembly with source code, we should, where
    possible provide that source code, even when earlier compilation
    units within the region being disassembled don't have debug
    information.

gdb/ChangeLog:

	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
	count, and take extra parameter.  Fill in final_pc parameter.
	(do_mixed_source_and_assembly): Likewise.
	(do_assembly_only): Likewise, also return from function when we've
	displayed enough instructions.
	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
	raw instructions as required.

gdb/testsuite/ChangeLog:

	* gdb.base/disasm-end-cu-1.c: Deleted.
	* gdb.base/disasm-end-cu-2.c: Deleted.
	* gdb.base/disasm-end-cu.exp: Deleted.
	* gdb.base/disasm-multi-cu-1.c: New file.
	* gdb.base/disasm-multi-cu-2.c: New file.
	* gdb.base/disasm-multi-cu-3.c: New file.
	* gdb.base/disasm-multi-cu.exp: New file.
	* gdb.base/disasm-multi-cu.h: New file.
---
 gdb/ChangeLog                                      |  10 ++
 gdb/disasm.c                                       | 127 ++++++++++++++++-----
 gdb/testsuite/ChangeLog                            |  11 ++
 gdb/testsuite/gdb.base/disasm-end-cu.exp           |  49 --------
 .../{disasm-end-cu-1.c => disasm-multi-cu-1.c}     |  10 +-
 .../{disasm-end-cu-2.c => disasm-multi-cu-2.c}     |  14 +--
 gdb/testsuite/gdb.base/disasm-multi-cu-3.c         |  30 +++++
 gdb/testsuite/gdb.base/disasm-multi-cu.exp         | 111 ++++++++++++++++++
 gdb/testsuite/gdb.base/disasm-multi-cu.h           |  25 ++++
 9 files changed, 294 insertions(+), 93 deletions(-)
 delete mode 100644 gdb/testsuite/gdb.base/disasm-end-cu.exp
 rename gdb/testsuite/gdb.base/{disasm-end-cu-1.c => disasm-multi-cu-1.c} (86%)
 rename gdb/testsuite/gdb.base/{disasm-end-cu-2.c => disasm-multi-cu-2.c} (80%)
 create mode 100644 gdb/testsuite/gdb.base/disasm-multi-cu-3.c
 create mode 100644 gdb/testsuite/gdb.base/disasm-multi-cu.exp
 create mode 100644 gdb/testsuite/gdb.base/disasm-multi-cu.h

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 463b1b9..7ff35a2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,15 @@
 2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
+	count, and take extra parameter.  Fill in final_pc parameter.
+	(do_mixed_source_and_assembly): Likewise.
+	(do_assembly_only): Likewise, also return from function when we've
+	displayed enough instructions.
+	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
+	raw instructions as required.
+
+2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
 	asm_insns list.
 	(do_mixed_source_and_assembly): Likewise.
diff --git a/gdb/disasm.c b/gdb/disasm.c
index 3032090..9199eb2 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -274,12 +274,12 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
 
    N.B. This view is deprecated.  */
 
-static void
+static int
 do_mixed_source_and_assembly_deprecated
   (struct gdbarch *gdbarch, struct ui_out *uiout,
    struct disassemble_info *di, struct symtab *symtab,
    CORE_ADDR low, CORE_ADDR high,
-   int how_many, int flags, struct ui_file *stb)
+   int how_many, int flags, struct ui_file *stb, CORE_ADDR *final_pc)
 {
   int newlines = 0;
   int nlines;
@@ -293,6 +293,7 @@ do_mixed_source_and_assembly_deprecated
   enum print_source_lines_flags psl_flags = 0;
   struct cleanup *ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
   struct cleanup *ui_out_list_chain = make_cleanup (null_cleanup, 0);
+  CORE_ADDR next_pc = low;
 
   gdb_assert (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL);
 
@@ -411,7 +412,7 @@ do_mixed_source_and_assembly_deprecated
 
       num_displayed += dump_insns (gdbarch, uiout, di,
 				   mle[i].start_pc, mle[i].end_pc,
-				   how_many, flags, stb, NULL);
+				   how_many, flags, stb, &next_pc);
 
       /* When we've reached the end of the mle array, or we've seen the last
          assembly range for this source line, close out the list/tuple.  */
@@ -426,6 +427,11 @@ do_mixed_source_and_assembly_deprecated
       if (how_many >= 0 && num_displayed >= how_many)
 	break;
     }
+
+  if (final_pc != NULL)
+    *final_pc = next_pc;
+
+  return num_displayed;
 }
 
 /* The idea here is to present a source-O-centric view of a
@@ -433,12 +439,13 @@ do_mixed_source_and_assembly_deprecated
    in source order, with (possibly) out of order assembly
    immediately following.  */
 
-static void
+static int
 do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 			      struct disassemble_info *di,
 			      struct symtab *main_symtab,
 			      CORE_ADDR low, CORE_ADDR high,
-			      int how_many, int flags, struct ui_file *stb)
+			      int how_many, int flags, struct ui_file *stb,
+			      CORE_ADDR *final_pc)
 {
   int newlines = 0;
   const struct linetable_entry *le, *first_le;
@@ -455,6 +462,7 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
   struct symtab *last_symtab;
   int last_line;
   htab_t dis_line_table;
+  CORE_ADDR end_pc = low;
 
   gdb_assert (main_symtab != NULL && SYMTAB_LINETABLE (main_symtab) != NULL);
 
@@ -537,7 +545,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
     {
       struct linetable_entry *le = NULL;
       struct symtab_and_line sal;
-      CORE_ADDR end_pc;
       int start_preceding_line_to_display = 0;
       int end_preceding_line_to_display = 0;
       int new_source_line = 0;
@@ -676,18 +683,39 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
     }
 
   do_cleanups (cleanups);
+
+  if (final_pc != NULL)
+    *final_pc = end_pc;
+
+  return num_displayed;
 }
 
-static void
+static int
 do_assembly_only (struct gdbarch *gdbarch, struct ui_out *uiout,
 		  struct disassemble_info * di,
 		  CORE_ADDR low, CORE_ADDR high,
-		  int how_many, int flags, struct ui_file *stb)
+		  int how_many, int flags, struct ui_file *stb,
+		  CORE_ADDR *final_pc)
 {
-  int num_displayed = 0;
+  CORE_ADDR npc;
+  int total = 0;
+
+  do
+    {
+      total += dump_insns (gdbarch, uiout, di, low, high,
+			   how_many, flags, stb, &npc);
+      if (npc >= high || total >= how_many)
+        break;
+      if (low == npc)
+	low = npc + 1;
+      else
+	low = npc;
+    } while (1);
+
+  if (final_pc != NULL)
+    *final_pc = npc;
 
-  num_displayed = dump_insns (gdbarch, uiout, di, low, high, how_many,
-                              flags, stb, NULL);
+  return total;
 }
 
 /* Initialize the disassemble info struct ready for the specified
@@ -740,31 +768,72 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
   struct ui_file *stb = mem_fileopen ();
   struct cleanup *cleanups = make_cleanup_ui_file_delete (stb);
   struct disassemble_info di = gdb_disassemble_info (gdbarch, stb);
-  struct symtab *symtab;
-  struct linetable_entry *le = NULL;
-  int nlines = -1;
-
-  /* Assume symtab is valid for whole PC range.  */
-  symtab = find_pc_line_symtab (low);
-
-  if (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL)
-    nlines = SYMTAB_LINETABLE (symtab)->nitems;
 
   /* This cleanup is inner to CLEANUPS, so we don't need a separate
      variable to track it.  */
   (void) make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
 
-  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
-      || nlines <= 0)
-    do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
+  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE)))
+    (void) do_assembly_only (gdbarch, uiout, &di, low, high,
+			     how_many, flags, stb, NULL);
+  else if (flags & (DISASSEMBLY_SOURCE | DISASSEMBLY_SOURCE_DEPRECATED))
+    {
+      /* We want to disassemble with source information, however, if such
+	 information is not available then we'd rather have raw assembly
+	 than nothing.  Here we loop until the entire range is
+	 disassembled, giving source information where possible.  */
+      while (low < high)
+	{
+	  int nlines = -1;
+	  CORE_ADDR prev_low = low;
+	  struct symtab *symtab;
+	  struct linetable_entry *le = NULL;
+	  int num_displayed = 0;
 
-  else if (flags & DISASSEMBLY_SOURCE)
-    do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab, low, high,
-				  how_many, flags, stb);
+	  /* Attempt to find a symtab for the PC range.  */
+	  nlines = -1;
+	  symtab = find_pc_line_symtab (low);
 
-  else if (flags & DISASSEMBLY_SOURCE_DEPRECATED)
-    do_mixed_source_and_assembly_deprecated (gdbarch, uiout, &di, symtab,
-					     low, high, how_many, flags, stb);
+	  if (symtab != NULL && symtab->linetable != NULL)
+	    {
+	      /* Convert the linetable to a bunch of my_line_entry's.  */
+	      le = SYMTAB_LINETABLE (symtab)->item;
+	      nlines = SYMTAB_LINETABLE (symtab)->nitems;
+
+	      if (flags & DISASSEMBLY_SOURCE)
+		num_displayed +=
+		  do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab,
+						low, high, how_many, flags,
+						stb, &low);
+	      else
+		num_displayed +=
+		  do_mixed_source_and_assembly_deprecated (gdbarch, uiout,
+							   &di, symtab,
+							   low, high,
+							   how_many,
+							   flags, stb,
+							   &low);
+	    }
+
+	  /* Disassemble a single instruction if we couldn't find a
+	     suitable symtab, or if, for any reason, the above disassembly
+	     didn't move the LOW address on at all.  */
+	  if (how_many != 0
+	      && (nlines <= 0
+		  || symtab == NULL
+		  || symtab->linetable == NULL
+		  || low == prev_low))
+	    num_displayed += do_assembly_only (gdbarch, uiout, &di, low,
+					       high, 1, flags, stb, &low);
+
+	  if (how_many >=0 && num_displayed >= how_many)
+	    break;
+
+	  /* For sanity, if we've not moved then nudge onward.  */
+	  if (low <= prev_low)
+	    low = prev_low + 1;
+	}
+    }
 
   do_cleanups (cleanups);
   gdb_flush (gdb_stdout);
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index e5de0a3..1a613e6 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,14 @@
+2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.base/disasm-end-cu-1.c: Deleted.
+	* gdb.base/disasm-end-cu-2.c: Deleted.
+	* gdb.base/disasm-end-cu.exp: Deleted.
+	* gdb.base/disasm-multi-cu-1.c: New file.
+	* gdb.base/disasm-multi-cu-2.c: New file.
+	* gdb.base/disasm-multi-cu-3.c: New file.
+	* gdb.base/disasm-multi-cu.exp: New file.
+	* gdb.base/disasm-multi-cu.h: New file.
+
 2015-08-27  Ulrich Weigand  <uweigand@de.ibm.com>
 
 	* lib/cell.exp (skip_cell_tests): Report UNRESOLVED on unexpected
diff --git a/gdb/testsuite/gdb.base/disasm-end-cu.exp b/gdb/testsuite/gdb.base/disasm-end-cu.exp
deleted file mode 100644
index fd2c2ac..0000000
--- a/gdb/testsuite/gdb.base/disasm-end-cu.exp
+++ /dev/null
@@ -1,49 +0,0 @@
-# Copyright (C) 2010-2015 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"
-    }
-}
diff --git a/gdb/testsuite/gdb.base/disasm-end-cu-1.c b/gdb/testsuite/gdb.base/disasm-multi-cu-1.c
similarity index 86%
rename from gdb/testsuite/gdb.base/disasm-end-cu-1.c
rename to gdb/testsuite/gdb.base/disasm-multi-cu-1.c
index fe953d7..27470cd 100644
--- a/gdb/testsuite/gdb.base/disasm-end-cu-1.c
+++ b/gdb/testsuite/gdb.base/disasm-multi-cu-1.c
@@ -1,6 +1,6 @@
 /* This testcase is part of GDB, the GNU debugger.
 
-   Copyright (C) 2010-2015 Free Software Foundation, Inc.
+   Copyright (C) 2015 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
@@ -15,14 +15,12 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-int
-dummy_1 (int a, int b, int c)
-{
-  return a + b + c;
-}
+
+#include "disasm-multi-cu.h"
 
 int
 main ()
 {
+  function_1 (); /* debug: main */
   return 0;
 }
diff --git a/gdb/testsuite/gdb.base/disasm-end-cu-2.c b/gdb/testsuite/gdb.base/disasm-multi-cu-2.c
similarity index 80%
rename from gdb/testsuite/gdb.base/disasm-end-cu-2.c
rename to gdb/testsuite/gdb.base/disasm-multi-cu-2.c
index 27dc295..81f8437 100644
--- a/gdb/testsuite/gdb.base/disasm-end-cu-2.c
+++ b/gdb/testsuite/gdb.base/disasm-multi-cu-2.c
@@ -1,6 +1,6 @@
 /* This testcase is part of GDB, the GNU debugger.
 
-   Copyright (C) 2010-2015 Free Software Foundation, Inc.
+   Copyright (C) 2015 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
@@ -15,14 +15,10 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-int
-dummy_2 (int a, int b, int c)
-{
-  return a + b + c;
-}
+#include "disasm-multi-cu.h"
 
-int
-dummy_3 (int a, int b, int c)
+void
+function_1 ()
 {
-  return a - b - c;
+  function_2 (); /* debug: function_1 */
 }
diff --git a/gdb/testsuite/gdb.base/disasm-multi-cu-3.c b/gdb/testsuite/gdb.base/disasm-multi-cu-3.c
new file mode 100644
index 0000000..31bcd01
--- /dev/null
+++ b/gdb/testsuite/gdb.base/disasm-multi-cu-3.c
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2015 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 "disasm-multi-cu.h"
+
+void
+function_2 ()
+{
+  marker (); /* debug: function_2 */
+}
+
+void
+marker ()
+{
+  /* Nothing.  */
+}
diff --git a/gdb/testsuite/gdb.base/disasm-multi-cu.exp b/gdb/testsuite/gdb.base/disasm-multi-cu.exp
new file mode 100644
index 0000000..69f2c65
--- /dev/null
+++ b/gdb/testsuite/gdb.base/disasm-multi-cu.exp
@@ -0,0 +1,111 @@
+# Copyright (C) 2015 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 checks the ability of gdb to disassemble out of one
+# compilation unit and into another, especially when one of the
+# compilation units has debug infomration, and one of them does not.
+
+standard_testfile disasm-multi-cu-1.c disasm-multi-cu-2.c disasm-multi-cu-3.c
+
+set possible_debug_flags [list debug ""]
+set counter 0
+
+foreach srcfile_debug $possible_debug_flags {
+    foreach srcfile2_debug $possible_debug_flags {
+	foreach srcfile3_debug $possible_debug_flags {
+
+	    incr counter
+	    set testname "${testfile}.${counter}"
+
+	    verbose -log "Starting sub-test ${testname}: { $srcfile_debug , $srcfile2_debug , $srcfile3_debug }"
+	    with_test_prefix "${testname}" {
+
+		if { [prepare_for_testing_full ${testname}.exp \
+			  [list ${testname} debug \
+			       $srcfile [list $srcfile_debug] \
+			       $srcfile2 [list $srcfile2_debug] \
+			       $srcfile3 [list $srcfile3_debug]]]} {
+		    fail "Failed to compile"
+		    continue
+		}
+
+		if ![runto_main] {
+		    fail "Failed to reach main"
+		    continue
+		}
+
+		set main_addr [get_hexadecimal_valueof "&main" "0"]
+		set func_1_addr [get_hexadecimal_valueof "&function_1" "0"]
+		set func_2_addr [get_hexadecimal_valueof "&function_2" "0"]
+		set marker_addr [get_hexadecimal_valueof "&marker" "0"]
+
+		if {$main_addr == 0 || $func_1_addr == 0 || $func_2_addr == 0 \
+			|| $marker_addr <= $main_addr \
+			|| $marker_addr <= $func_1_addr \
+			|| $marker_addr <= $func_2_addr \
+			|| $func_2_addr <= $main_addr \
+			|| $func_2_addr <= $func_1_addr \
+			|| $func_1_addr <= $main_addr } {
+		    fail "Unable to extract required addresses, or addresses out of order"
+		    continue
+		}
+
+		set main_pattern [string replace "${main_addr}" 0 1 "0x0*"]
+		if {$srcfile_debug == "debug"} {
+		    set best_main_pattern "${main_pattern}.*debug: main"
+		} else {
+		    set best_main_pattern ${main_pattern}
+		}
+		set func_1_pattern [string replace "${func_1_addr}" 0 1 "0x0*"]
+		if {$srcfile2_debug == "debug"} {
+		    set best_func_1_pattern "${func_1_pattern}.*debug: function_1"
+		} else {
+		    set best_func_1_pattern ${func_1_pattern}
+		}
+		set func_2_pattern [string replace "${func_2_addr}" 0 1 "0x0*"]
+		if {$srcfile3_debug == "debug"} {
+		    set best_func_2_pattern "${func_2_pattern}.*debug: function_2"
+		} else {
+		    set best_func_2_pattern ${func_2_pattern}
+		}
+
+		foreach disassembler_flag [list "/s" "/m"] {
+		    with_test_prefix "${disassembler_flag}" {
+			gdb_test_multiple "disassemble ${disassembler_flag} ${main_addr},${marker_addr}" "Disassemble address range with source" {
+			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+				fail "No output from the disassemble command"
+			    }
+			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:.*\r\n *${best_main_pattern}.*\r\n *${best_func_1_pattern}.*\r\n *${best_func_2_pattern}.*\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+				pass "disassembly output looks correct"
+			    }
+			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:.*\r\n *${main_pattern}.*\r\n *${func_1_pattern}.*\r\n *${func_2_pattern}.*\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+				fail "missing intermixed source code"
+			    }
+			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:.*\r\n *${main_pattern}.*\r\n *${func_1_pattern}.*\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+				fail "disassembly output truncated after function_1"
+			    }
+			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:.*\r\n *${main_pattern}.*\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
+				fail "disassembly output truncated after main function"
+			    }
+			    -re ".*$gdb_prompt $" {
+				fail "Unexpected output from disassemble command"
+			    }
+			}
+		    }
+		}
+	    }
+	}
+    }
+}
diff --git a/gdb/testsuite/gdb.base/disasm-multi-cu.h b/gdb/testsuite/gdb.base/disasm-multi-cu.h
new file mode 100644
index 0000000..107998c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/disasm-multi-cu.h
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2015 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/>.  */
+
+#ifndef __DISASM_MULTI_CU_H__
+#define __DISASM_MULTI_CU_H__
+
+extern void function_1 ();
+extern void function_2 ();
+extern void marker ();
+
+#endif /* __DISASM_MULTI_CU_H__ */
-- 
2.5.1

Re: [PATCH 2/2] gdb: Ensure disassembler covers requested address range.

[Doug Evans]

Andrew Burgess <andrew.burgess@embecosm.com> writes:
> If gdb is asked to disassemble a particular address range, but that
> address range spans the end of a symbol table, then currently gdb will
> exit the disassembler early.
>
> This commit makes the disassembler handle passing over the boundary
> between different symbol tables, and even over regions where there is no
> symbol table.
>
> The only reason that the disassembler will now not disassemble a
> complete address range as asked is if the inferior memory can't be
> read within that region.
>
> The existing test for disassembling over compilation unit boundary is
> not sufficient, in fact the behaviour has regressed since the test was
> first added in 2011 (commit 9011945e46bf8b) and the test failed to
> highlight this regression.
>
> This commit removes the old test in this area, and adds new, more
> comprehensive tests that cover the following features:
>
>  1. Disassembly over the end of a compilation unit and into the next
>     compilation unit should work.
>
>  2. When different compilation units do, or do not, have debug
>     information, we should still get some disassembly output, even when
>     asking for disassembly with source code.
>
>  3. When asking for disassembly with source code, we should, where
>     possible provide that source code, even when earlier compilation
>     units within the region being disassembled don't have debug
>     information.
>
> gdb/ChangeLog:
>
> 	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
> 	count, and take extra parameter.  Fill in final_pc parameter.
> 	(do_mixed_source_and_assembly): Likewise.
> 	(do_assembly_only): Likewise, also return from function when we've
> 	displayed enough instructions.
> 	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
> 	raw instructions as required.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.base/disasm-end-cu-1.c: Deleted.
> 	* gdb.base/disasm-end-cu-2.c: Deleted.
> 	* gdb.base/disasm-end-cu.exp: Deleted.
> 	* gdb.base/disasm-multi-cu-1.c: New file.
> 	* gdb.base/disasm-multi-cu-2.c: New file.
> 	* gdb.base/disasm-multi-cu-3.c: New file.
> 	* gdb.base/disasm-multi-cu.exp: New file.
> 	* gdb.base/disasm-multi-cu.h: New file.

Hi. Some comments inline.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 463b1b9..7ff35a2 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,15 @@
>  2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
>  
> +	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
> +	count, and take extra parameter.  Fill in final_pc parameter.
> +	(do_mixed_source_and_assembly): Likewise.
> +	(do_assembly_only): Likewise, also return from function when we've
> +	displayed enough instructions.
> +	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
> +	raw instructions as required.
> +
> +2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
>  	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
>  	asm_insns list.
>  	(do_mixed_source_and_assembly): Likewise.
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 3032090..9199eb2 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -274,12 +274,12 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
>  
>     N.B. This view is deprecated.  */
>  
> -static void
> +static int
>  do_mixed_source_and_assembly_deprecated
>    (struct gdbarch *gdbarch, struct ui_out *uiout,
>     struct disassemble_info *di, struct symtab *symtab,
>     CORE_ADDR low, CORE_ADDR high,
> -   int how_many, int flags, struct ui_file *stb)
> +   int how_many, int flags, struct ui_file *stb, CORE_ADDR *final_pc)
>  {
>    int newlines = 0;
>    int nlines;
> @@ -293,6 +293,7 @@ do_mixed_source_and_assembly_deprecated
>    enum print_source_lines_flags psl_flags = 0;
>    struct cleanup *ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
>    struct cleanup *ui_out_list_chain = make_cleanup (null_cleanup, 0);
> +  CORE_ADDR next_pc = low;
>  
>    gdb_assert (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL);
>  
> @@ -411,7 +412,7 @@ do_mixed_source_and_assembly_deprecated
>  
>        num_displayed += dump_insns (gdbarch, uiout, di,
>  				   mle[i].start_pc, mle[i].end_pc,
> -				   how_many, flags, stb, NULL);
> +				   how_many, flags, stb, &next_pc);
>  
>        /* When we've reached the end of the mle array, or we've seen the last
>           assembly range for this source line, close out the list/tuple.  */
> @@ -426,6 +427,11 @@ do_mixed_source_and_assembly_deprecated
>        if (how_many >= 0 && num_displayed >= how_many)
>  	break;
>      }
> +
> +  if (final_pc != NULL)
> +    *final_pc = next_pc;
> +
> +  return num_displayed;
>  }
>  
>  /* The idea here is to present a source-O-centric view of a
> @@ -433,12 +439,13 @@ do_mixed_source_and_assembly_deprecated
>     in source order, with (possibly) out of order assembly
>     immediately following.  */

Bleah. This function comment needs updating. Mea culpa.
I'll get to it after this patch (to not interfere with it).

>  
> -static void
> +static int
>  do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>  			      struct disassemble_info *di,
>  			      struct symtab *main_symtab,
>  			      CORE_ADDR low, CORE_ADDR high,
> -			      int how_many, int flags, struct ui_file *stb)
> +			      int how_many, int flags, struct ui_file *stb,
> +			      CORE_ADDR *final_pc)
>  {
>    int newlines = 0;
>    const struct linetable_entry *le, *first_le;
> @@ -455,6 +462,7 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>    struct symtab *last_symtab;
>    int last_line;
>    htab_t dis_line_table;
> +  CORE_ADDR end_pc = low;
>  
>    gdb_assert (main_symtab != NULL && SYMTAB_LINETABLE (main_symtab) != NULL);
>  
> @@ -537,7 +545,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>      {
>        struct linetable_entry *le = NULL;
>        struct symtab_and_line sal;
> -      CORE_ADDR end_pc;
>        int start_preceding_line_to_display = 0;
>        int end_preceding_line_to_display = 0;
>        int new_source_line = 0;
> @@ -676,18 +683,39 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>      }
>  
>    do_cleanups (cleanups);
> +
> +  if (final_pc != NULL)
> +    *final_pc = end_pc;
> +
> +  return num_displayed;
>  }
>  
> -static void
> +static int
>  do_assembly_only (struct gdbarch *gdbarch, struct ui_out *uiout,
>  		  struct disassemble_info * di,
>  		  CORE_ADDR low, CORE_ADDR high,
> -		  int how_many, int flags, struct ui_file *stb)
> +		  int how_many, int flags, struct ui_file *stb,
> +		  CORE_ADDR *final_pc)
>  {
> -  int num_displayed = 0;
> +  CORE_ADDR npc;
> +  int total = 0;
> +
> +  do
> +    {
> +      total += dump_insns (gdbarch, uiout, di, low, high,
> +			   how_many, flags, stb, &npc);
> +      if (npc >= high || total >= how_many)
> +        break;
> +      if (low == npc)
> +	low = npc + 1;

npc + 1?
Perhaps it's correct, but at a glance I don't see how.

> +      else
> +	low = npc;
> +    } while (1);

I think(!) convention requires the "while (1);" to go on the next line.

The higher order question, though, is why is this loop needed?
IOW, what does the loop do that dump_insns doesn't already do?
Am I missing something?

> +
> +  if (final_pc != NULL)
> +    *final_pc = npc;
>  
> -  num_displayed = dump_insns (gdbarch, uiout, di, low, high, how_many,
> -                              flags, stb, NULL);
> +  return total;
>  }
>  
>  /* Initialize the disassemble info struct ready for the specified
> @@ -740,31 +768,72 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>    struct ui_file *stb = mem_fileopen ();
>    struct cleanup *cleanups = make_cleanup_ui_file_delete (stb);
>    struct disassemble_info di = gdb_disassemble_info (gdbarch, stb);
> -  struct symtab *symtab;
> -  struct linetable_entry *le = NULL;
> -  int nlines = -1;
> -
> -  /* Assume symtab is valid for whole PC range.  */
> -  symtab = find_pc_line_symtab (low);
> -
> -  if (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL)
> -    nlines = SYMTAB_LINETABLE (symtab)->nitems;
>  
>    /* This cleanup is inner to CLEANUPS, so we don't need a separate
>       variable to track it.  */
>    (void) make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
>  
> -  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
> -      || nlines <= 0)
> -    do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
> +  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE)))
> +    (void) do_assembly_only (gdbarch, uiout, &di, low, high,
> +			     how_many, flags, stb, NULL);
> +  else if (flags & (DISASSEMBLY_SOURCE | DISASSEMBLY_SOURCE_DEPRECATED))
> +    {
> +      /* We want to disassemble with source information, however, if such
> +	 information is not available then we'd rather have raw assembly
> +	 than nothing.  Here we loop until the entire range is
> +	 disassembled, giving source information where possible.  */
> +      while (low < high)
> +	{
> +	  int nlines = -1;

No need to init nlines to -1 twice.
I'd say delete this init.

> +	  CORE_ADDR prev_low = low;
> +	  struct symtab *symtab;
> +	  struct linetable_entry *le = NULL;
> +	  int num_displayed = 0;
>  
> -  else if (flags & DISASSEMBLY_SOURCE)
> -    do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab, low, high,
> -				  how_many, flags, stb);
> +	  /* Attempt to find a symtab for the PC range.  */
> +	  nlines = -1;
> +	  symtab = find_pc_line_symtab (low);
>  
> -  else if (flags & DISASSEMBLY_SOURCE_DEPRECATED)
> -    do_mixed_source_and_assembly_deprecated (gdbarch, uiout, &di, symtab,
> -					     low, high, how_many, flags, stb);
> +	  if (symtab != NULL && symtab->linetable != NULL)
> +	    {
> +	      /* Convert the linetable to a bunch of my_line_entry's.  */
> +	      le = SYMTAB_LINETABLE (symtab)->item;
> +	      nlines = SYMTAB_LINETABLE (symtab)->nitems;
> +
> +	      if (flags & DISASSEMBLY_SOURCE)
> +		num_displayed +=
> +		  do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab,
> +						low, high, how_many, flags,
> +						stb, &low);

Storing final_pc in low here is awkward.
Can you store it in end_pc or some such first?

> +	      else
> +		num_displayed +=
> +		  do_mixed_source_and_assembly_deprecated (gdbarch, uiout,
> +							   &di, symtab,
> +							   low, high,
> +							   how_many,
> +							   flags, stb,
> +							   &low);

Ditto.

> +	    }
> +
> +	  /* Disassemble a single instruction if we couldn't find a
> +	     suitable symtab, or if, for any reason, the above disassembly
> +	     didn't move the LOW address on at all.  */

I think a reasonable case can be made that this is the wrong thing to
do for the deprecated case (based on my observations of how it works,
and does not work). Consider that it doesn't print inlined function calls
that come from another file. If it doesn't do that, why go to any effort
to print non-source-based disassembly in the gaps between source files.
[Over time I can imagine attempts to try to morph the deprecated
case into the new non-deprecated case, i.e., changing /m to be more like /s.
How about leaving /m the way it is, and just encourage everyone to use /s.]

> +	  if (how_many != 0
> +	      && (nlines <= 0
> +		  || symtab == NULL
> +		  || symtab->linetable == NULL
> +		  || low == prev_low))
> +	    num_displayed += do_assembly_only (gdbarch, uiout, &di, low,
> +					       high, 1, flags, stb, &low);
> +
> +	  if (how_many >=0 && num_displayed >= how_many)
> +	    break;
> +
> +	  /* For sanity, if we've not moved then nudge onward.  */
> +	  if (low <= prev_low)
> +	    low = prev_low + 1;

This doesn't feel right, e.g. for ISAs with instruction widths always > 1.
Am I missing something?

I didn't have time to go through the testsuite changes with a
fine-toothed comb, but it seemed ok.

> +	}
> +    }
>  
>    do_cleanups (cleanups);
>    gdb_flush (gdb_stdout);
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index e5de0a3..1a613e6 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,14 @@
> +2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	* gdb.base/disasm-end-cu-1.c: Deleted.
> +	* gdb.base/disasm-end-cu-2.c: Deleted.
> +	* gdb.base/disasm-end-cu.exp: Deleted.
> +	* gdb.base/disasm-multi-cu-1.c: New file.
> +	* gdb.base/disasm-multi-cu-2.c: New file.
> +	* gdb.base/disasm-multi-cu-3.c: New file.
> +	* gdb.base/disasm-multi-cu.exp: New file.
> +	* gdb.base/disasm-multi-cu.h: New file.
> +
>  2015-08-27  Ulrich Weigand  <uweigand@de.ibm.com>
>  
>  	* lib/cell.exp (skip_cell_tests): Report UNRESOLVED on unexpected
> diff --git a/gdb/testsuite/gdb.base/disasm-end-cu.exp b/gdb/testsuite/gdb.base/disasm-end-cu.exp
> deleted file mode 100644
> index fd2c2ac..0000000
> --- a/gdb/testsuite/gdb.base/disasm-end-cu.exp
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -# Copyright (C) 2010-2015 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"
> -    }
> -}
> diff --git a/gdb/testsuite/gdb.base/disasm-end-cu-1.c b/gdb/testsuite/gdb.base/disasm-multi-cu-1.c
> similarity index 86%
> rename from gdb/testsuite/gdb.base/disasm-end-cu-1.c
> rename to gdb/testsuite/gdb.base/disasm-multi-cu-1.c
> index fe953d7..27470cd 100644
> --- a/gdb/testsuite/gdb.base/disasm-end-cu-1.c
> +++ b/gdb/testsuite/gdb.base/disasm-multi-cu-1.c
> @@ -1,6 +1,6 @@
>  /* This testcase is part of GDB, the GNU debugger.
>  
> -   Copyright (C) 2010-2015 Free Software Foundation, Inc.
> +   Copyright (C) 2015 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
> @@ -15,14 +15,12 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> -int
> -dummy_1 (int a, int b, int c)
> -{
> -  return a + b + c;
> -}
> +
> +#include "disasm-multi-cu.h"
>  
>  int
>  main ()
>  {
> +  function_1 (); /* debug: main */
>    return 0;
>  }
> diff --git a/gdb/testsuite/gdb.base/disasm-end-cu-2.c b/gdb/testsuite/gdb.base/disasm-multi-cu-2.c
> similarity index 80%
> rename from gdb/testsuite/gdb.base/disasm-end-cu-2.c
> rename to gdb/testsuite/gdb.base/disasm-multi-cu-2.c
> index 27dc295..81f8437 100644
> --- a/gdb/testsuite/gdb.base/disasm-end-cu-2.c
> +++ b/gdb/testsuite/gdb.base/disasm-multi-cu-2.c
> @@ -1,6 +1,6 @@
>  /* This testcase is part of GDB, the GNU debugger.
>  
> -   Copyright (C) 2010-2015 Free Software Foundation, Inc.
> +   Copyright (C) 2015 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
> @@ -15,14 +15,10 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> -int
> -dummy_2 (int a, int b, int c)
> -{
> -  return a + b + c;
> -}
> +#include "disasm-multi-cu.h"
>  
> -int
> -dummy_3 (int a, int b, int c)
> +void
> +function_1 ()
>  {
> -  return a - b - c;
> +  function_2 (); /* debug: function_1 */
>  }
> diff --git a/gdb/testsuite/gdb.base/disasm-multi-cu-3.c b/gdb/testsuite/gdb.base/disasm-multi-cu-3.c
> new file mode 100644
> index 0000000..31bcd01
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/disasm-multi-cu-3.c
> @@ -0,0 +1,30 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright (C) 2015 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 "disasm-multi-cu.h"
> +
> +void
> +function_2 ()
> +{
> +  marker (); /* debug: function_2 */
> +}
> +
> +void
> +marker ()
> +{
> +  /* Nothing.  */
> +}
> diff --git a/gdb/testsuite/gdb.base/disasm-multi-cu.exp b/gdb/testsuite/gdb.base/disasm-multi-cu.exp
> new file mode 100644
> index 0000000..69f2c65
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/disasm-multi-cu.exp
> @@ -0,0 +1,111 @@
> +# Copyright (C) 2015 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 checks the ability of gdb to disassemble out of one
> +# compilation unit and into another, especially when one of the
> +# compilation units has debug infomration, and one of them does not.
> +
> +standard_testfile disasm-multi-cu-1.c disasm-multi-cu-2.c disasm-multi-cu-3.c
> +
> +set possible_debug_flags [list debug ""]
> +set counter 0
> +
> +foreach srcfile_debug $possible_debug_flags {
> +    foreach srcfile2_debug $possible_debug_flags {
> +	foreach srcfile3_debug $possible_debug_flags {
> +
> +	    incr counter
> +	    set testname "${testfile}.${counter}"
> +
> +	    verbose -log "Starting sub-test ${testname}: { $srcfile_debug , $srcfile2_debug , $srcfile3_debug }"
> +	    with_test_prefix "${testname}" {
> +
> +		if { [prepare_for_testing_full ${testname}.exp \
> +			  [list ${testname} debug \
> +			       $srcfile [list $srcfile_debug] \
> +			       $srcfile2 [list $srcfile2_debug] \
> +			       $srcfile3 [list $srcfile3_debug]]]} {
> +		    fail "Failed to compile"
> +		    continue
> +		}
> +
> +		if ![runto_main] {
> +		    fail "Failed to reach main"
> +		    continue
> +		}
> +
> +		set main_addr [get_hexadecimal_valueof "&main" "0"]
> +		set func_1_addr [get_hexadecimal_valueof "&function_1" "0"]
> +		set func_2_addr [get_hexadecimal_valueof "&function_2" "0"]
> +		set marker_addr [get_hexadecimal_valueof "&marker" "0"]
> +
> +		if {$main_addr == 0 || $func_1_addr == 0 || $func_2_addr == 0 \
> +			|| $marker_addr <= $main_addr \
> +			|| $marker_addr <= $func_1_addr \
> +			|| $marker_addr <= $func_2_addr \
> +			|| $func_2_addr <= $main_addr \
> +			|| $func_2_addr <= $func_1_addr \
> +			|| $func_1_addr <= $main_addr } {
> +		    fail "Unable to extract required addresses, or addresses out of order"
> +		    continue
> +		}
> +
> +		set main_pattern [string replace "${main_addr}" 0 1 "0x0*"]
> +		if {$srcfile_debug == "debug"} {
> +		    set best_main_pattern "${main_pattern}.*debug: main"
> +		} else {
> +		    set best_main_pattern ${main_pattern}
> +		}
> +		set func_1_pattern [string replace "${func_1_addr}" 0 1 "0x0*"]
> +		if {$srcfile2_debug == "debug"} {
> +		    set best_func_1_pattern "${func_1_pattern}.*debug: function_1"
> +		} else {
> +		    set best_func_1_pattern ${func_1_pattern}
> +		}
> +		set func_2_pattern [string replace "${func_2_addr}" 0 1 "0x0*"]
> +		if {$srcfile3_debug == "debug"} {
> +		    set best_func_2_pattern "${func_2_pattern}.*debug: function_2"
> +		} else {
> +		    set best_func_2_pattern ${func_2_pattern}
> +		}
> +
> +		foreach disassembler_flag [list "/s" "/m"] {
> +		    with_test_prefix "${disassembler_flag}" {
> +			gdb_test_multiple "disassemble ${disassembler_flag} ${main_addr},${marker_addr}" "Disassemble address range with source" {
> +			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
> +				fail "No output from the disassemble command"
> +			    }
> +			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:.*\r\n *${best_main_pattern}.*\r\n *${best_func_1_pattern}.*\r\n *${best_func_2_pattern}.*\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
> +				pass "disassembly output looks correct"
> +			    }
> +			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:.*\r\n *${main_pattern}.*\r\n *${func_1_pattern}.*\r\n *${func_2_pattern}.*\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
> +				fail "missing intermixed source code"
> +			    }
> +			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:.*\r\n *${main_pattern}.*\r\n *${func_1_pattern}.*\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
> +				fail "disassembly output truncated after function_1"
> +			    }
> +			    -re "Dump of assembler code from ${main_addr} to ${marker_addr}:.*\r\n *${main_pattern}.*\r\nEnd of assembler dump\.\r\n$gdb_prompt $" {
> +				fail "disassembly output truncated after main function"
> +			    }
> +			    -re ".*$gdb_prompt $" {
> +				fail "Unexpected output from disassemble command"
> +			    }
> +			}
> +		    }
> +		}
> +	    }
> +	}
> +    }
> +}
> diff --git a/gdb/testsuite/gdb.base/disasm-multi-cu.h b/gdb/testsuite/gdb.base/disasm-multi-cu.h
> new file mode 100644
> index 0000000..107998c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/disasm-multi-cu.h
> @@ -0,0 +1,25 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright (C) 2015 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/>.  */
> +
> +#ifndef __DISASM_MULTI_CU_H__
> +#define __DISASM_MULTI_CU_H__
> +
> +extern void function_1 ();
> +extern void function_2 ();
> +extern void marker ();
> +
> +#endif /* __DISASM_MULTI_CU_H__ */

Re: [PATCH 2/2] gdb: Ensure disassembler covers requested address range.

[Andrew Burgess]

Doug,

Thanks for the review.  I don't have a new revision of this patch yet,
I would like to discuss one of the issue you raised first.  Feedback
inline.

* Doug Evans <xdje42@gmail.com> [2015-09-16 21:03:46 -0700]:

> Andrew Burgess <andrew.burgess@embecosm.com> writes:
> > If gdb is asked to disassemble a particular address range, but that
> > address range spans the end of a symbol table, then currently gdb will
> > exit the disassembler early.
> >
> > This commit makes the disassembler handle passing over the boundary
> > between different symbol tables, and even over regions where there is no
> > symbol table.
> >
> > The only reason that the disassembler will now not disassemble a
> > complete address range as asked is if the inferior memory can't be
> > read within that region.
> >
> > The existing test for disassembling over compilation unit boundary is
> > not sufficient, in fact the behaviour has regressed since the test was
> > first added in 2011 (commit 9011945e46bf8b) and the test failed to
> > highlight this regression.
> >
> > This commit removes the old test in this area, and adds new, more
> > comprehensive tests that cover the following features:
> >
> >  1. Disassembly over the end of a compilation unit and into the next
> >     compilation unit should work.
> >
> >  2. When different compilation units do, or do not, have debug
> >     information, we should still get some disassembly output, even when
> >     asking for disassembly with source code.
> >
> >  3. When asking for disassembly with source code, we should, where
> >     possible provide that source code, even when earlier compilation
> >     units within the region being disassembled don't have debug
> >     information.
> >
> > gdb/ChangeLog:
> >
> > 	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
> > 	count, and take extra parameter.  Fill in final_pc parameter.
> > 	(do_mixed_source_and_assembly): Likewise.
> > 	(do_assembly_only): Likewise, also return from function when we've
> > 	displayed enough instructions.
> > 	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
> > 	raw instructions as required.
> >
> > gdb/testsuite/ChangeLog:
> >
> > 	* gdb.base/disasm-end-cu-1.c: Deleted.
> > 	* gdb.base/disasm-end-cu-2.c: Deleted.
> > 	* gdb.base/disasm-end-cu.exp: Deleted.
> > 	* gdb.base/disasm-multi-cu-1.c: New file.
> > 	* gdb.base/disasm-multi-cu-2.c: New file.
> > 	* gdb.base/disasm-multi-cu-3.c: New file.
> > 	* gdb.base/disasm-multi-cu.exp: New file.
> > 	* gdb.base/disasm-multi-cu.h: New file.
> 
> Hi. Some comments inline.
> 
> > diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> > index 463b1b9..7ff35a2 100644
> > --- a/gdb/ChangeLog
> > +++ b/gdb/ChangeLog
> > @@ -1,5 +1,15 @@
> >  2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
> >  
> > +	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
> > +	count, and take extra parameter.  Fill in final_pc parameter.
> > +	(do_mixed_source_and_assembly): Likewise.
> > +	(do_assembly_only): Likewise, also return from function when we've
> > +	displayed enough instructions.
> > +	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
> > +	raw instructions as required.
> > +
> > +2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
> > +
> >  	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
> >  	asm_insns list.
> >  	(do_mixed_source_and_assembly): Likewise.
> > diff --git a/gdb/disasm.c b/gdb/disasm.c
> > index 3032090..9199eb2 100644
> > --- a/gdb/disasm.c
> > +++ b/gdb/disasm.c
> > @@ -274,12 +274,12 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
> >  
> >     N.B. This view is deprecated.  */
> >  
> > -static void
> > +static int
> >  do_mixed_source_and_assembly_deprecated
> >    (struct gdbarch *gdbarch, struct ui_out *uiout,
> >     struct disassemble_info *di, struct symtab *symtab,
> >     CORE_ADDR low, CORE_ADDR high,
> > -   int how_many, int flags, struct ui_file *stb)
> > +   int how_many, int flags, struct ui_file *stb, CORE_ADDR *final_pc)
> >  {
> >    int newlines = 0;
> >    int nlines;
> > @@ -293,6 +293,7 @@ do_mixed_source_and_assembly_deprecated
> >    enum print_source_lines_flags psl_flags = 0;
> >    struct cleanup *ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
> >    struct cleanup *ui_out_list_chain = make_cleanup (null_cleanup, 0);
> > +  CORE_ADDR next_pc = low;
> >  
> >    gdb_assert (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL);
> >  
> > @@ -411,7 +412,7 @@ do_mixed_source_and_assembly_deprecated
> >  
> >        num_displayed += dump_insns (gdbarch, uiout, di,
> >  				   mle[i].start_pc, mle[i].end_pc,
> > -				   how_many, flags, stb, NULL);
> > +				   how_many, flags, stb, &next_pc);
> >  
> >        /* When we've reached the end of the mle array, or we've seen the last
> >           assembly range for this source line, close out the list/tuple.  */
> > @@ -426,6 +427,11 @@ do_mixed_source_and_assembly_deprecated
> >        if (how_many >= 0 && num_displayed >= how_many)
> >  	break;
> >      }
> > +
> > +  if (final_pc != NULL)
> > +    *final_pc = next_pc;
> > +
> > +  return num_displayed;
> >  }
> >  
> >  /* The idea here is to present a source-O-centric view of a
> > @@ -433,12 +439,13 @@ do_mixed_source_and_assembly_deprecated
> >     in source order, with (possibly) out of order assembly
> >     immediately following.  */
> 
> Bleah. This function comment needs updating. Mea culpa.
> I'll get to it after this patch (to not interfere with it).

If you're going to do that, do you think you could rename the
function to deprecated_do_mixed_source_and_assembly.  All the other
deprecated functions in gdb, and almost all of the deprecated
variables use a prefix rather than suffix approach - it just makes
finding deprecated stuff easier :)

> 
> >  
> > -static void
> > +static int
> >  do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
> >  			      struct disassemble_info *di,
> >  			      struct symtab *main_symtab,
> >  			      CORE_ADDR low, CORE_ADDR high,
> > -			      int how_many, int flags, struct ui_file *stb)
> > +			      int how_many, int flags, struct ui_file *stb,
> > +			      CORE_ADDR *final_pc)
> >  {
> >    int newlines = 0;
> >    const struct linetable_entry *le, *first_le;
> > @@ -455,6 +462,7 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
> >    struct symtab *last_symtab;
> >    int last_line;
> >    htab_t dis_line_table;
> > +  CORE_ADDR end_pc = low;
> >  
> >    gdb_assert (main_symtab != NULL && SYMTAB_LINETABLE (main_symtab) != NULL);
> >  
> > @@ -537,7 +545,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
> >      {
> >        struct linetable_entry *le = NULL;
> >        struct symtab_and_line sal;
> > -      CORE_ADDR end_pc;
> >        int start_preceding_line_to_display = 0;
> >        int end_preceding_line_to_display = 0;
> >        int new_source_line = 0;
> > @@ -676,18 +683,39 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
> >      }
> >  
> >    do_cleanups (cleanups);
> > +
> > +  if (final_pc != NULL)
> > +    *final_pc = end_pc;
> > +
> > +  return num_displayed;
> >  }
> >  
> > -static void
> > +static int
> >  do_assembly_only (struct gdbarch *gdbarch, struct ui_out *uiout,
> >  		  struct disassemble_info * di,
> >  		  CORE_ADDR low, CORE_ADDR high,
> > -		  int how_many, int flags, struct ui_file *stb)
> > +		  int how_many, int flags, struct ui_file *stb,
> > +		  CORE_ADDR *final_pc)
> >  {
> > -  int num_displayed = 0;
> > +  CORE_ADDR npc;
> > +  int total = 0;
> > +
> > +  do
> > +    {
> > +      total += dump_insns (gdbarch, uiout, di, low, high,
> > +			   how_many, flags, stb, &npc);
> > +      if (npc >= high || total >= how_many)
> > +        break;
> > +      if (low == npc)
> > +	low = npc + 1;
> 
> npc + 1?
> Perhaps it's correct, but at a glance I don't see how.

No you're not.  This should have more comments.  I think on revision
I'll remove this change for now.

These disassembler changes were pulled out of an even larger
disassembler patch that I have had say around for ages.

The addition of +1 both here, and further down are part of addressing
disassembling over unreadable memory.  The +1 was part of ensuring
that the disassembler continued to make progress.

With out the supporting changes though the +1 alone does not make a
lot of sense, my only defence for leaving it in was that it makes the
gdb side of the disassembler super-paranoid: if for any reason the
disassembler fails to make progress gdb will still nudge the address
on.  However, in general gdb is not written that way so I should
probably drop this change.

>
> > +      else
> > +	low = npc;
> > +    } while (1);
> 
> I think(!) convention requires the "while (1);" to go on the next line.
> 
> The higher order question, though, is why is this loop needed?
> IOW, what does the loop do that dump_insns doesn't already do?
> Am I missing something?

This relates to the above point and should be removed (for now), the
assumption will be that dump_insns will always cover the requested
range.

> > +
> > +  if (final_pc != NULL)
> > +    *final_pc = npc;
> >  
> > -  num_displayed = dump_insns (gdbarch, uiout, di, low, high, how_many,
> > -                              flags, stb, NULL);
> > +  return total;
> >  }
> >  
> >  /* Initialize the disassemble info struct ready for the specified
> > @@ -740,31 +768,72 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
> >    struct ui_file *stb = mem_fileopen ();
> >    struct cleanup *cleanups = make_cleanup_ui_file_delete (stb);
> >    struct disassemble_info di = gdb_disassemble_info (gdbarch, stb);
> > -  struct symtab *symtab;
> > -  struct linetable_entry *le = NULL;
> > -  int nlines = -1;
> > -
> > -  /* Assume symtab is valid for whole PC range.  */
> > -  symtab = find_pc_line_symtab (low);
> > -
> > -  if (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL)
> > -    nlines = SYMTAB_LINETABLE (symtab)->nitems;
> >  
> >    /* This cleanup is inner to CLEANUPS, so we don't need a separate
> >       variable to track it.  */
> >    (void) make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
> >  
> > -  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
> > -      || nlines <= 0)
> > -    do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
> > +  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE)))
> > +    (void) do_assembly_only (gdbarch, uiout, &di, low, high,
> > +			     how_many, flags, stb, NULL);
> > +  else if (flags & (DISASSEMBLY_SOURCE | DISASSEMBLY_SOURCE_DEPRECATED))
> > +    {
> > +      /* We want to disassemble with source information, however, if such
> > +	 information is not available then we'd rather have raw assembly
> > +	 than nothing.  Here we loop until the entire range is
> > +	 disassembled, giving source information where possible.  */
> > +      while (low < high)
> > +	{
> > +	  int nlines = -1;
> 
> No need to init nlines to -1 twice.
> I'd say delete this init.

My mistake.  Will fix.

> 
> > +	  CORE_ADDR prev_low = low;
> > +	  struct symtab *symtab;
> > +	  struct linetable_entry *le = NULL;
> > +	  int num_displayed = 0;
> >  
> > -  else if (flags & DISASSEMBLY_SOURCE)
> > -    do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab, low, high,
> > -				  how_many, flags, stb);
> > +	  /* Attempt to find a symtab for the PC range.  */
> > +	  nlines = -1;
> > +	  symtab = find_pc_line_symtab (low);
> >  
> > -  else if (flags & DISASSEMBLY_SOURCE_DEPRECATED)
> > -    do_mixed_source_and_assembly_deprecated (gdbarch, uiout, &di, symtab,
> > -					     low, high, how_many, flags, stb);
> > +	  if (symtab != NULL && symtab->linetable != NULL)
> > +	    {
> > +	      /* Convert the linetable to a bunch of my_line_entry's.  */
> > +	      le = SYMTAB_LINETABLE (symtab)->item;
> > +	      nlines = SYMTAB_LINETABLE (symtab)->nitems;
> > +
> > +	      if (flags & DISASSEMBLY_SOURCE)
> > +		num_displayed +=
> > +		  do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab,
> > +						low, high, how_many, flags,
> > +						stb, &low);
> 
> Storing final_pc in low here is awkward.
> Can you store it in end_pc or some such first?

OK, will rename some of the variable around this area.

> 
> > +	      else
> > +		num_displayed +=
> > +		  do_mixed_source_and_assembly_deprecated (gdbarch, uiout,
> > +							   &di, symtab,
> > +							   low, high,
> > +							   how_many,
> > +							   flags, stb,
> > +							   &low);
> 
> Ditto.
> 
> > +	    }
> > +
> > +	  /* Disassemble a single instruction if we couldn't find a
> > +	     suitable symtab, or if, for any reason, the above disassembly
> > +	     didn't move the LOW address on at all.  */
> 
> I think a reasonable case can be made that this is the wrong thing to
> do for the deprecated case (based on my observations of how it works,
> and does not work). Consider that it doesn't print inlined function calls
> that come from another file. If it doesn't do that, why go to any effort
> to print non-source-based disassembly in the gaps between source
> files.

I guess I see the question the other way around; why should we
maintain a separate way of doing things just to preserve (what I see
as) a bug?

By placing the choice between the old and new right into the middle of
my new disassembly control loop both old and new pick up the fix.  To
do what you suggest I see two choices (I'm open to alternative
suggestions though):

  1. Move the call to the old, deprecated, function outside of the new
     disassembly control loop.  We'd probably end up with code
     duplication, so extra maintenance overhead, just to support a
     deprecated method.

  2. Add a 'break;' immediately after the call to the deprecated
     disassembly function (within the else block).  This would ensure
     we only get once chance to disassemble.  Sure, if that's what it
     takes to get my patch in I can do that, but it feels kind of
     petty to me, like we're leaving a bug in place just to force
     people to move to the new method.

> [Over time I can imagine attempts to try to morph the deprecated
> case into the new non-deprecated case, i.e., changing /m to be more like /s.
> How about leaving /m the way it is, and just encourage everyone to
> use /s.]

Is that really so bad?  I'm going from memory here so I may have the
history wrong, but isn't the story that you found the inline-functions
issue with the /m case, looked into fixing it and discovered that
you'd be better off writing a brand new function, hence /s.

But surely, if someone put forward a patch that "fixed" /m (without
making it exactly the same as /s) then would that really be so bad?

I'm in no way proposing to make any changes to /m, I guess I'm just
arguing that the inline-functions issue with /m is orthogonal to this
cross-symtab issue.  Fixing the first was hard for /m, but it feels
like more work to _not_ fix the second issue.

> 
> > +	  if (how_many != 0
> > +	      && (nlines <= 0
> > +		  || symtab == NULL
> > +		  || symtab->linetable == NULL
> > +		  || low == prev_low))
> > +	    num_displayed += do_assembly_only (gdbarch, uiout, &di, low,
> > +					       high, 1, flags, stb, &low);
> > +
> > +	  if (how_many >=0 && num_displayed >= how_many)
> > +	    break;
> > +
> > +	  /* For sanity, if we've not moved then nudge onward.  */
> > +	  if (low <= prev_low)
> > +	    low = prev_low + 1;
> 
> This doesn't feel right, e.g. for ISAs with instruction widths always > 1.
> Am I missing something?

As for the +1 at the very top this was part of the unreadable memory
changes; I'll remove these in a revised patch.

Thanks again for your time,

Andrew

Re: [PATCH 2/2] gdb: Ensure disassembler covers requested address range.

[Doug Evans]

Andrew Burgess <andrew.burgess@embecosm.com> writes:
> Doug,
>
> Thanks for the review.  I don't have a new revision of this patch yet,
> I would like to discuss one of the issue you raised first.  Feedback
> inline.
>
> * Doug Evans <xdje42@gmail.com> [2015-09-16 21:03:46 -0700]:
>
>> Andrew Burgess <andrew.burgess@embecosm.com> writes:
>> > If gdb is asked to disassemble a particular address range, but that
>> > address range spans the end of a symbol table, then currently gdb will
>> > exit the disassembler early.
>> >
>> > This commit makes the disassembler handle passing over the boundary
>> > between different symbol tables, and even over regions where there is no
>> > symbol table.
>> >
>> > The only reason that the disassembler will now not disassemble a
>> > complete address range as asked is if the inferior memory can't be
>> > read within that region.
>> >
>> > The existing test for disassembling over compilation unit boundary is
>> > not sufficient, in fact the behaviour has regressed since the test was
>> > first added in 2011 (commit 9011945e46bf8b) and the test failed to
>> > highlight this regression.
>> >
>> > This commit removes the old test in this area, and adds new, more
>> > comprehensive tests that cover the following features:
>> >
>> >  1. Disassembly over the end of a compilation unit and into the next
>> >     compilation unit should work.
>> >
>> >  2. When different compilation units do, or do not, have debug
>> >     information, we should still get some disassembly output, even when
>> >     asking for disassembly with source code.
>> >
>> >  3. When asking for disassembly with source code, we should, where
>> >     possible provide that source code, even when earlier compilation
>> >     units within the region being disassembled don't have debug
>> >     information.
>> >
>> > gdb/ChangeLog:
>> >
>> > 	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
>> > 	count, and take extra parameter.  Fill in final_pc parameter.
>> > 	(do_mixed_source_and_assembly): Likewise.
>> > 	(do_assembly_only): Likewise, also return from function when we've
>> > 	displayed enough instructions.
>> > 	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
>> > 	raw instructions as required.
>> >
>> > gdb/testsuite/ChangeLog:
>> >
>> > 	* gdb.base/disasm-end-cu-1.c: Deleted.
>> > 	* gdb.base/disasm-end-cu-2.c: Deleted.
>> > 	* gdb.base/disasm-end-cu.exp: Deleted.
>> > 	* gdb.base/disasm-multi-cu-1.c: New file.
>> > 	* gdb.base/disasm-multi-cu-2.c: New file.
>> > 	* gdb.base/disasm-multi-cu-3.c: New file.
>> > 	* gdb.base/disasm-multi-cu.exp: New file.
>> > 	* gdb.base/disasm-multi-cu.h: New file.
>> 
>> Hi. Some comments inline.
>> 
>> > diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> > index 463b1b9..7ff35a2 100644
>> > --- a/gdb/ChangeLog
>> > +++ b/gdb/ChangeLog
>> > @@ -1,5 +1,15 @@
>> >  2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
>> >  
>> > +	* disasm.c (do_mixed_source_and_assembly_deprecated): Return
>> > +	count, and take extra parameter.  Fill in final_pc parameter.
>> > +	(do_mixed_source_and_assembly): Likewise.
>> > +	(do_assembly_only): Likewise, also return from function when we've
>> > +	displayed enough instructions.
>> > +	(gdb_disassembly): Add loop, looking up new symtabs, or displaying
>> > +	raw instructions as required.
>> > +
>> > +2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
>> > +
>> >  	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
>> >  	asm_insns list.
>> >  	(do_mixed_source_and_assembly): Likewise.
>> > diff --git a/gdb/disasm.c b/gdb/disasm.c
>> > index 3032090..9199eb2 100644
>> > --- a/gdb/disasm.c
>> > +++ b/gdb/disasm.c
>> > @@ -274,12 +274,12 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >  
>> >     N.B. This view is deprecated.  */
>> >  
>> > -static void
>> > +static int
>> >  do_mixed_source_and_assembly_deprecated
>> >    (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >     struct disassemble_info *di, struct symtab *symtab,
>> >     CORE_ADDR low, CORE_ADDR high,
>> > -   int how_many, int flags, struct ui_file *stb)
>> > +   int how_many, int flags, struct ui_file *stb, CORE_ADDR *final_pc)
>> >  {
>> >    int newlines = 0;
>> >    int nlines;
>> > @@ -293,6 +293,7 @@ do_mixed_source_and_assembly_deprecated
>> >    enum print_source_lines_flags psl_flags = 0;
>> >    struct cleanup *ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
>> >    struct cleanup *ui_out_list_chain = make_cleanup (null_cleanup, 0);
>> > +  CORE_ADDR next_pc = low;
>> >  
>> >    gdb_assert (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL);
>> >  
>> > @@ -411,7 +412,7 @@ do_mixed_source_and_assembly_deprecated
>> >  
>> >        num_displayed += dump_insns (gdbarch, uiout, di,
>> >  				   mle[i].start_pc, mle[i].end_pc,
>> > -				   how_many, flags, stb, NULL);
>> > +				   how_many, flags, stb, &next_pc);
>> >  
>> >        /* When we've reached the end of the mle array, or we've seen the last
>> >           assembly range for this source line, close out the list/tuple.  */
>> > @@ -426,6 +427,11 @@ do_mixed_source_and_assembly_deprecated
>> >        if (how_many >= 0 && num_displayed >= how_many)
>> >  	break;
>> >      }
>> > +
>> > +  if (final_pc != NULL)
>> > +    *final_pc = next_pc;
>> > +
>> > +  return num_displayed;
>> >  }
>> >  
>> >  /* The idea here is to present a source-O-centric view of a
>> > @@ -433,12 +439,13 @@ do_mixed_source_and_assembly_deprecated
>> >     in source order, with (possibly) out of order assembly
>> >     immediately following.  */
>> 
>> Bleah. This function comment needs updating. Mea culpa.
>> I'll get to it after this patch (to not interfere with it).
>
> If you're going to do that, do you think you could rename the
> function to deprecated_do_mixed_source_and_assembly.  All the other
> deprecated functions in gdb, and almost all of the deprecated
> variables use a prefix rather than suffix approach - it just makes
> finding deprecated stuff easier :)

Sure.

>> 
>> >  
>> > -static void
>> > +static int
>> >  do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >  			      struct disassemble_info *di,
>> >  			      struct symtab *main_symtab,
>> >  			      CORE_ADDR low, CORE_ADDR high,
>> > -			      int how_many, int flags, struct ui_file *stb)
>> > +			      int how_many, int flags, struct ui_file *stb,
>> > +			      CORE_ADDR *final_pc)
>> >  {
>> >    int newlines = 0;
>> >    const struct linetable_entry *le, *first_le;
>> > @@ -455,6 +462,7 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >    struct symtab *last_symtab;
>> >    int last_line;
>> >    htab_t dis_line_table;
>> > +  CORE_ADDR end_pc = low;
>> >  
>> >    gdb_assert (main_symtab != NULL && SYMTAB_LINETABLE (main_symtab) != NULL);
>> >  
>> > @@ -537,7 +545,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >      {
>> >        struct linetable_entry *le = NULL;
>> >        struct symtab_and_line sal;
>> > -      CORE_ADDR end_pc;
>> >        int start_preceding_line_to_display = 0;
>> >        int end_preceding_line_to_display = 0;
>> >        int new_source_line = 0;
>> > @@ -676,18 +683,39 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >      }
>> >  
>> >    do_cleanups (cleanups);
>> > +
>> > +  if (final_pc != NULL)
>> > +    *final_pc = end_pc;
>> > +
>> > +  return num_displayed;
>> >  }
>> >  
>> > -static void
>> > +static int
>> >  do_assembly_only (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >  		  struct disassemble_info * di,
>> >  		  CORE_ADDR low, CORE_ADDR high,
>> > -		  int how_many, int flags, struct ui_file *stb)
>> > +		  int how_many, int flags, struct ui_file *stb,
>> > +		  CORE_ADDR *final_pc)
>> >  {
>> > -  int num_displayed = 0;
>> > +  CORE_ADDR npc;
>> > +  int total = 0;
>> > +
>> > +  do
>> > +    {
>> > +      total += dump_insns (gdbarch, uiout, di, low, high,
>> > +			   how_many, flags, stb, &npc);
>> > +      if (npc >= high || total >= how_many)
>> > +        break;
>> > +      if (low == npc)
>> > +	low = npc + 1;
>> 
>> npc + 1?
>> Perhaps it's correct, but at a glance I don't see how.
>
> No you're not.  This should have more comments.  I think on revision
> I'll remove this change for now.
>
> These disassembler changes were pulled out of an even larger
> disassembler patch that I have had say around for ages.
>
> The addition of +1 both here, and further down are part of addressing
> disassembling over unreadable memory.  The +1 was part of ensuring
> that the disassembler continued to make progress.
>
> With out the supporting changes though the +1 alone does not make a
> lot of sense, my only defence for leaving it in was that it makes the
> gdb side of the disassembler super-paranoid: if for any reason the
> disassembler fails to make progress gdb will still nudge the address
> on.  However, in general gdb is not written that way so I should
> probably drop this change.
>
>>
>> > +      else
>> > +	low = npc;
>> > +    } while (1);
>> 
>> I think(!) convention requires the "while (1);" to go on the next line.
>> 
>> The higher order question, though, is why is this loop needed?
>> IOW, what does the loop do that dump_insns doesn't already do?
>> Am I missing something?
>
> This relates to the above point and should be removed (for now), the
> assumption will be that dump_insns will always cover the requested
> range.
>
>> > +
>> > +  if (final_pc != NULL)
>> > +    *final_pc = npc;
>> >  
>> > -  num_displayed = dump_insns (gdbarch, uiout, di, low, high, how_many,
>> > -                              flags, stb, NULL);
>> > +  return total;
>> >  }
>> >  
>> >  /* Initialize the disassemble info struct ready for the specified
>> > @@ -740,31 +768,72 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
>> >    struct ui_file *stb = mem_fileopen ();
>> >    struct cleanup *cleanups = make_cleanup_ui_file_delete (stb);
>> >    struct disassemble_info di = gdb_disassemble_info (gdbarch, stb);
>> > -  struct symtab *symtab;
>> > -  struct linetable_entry *le = NULL;
>> > -  int nlines = -1;
>> > -
>> > -  /* Assume symtab is valid for whole PC range.  */
>> > -  symtab = find_pc_line_symtab (low);
>> > -
>> > -  if (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL)
>> > -    nlines = SYMTAB_LINETABLE (symtab)->nitems;
>> >  
>> >    /* This cleanup is inner to CLEANUPS, so we don't need a separate
>> >       variable to track it.  */
>> >    (void) make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
>> >  
>> > -  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
>> > -      || nlines <= 0)
>> > -    do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
>> > +  if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE)))
>> > +    (void) do_assembly_only (gdbarch, uiout, &di, low, high,
>> > +			     how_many, flags, stb, NULL);
>> > +  else if (flags & (DISASSEMBLY_SOURCE | DISASSEMBLY_SOURCE_DEPRECATED))
>> > +    {
>> > +      /* We want to disassemble with source information, however, if such
>> > +	 information is not available then we'd rather have raw assembly
>> > +	 than nothing.  Here we loop until the entire range is
>> > +	 disassembled, giving source information where possible.  */
>> > +      while (low < high)
>> > +	{
>> > +	  int nlines = -1;
>> 
>> No need to init nlines to -1 twice.
>> I'd say delete this init.
>
> My mistake.  Will fix.
>
>> 
>> > +	  CORE_ADDR prev_low = low;
>> > +	  struct symtab *symtab;
>> > +	  struct linetable_entry *le = NULL;
>> > +	  int num_displayed = 0;
>> >  
>> > -  else if (flags & DISASSEMBLY_SOURCE)
>> > -    do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab, low, high,
>> > -				  how_many, flags, stb);
>> > +	  /* Attempt to find a symtab for the PC range.  */
>> > +	  nlines = -1;
>> > +	  symtab = find_pc_line_symtab (low);
>> >  
>> > -  else if (flags & DISASSEMBLY_SOURCE_DEPRECATED)
>> > -    do_mixed_source_and_assembly_deprecated (gdbarch, uiout, &di, symtab,
>> > -					     low, high, how_many, flags, stb);
>> > +	  if (symtab != NULL && symtab->linetable != NULL)
>> > +	    {
>> > +	      /* Convert the linetable to a bunch of my_line_entry's.  */
>> > +	      le = SYMTAB_LINETABLE (symtab)->item;
>> > +	      nlines = SYMTAB_LINETABLE (symtab)->nitems;
>> > +
>> > +	      if (flags & DISASSEMBLY_SOURCE)
>> > +		num_displayed +=
>> > +		  do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab,
>> > +						low, high, how_many, flags,
>> > +						stb, &low);
>> 
>> Storing final_pc in low here is awkward.
>> Can you store it in end_pc or some such first?
>
> OK, will rename some of the variable around this area.
>
>> 
>> > +	      else
>> > +		num_displayed +=
>> > +		  do_mixed_source_and_assembly_deprecated (gdbarch, uiout,
>> > +							   &di, symtab,
>> > +							   low, high,
>> > +							   how_many,
>> > +							   flags, stb,
>> > +							   &low);
>> 
>> Ditto.
>> 
>> > +	    }
>> > +
>> > +	  /* Disassemble a single instruction if we couldn't find a
>> > +	     suitable symtab, or if, for any reason, the above disassembly
>> > +	     didn't move the LOW address on at all.  */
>> 
>> I think a reasonable case can be made that this is the wrong thing to
>> do for the deprecated case (based on my observations of how it works,
>> and does not work). Consider that it doesn't print inlined function calls
>> that come from another file. If it doesn't do that, why go to any effort
>> to print non-source-based disassembly in the gaps between source
>> files.
>
> I guess I see the question the other way around; why should we
> maintain a separate way of doing things just to preserve (what I see
> as) a bug?
>
> By placing the choice between the old and new right into the middle of
> my new disassembly control loop both old and new pick up the fix.  To
> do what you suggest I see two choices (I'm open to alternative
> suggestions though):
>
>   1. Move the call to the old, deprecated, function outside of the new
>      disassembly control loop.  We'd probably end up with code
>      duplication, so extra maintenance overhead, just to support a
>      deprecated method.
>
>   2. Add a 'break;' immediately after the call to the deprecated
>      disassembly function (within the else block).  This would ensure
>      we only get once chance to disassemble.  Sure, if that's what it
>      takes to get my patch in I can do that, but it feels kind of
>      petty to me, like we're leaving a bug in place just to force
>      people to move to the new method.

As a counter proposal, let's just get rid of /m,
and make /s the new /m (I'm ok with then getting rid of /s).

>> [Over time I can imagine attempts to try to morph the deprecated
>> case into the new non-deprecated case, i.e., changing /m to be more like /s.
>> How about leaving /m the way it is, and just encourage everyone to
>> use /s.]
>
> Is that really so bad?  I'm going from memory here so I may have the
> history wrong, but isn't the story that you found the inline-functions
> issue with the /m case, looked into fixing it and discovered that
> you'd be better off writing a brand new function, hence /s.
>
> But surely, if someone put forward a patch that "fixed" /m (without
> making it exactly the same as /s) then would that really be so bad?

If someone can make a reasonable case that the intent behind /m output
is usable I could considerate it.
I'm just not in favor of making other code more complicated just
to placate /m.

Btw, there is another patch in the works for refactoring a lot of this.
https://sourceware.org/ml/gdb-patches/2015-09/msg00510.html
et.al.
Heads up.

[PATCH 1/2] gdb: Move common MI code to outer function.

[Andrew Burgess]

The disassembly code has a common entry function gdb_disassembly, and
three handler functions which are called based on what type of
disassembly we are performing.

Each of the handler functions creates an MI list called asm_insns, which
is required for the MI disassembly output.

The commit moves the common asm_insns list to the outer level
gdb_disassembly function, reducing duplicate code.

gdb/ChangeLog:

	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
	asm_insns list.
	(do_mixed_source_and_assembly): Likewise.
	(do_assembly_only): Likewise.
	(gdb_disassembly): Create asm_insns list here.
---
 gdb/ChangeLog |  8 ++++++++
 gdb/disasm.c  | 22 ++++++----------------
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9dd591c..463b1b9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2015-08-30  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
+	asm_insns list.
+	(do_mixed_source_and_assembly): Likewise.
+	(do_assembly_only): Likewise.
+	(gdb_disassembly): Create asm_insns list here.
+
 2015-08-29  Doug Evans  <xdje42@gmail.com>
 
 	* symtab.h (struct symbol): Tweak comment.
diff --git a/gdb/disasm.c b/gdb/disasm.c
index 2b65c6a..3032090 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -291,7 +291,6 @@ do_mixed_source_and_assembly_deprecated
   int next_line = 0;
   int num_displayed = 0;
   enum print_source_lines_flags psl_flags = 0;
-  struct cleanup *ui_out_chain;
   struct cleanup *ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
   struct cleanup *ui_out_list_chain = make_cleanup (null_cleanup, 0);
 
@@ -355,8 +354,6 @@ do_mixed_source_and_assembly_deprecated
      they have been emitted before), followed by the assembly code
      for that line.  */
 
-  ui_out_chain = make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
-
   for (i = 0; i < newlines; i++)
     {
       /* Print out everything from next_line to the current line.  */
@@ -429,7 +426,6 @@ do_mixed_source_and_assembly_deprecated
       if (how_many >= 0 && num_displayed >= how_many)
 	break;
     }
-  do_cleanups (ui_out_chain);
 }
 
 /* The idea here is to present a source-O-centric view of a
@@ -453,7 +449,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
   int num_displayed = 0;
   enum print_source_lines_flags psl_flags = 0;
   struct cleanup *cleanups;
-  struct cleanup *ui_out_chain;
   struct cleanup *ui_out_tuple_chain;
   struct cleanup *ui_out_list_chain;
   CORE_ADDR pc;
@@ -509,7 +504,8 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 
      Output format, from an MI perspective:
        The result is a ui_out list, field name "asm_insns", where elements have
-       name "src_and_asm_line".
+       name "src_and_asm_line".  The outer "asm_insns" list is created in
+       the outer function gdb_disassembly.
        Each element is a tuple of source line specs (field names line, file,
        fullname), and field "line_asm_insn" which contains the disassembly.
        Field "line_asm_insn" is a list of tuples: address, func-name, offset,
@@ -522,8 +518,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
      cleanups:
        For things created at the beginning of this function and need to be
        kept until the end of this function.
-     ui_out_chain
-       Handles the outer "asm_insns" list.
      ui_out_tuple_chain
        The tuples for each group of consecutive disassemblies.
      ui_out_list_chain
@@ -532,8 +526,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
   if (flags & DISASSEMBLY_FILENAME)
     psl_flags |= PRINT_SOURCE_LINES_FILENAME;
 
-  ui_out_chain = make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
-
   ui_out_tuple_chain = NULL;
   ui_out_list_chain = NULL;
 
@@ -683,7 +675,6 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
       last_line = sal.line;
     }
 
-  do_cleanups (ui_out_chain);
   do_cleanups (cleanups);
 }
 
@@ -694,14 +685,9 @@ do_assembly_only (struct gdbarch *gdbarch, struct ui_out *uiout,
 		  int how_many, int flags, struct ui_file *stb)
 {
   int num_displayed = 0;
-  struct cleanup *ui_out_chain;
-
-  ui_out_chain = make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
 
   num_displayed = dump_insns (gdbarch, uiout, di, low, high, how_many,
                               flags, stb, NULL);
-
-  do_cleanups (ui_out_chain);
 }
 
 /* Initialize the disassemble info struct ready for the specified
@@ -764,6 +750,10 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
   if (symtab != NULL && SYMTAB_LINETABLE (symtab) != NULL)
     nlines = SYMTAB_LINETABLE (symtab)->nitems;
 
+  /* This cleanup is inner to CLEANUPS, so we don't need a separate
+     variable to track it.  */
+  (void) make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
+
   if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
       || nlines <= 0)
     do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
-- 
2.5.1

Re: [PATCH 1/2] gdb: Move common MI code to outer function.

[Doug Evans]

Andrew Burgess <andrew.burgess@embecosm.com> writes:
> The disassembly code has a common entry function gdb_disassembly, and
> three handler functions which are called based on what type of
> disassembly we are performing.
>
> Each of the handler functions creates an MI list called asm_insns, which
> is required for the MI disassembly output.
>
> The commit moves the common asm_insns list to the outer level
> gdb_disassembly function, reducing duplicate code.
>
> gdb/ChangeLog:
>
> 	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
> 	asm_insns list.
> 	(do_mixed_source_and_assembly): Likewise.
> 	(do_assembly_only): Likewise.
> 	(gdb_disassembly): Create asm_insns list here.

Hi.

fwiw, I've got reservations about this cleanup.
Each of these do_* functions is tasked with emitting
the disassembly, and splitting out "asm_insns" makes
them no longer self-contained as far as what is emitted goes
(feels like a net loss in readability).

If someone else wants to approve this, go for it.

> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 2b65c6a..3032090 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -291,7 +291,6 @@ do_mixed_source_and_assembly_deprecated
>    int next_line = 0;
>    int num_displayed = 0;
>    enum print_source_lines_flags psl_flags = 0;
> -  struct cleanup *ui_out_chain;
>    struct cleanup *ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
>    struct cleanup *ui_out_list_chain = make_cleanup (null_cleanup, 0);
>  

btw, I think(!) the initial null_cleanups in
do_mixed_source_and_assembly_deprecated is wrong,
but it's been like that for awhile so maybe it's harmless.
I don't have time to dig deeper, just filing this as fyi.

Re: [PATCH 1/2] gdb: Move common MI code to outer function.

[Andrew Burgess]

* Doug Evans <xdje42@gmail.com> [2015-09-16 19:54:14 -0700]:

> Andrew Burgess <andrew.burgess@embecosm.com> writes:
> > The disassembly code has a common entry function gdb_disassembly, and
> > three handler functions which are called based on what type of
> > disassembly we are performing.
> >
> > Each of the handler functions creates an MI list called asm_insns, which
> > is required for the MI disassembly output.
> >
> > The commit moves the common asm_insns list to the outer level
> > gdb_disassembly function, reducing duplicate code.
> >
> > gdb/ChangeLog:
> >
> > 	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
> > 	asm_insns list.
> > 	(do_mixed_source_and_assembly): Likewise.
> > 	(do_assembly_only): Likewise.
> > 	(gdb_disassembly): Create asm_insns list here.
> 
> Hi.
> 
> fwiw, I've got reservations about this cleanup.
> Each of these do_* functions is tasked with emitting
> the disassembly, and splitting out "asm_insns" makes
> them no longer self-contained as far as what is emitted goes
> (feels like a net loss in readability).

I think this patch change would be required if the second patch in the
series is going to make it in.

For now lets ignore the deprecated case.

We basically have two disassemble methods, one is raw instructions,
the other is instructions with source.

In the pre-patch source the top level function looks up helper
information symtab, etc then forwards the disassemble request to the
correct helper.

In the post-patch source the control flow keeps returning to the top
level function each time it reaches the end of a symtab in order to
discover the "next" symtab.

Given that we want the complete disassembly to be nested under a
single "asm_insns" that would need to be created at the top level (in
gdb_disassembly)

[ OK, we could probably come up with a scheme where the "asm_insns" is
  only conditionally created in the helper functions, but that doesn't
  seem better to me. ]

> If someone else wants to approve this, go for it.

I'd rather try and convince you :)

I'm going to take a look through your review of patch #2 next, but at
a glance you didn't seem to be rejecting it.  So, my question, do you
have any suggestions for addressing the above issue?

I'm happy to settle for patch #1 being approved only if patch #2 is
also approved.

> > diff --git a/gdb/disasm.c b/gdb/disasm.c
> > index 2b65c6a..3032090 100644
> > --- a/gdb/disasm.c
> > +++ b/gdb/disasm.c
> > @@ -291,7 +291,6 @@ do_mixed_source_and_assembly_deprecated
> >    int next_line = 0;
> >    int num_displayed = 0;
> >    enum print_source_lines_flags psl_flags = 0;
> > -  struct cleanup *ui_out_chain;
> >    struct cleanup *ui_out_tuple_chain = make_cleanup (null_cleanup, 0);
> >    struct cleanup *ui_out_list_chain = make_cleanup (null_cleanup, 0);
> >  
> 
> btw, I think(!) the initial null_cleanups in
> do_mixed_source_and_assembly_deprecated is wrong,
> but it's been like that for awhile so maybe it's harmless.
> I don't have time to dig deeper, just filing this as fyi.

It does indeed seem like a very strange use of cleanups.


Thanks,
Andrew

Re: [PATCH 1/2] gdb: Move common MI code to outer function.

[Doug Evans]

Andrew Burgess <andrew.burgess@embecosm.com> writes:
> * Doug Evans <xdje42@gmail.com> [2015-09-16 19:54:14 -0700]:
>
>> Andrew Burgess <andrew.burgess@embecosm.com> writes:
>> > The disassembly code has a common entry function gdb_disassembly, and
>> > three handler functions which are called based on what type of
>> > disassembly we are performing.
>> >
>> > Each of the handler functions creates an MI list called asm_insns, which
>> > is required for the MI disassembly output.
>> >
>> > The commit moves the common asm_insns list to the outer level
>> > gdb_disassembly function, reducing duplicate code.
>> >
>> > gdb/ChangeLog:
>> >
>> > 	* disasm.c (do_mixed_source_and_assembly_deprecated): Remove
>> > 	asm_insns list.
>> > 	(do_mixed_source_and_assembly): Likewise.
>> > 	(do_assembly_only): Likewise.
>> > 	(gdb_disassembly): Create asm_insns list here.
>> 
>> Hi.
>> 
>> fwiw, I've got reservations about this cleanup.
>> Each of these do_* functions is tasked with emitting
>> the disassembly, and splitting out "asm_insns" makes
>> them no longer self-contained as far as what is emitted goes
>> (feels like a net loss in readability).
>
> I think this patch change would be required if the second patch in the
> series is going to make it in.
>
> For now lets ignore the deprecated case.
>
> We basically have two disassemble methods, one is raw instructions,
> the other is instructions with source.
>
> In the pre-patch source the top level function looks up helper
> information symtab, etc then forwards the disassemble request to the
> correct helper.
>
> In the post-patch source the control flow keeps returning to the top
> level function each time it reaches the end of a symtab in order to
> discover the "next" symtab.
>
> Given that we want the complete disassembly to be nested under a
> single "asm_insns" that would need to be created at the top level (in
> gdb_disassembly)

Ok, fair enough.
At least this is all internal to disasm.c.