diff mbox

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

Message ID dfbf766b7443265338ad1c6a87e774a7f58509e5.1440934890.git.andrew.burgess@embecosm.com
State New, archived
Headers

Commit Message

Andrew Burgess Aug. 30, 2015, 11:48 a.m. UTC 
  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(-)

Comments

Doug Evans Sept. 17, 2015, 2:54 a.m. UTC | #1 
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.
Andrew Burgess Sept. 17, 2015, 10:02 a.m. UTC | #2 
* 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
Doug Evans Oct. 5, 2015, 4:19 a.m. UTC | #3 
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.
diff mbox

Patch

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);