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 v4 1/2] Add back gdb_pretty_print_insn


On 2017-02-01 13:09, Simon Marchi wrote:
On 2017-01-31 19:30, Pedro Alves wrote:
ui_file_rewind is a ui_file method that only really works with mem
buffer files, and is a nop on other ui_file types.  It'd be desirable
to eliminate it from the base ui_file interface, and move it to the
"mem_fileopen" subclass of ui_file instead.  A following patch does
just that.

Unfortunately, there are a couple references to ui_file_rewind inside
gdb_disassembler::pretty_print_insn that were made harder to eliminate
with the recent addition of the gdb_disassembler wrapper.

Before the gdb_disassembler wrapper was added, in commit
e47ad6c0bd7aa3 ("Refactor disassembly code"), gdb_pretty_print_insn
used to be passed a ui_file pointer as argument, and it was simple to
adjust that pointer be a "mem_fileopen" ui_file pointer instead, since
there's only one gdb_pretty_print_insn caller.

That commit made gdb_pretty_print_insn be a method of
gdb_disassembler, and removed the method's ui_file parameter at the
same time, replaced by referencing the gdb_disassembler's stream
instead.  The trouble is that a gdb_disassembler can be instantiated
with a pointer any kind of ui_file.  Casting the gdb_disassembler's
stream to a mem_fileopen ui_file inside
gdb_disassembler::pretty_print_insn in order to call the reset method
would be gross hack.

The fix here is to:

 - make gdb_disassembler::pretty_print_insn a be free function again
   instead of a method of gdb_disassembler.  I.e., bring back
   gdb_pretty_print_insn.

 - but, don't add back the ui_file * parameter.  We'd always be
   passing in a fresh mem_fileopen anyway, so move the mem_fileopen
   allocation inside.  That is a better interface, given that the
   ui_file is only ever used as temporary scratch buffer as an
   implementation detail of gdb_pretty_print_insn.  The function's
   real "where to send output" parameter is the ui_out pointer.

 - don't add back a disassemble_info pointer either.  That used to be
   necessary for this bit:

	  err = m_di.read_memory_func (pc, &data, 1, &m_di);
	  if (err != 0)
	    m_di.memory_error_func (err, pc, &m_di);

   ... but AFAIK, it's not really necessary.  We can replace those
   three lines with a call to read_code.  This seems to fix a
   regression even, because before commit d8b49cf0c891d0 ("Don't throw
   exception in dis_asm_memory_error"), that memory_error_func call
   would throw an error/exception, but now it only records the error
   in the gdb_disassembler's m_err_memaddr field.  (read_code throws
   on error.)

With all these, gdb_pretty_print_insn is completely layered on top of
gdb_disassembler only using the latter's public API.

I don't think I understand the situation fully, but what you suggest
looks good to me.  I was confused by the fact that the
gdb_disassembler constructor accepts a stream, but the
pretty_print_insn method takes a uiout.  Which one is used for
printing then?  I think that your patch clears that up.

The only possible issue I can see is that in your version, one
gdb_disassembler and one string_file object are constructed for each
disassembled instruction, rather than re-using them for as long as we
need to disassemble.  I don't know how much impact it has on the
performance (probably negligible), but something to keep in mind.

I'll just mention it so we hopefully don't duplicate work, looking at your patch has prompted me to start a patch adding a "disassembly_flags" enum type.

https://github.com/simark/binutils-gdb/tree/disas-flags


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