This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH v2 3/4] btrace: change record instruction-history /m


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]