[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