Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> Suraj Jitindar Singh <address@hidden> writes:
> 
> > Add a monitor command "dump-stack" to be used to dump the stack for the
> > current cpu.
> 
> I guess this is just for debugging.  Correct?
> 
> Shouldn't this be "info stack", to match "info registers" and "info
> cpustats"?

Since this is primarily about walking the guests stack frames and not
walking qemu internal data structures, I think it's probably OK to be
a dump-stack rather than an info subcommand.

Dave

> >
> > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > ---
> >  hmp-commands.hx   | 13 +++++++++++++
> >  hmp.h             |  1 +
> >  include/qom/cpu.h | 10 ++++++++++
> >  monitor.c         | 12 ++++++++++++
> >  qom/cpu.c         | 10 ++++++++++
> >  5 files changed, 46 insertions(+)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 9b4035965c..965ccdea28 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -862,6 +862,19 @@ ETEXI
> >      },
> >  
> >  STEXI
> > address@hidden dump-stack
> > address@hidden dump-stack
> > +dump stack of the cpu
> > +ETEXI
> > +    {
> > +        .name           = "dump-stack",
> > +        .args_type      = "",
> > +        .params         = "",
> > +        .help           = "dump stack",
> > +        .cmd            = hmp_dumpstack,
> > +    },
> > +
> > +STEXI
> >  @item pmemsave @var{addr} @var{size} @var{file}
> >  @findex pmemsave
> >  save to disk physical memory dump starting at @var{addr} of size 
> > @var{size}.
> > diff --git a/hmp.h b/hmp.h
> > index 43617f2646..e6edf1215c 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
> >  void hmp_cpu(Monitor *mon, const QDict *qdict);
> >  void hmp_memsave(Monitor *mon, const QDict *qdict);
> >  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> > +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
> >  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
> >  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
> >  void hmp_cont(Monitor *mon, const QDict *qdict);
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 08abcbd3fe..f2e83e9918 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -181,6 +181,7 @@ typedef struct CPUClass {
> >      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
> >                             uint8_t *buf, int len, bool is_write);
> >      void (*dump_state)(CPUState *cpu, FILE *, int flags);
> > +    void (*dump_stack)(CPUState *cpu, FILE *f);
> >      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
> >      void (*dump_statistics)(CPUState *cpu, int flags);
> >      int64_t (*get_arch_id)(CPUState *cpu);
> > @@ -568,6 +569,15 @@ enum CPUDumpFlags {
> >  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> >  
> >  /**
> > + * cpu_dump_stack:
> > + * @cpu: The CPU whose stack is to be dumped.
> > + * @f: If non-null, dump to this stream, else to current print sink.
> > + *
> > + * Dumps CPU stack.
> > + */
> > +void cpu_dump_stack(CPUState *cpu, FILE *f);
> > +
> > +/**
> >   * cpu_dump_statistics:
> >   * @cpu: The CPU whose state is to be dumped.
> >   * @flags: Flags what to dump.
> > diff --git a/monitor.c b/monitor.c
> > index 9b5f10b475..dbec2e4376 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const 
> > QDict *qdict)
> >      }
> >  }
> >  
> > +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
> > +{
> > +    CPUState *cs = mon_get_cpu();
> > +
> > +    if (!cs) {
> > +        monitor_printf(mon, "No CPU available\n");
> > +        return;
> > +    }
> > +
> > +    cpu_dump_stack(cs, NULL);
> > +}
> > +
> >  #ifdef CONFIG_TCG
> >  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
> >  {
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index 3c5493c96c..0dc10004f4 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
> >      }
> >  }
> >  
> > +void cpu_dump_stack(CPUState *cpu, FILE *f)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +
> > +    if (cc->dump_stack) {
> > +        cpu_synchronize_state(cpu);
> > +        cc->dump_stack(cpu, f);
> > +    }
> > +}
> 
> Silently does nothing when the CPU doesn't implement dump_stack().
> Matches "info registers" and "info cpustats".  Awful UI, but at least
> it's consistently awful.
> 
> > +
> >  void cpu_dump_statistics(CPUState *cpu, int flags)
> >  {
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK