This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH v2 3/4] btrace: change record instruction-history /m
- From: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- To: Doug Evans <xdje42 at gmail dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, "palves at redhat dot com" <palves at redhat dot com>
- Date: Mon, 26 Oct 2015 08:17:26 +0000
- Subject: RE: [PATCH v2 3/4] btrace: change record instruction-history /m
- Authentication-results: sourceware.org; auth=none
- References: <1445246610-11645-1-git-send-email-markus dot t dot metzger at intel dot com> <1445246610-11645-4-git-send-email-markus dot t dot metzger at intel dot com> <86y4eqczok dot fsf at sspiff dot org>
> -----Original Message-----
> From: Doug Evans [mailto:xdje42@gmail.com]
> Sent: Monday, October 26, 2015 12:10 AM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org; palves@redhat.com
> Subject: Re: [PATCH v2 3/4] btrace: change record instruction-history /m
Hi Doug,
Thanks for your review.
> > +/* Print source lines in LINES. */
>
> UI_ITEM needs to be documented, use of cleanups like this
> is certainly atypical.
> Plus the name is confusing (sorry!). I read "ui_item" and the last thing
> I think of is it being a cleanup. ui_item_cleanup?
> Or ui_item_chain which would be more consistent with the use in disasm.c.
I changed it to ui_item_chain and documented the way it is used.
> > + for (line = lines.begin; line < lines.end; ++line)
> > + {
> > + if (*ui_item != NULL)
> > + do_cleanups (*ui_item);
> > +
> > + *ui_item
> > + = make_cleanup_ui_out_tuple_begin_end (uiout,
> "src_and_asm_line");
> > +
> > + print_source_lines (lines.symtab, line, line + 1, psl_flags);
> > +
> > + make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
>
> I think I understand what's going on here.
> If we're printing multiple source lines that don't have insns
> associated with them we still have to include in the output
> line_asm_insn lists, even if they're empty.
> It's a bit subtle though: the second,third,... times through the
> loop we'll immediately run the cleanup set up by
> make_cleanup_ui_out_list_begin_end, and I was left scratching my
> head for a moment. The last time through the loop
> we'll leave the line_asm_insn list open, so that we can add
> disassembled insns to the list. IWBN to explain this to the reader.
That's exactly how it works. I added a detailed comment to
btrace_print_lines and a brief comment to btrace_insn_history.
I'm waiting for your feedback on a better name for gdb_print_insn_tuple
before sending out v3.
Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928