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 2/6] Refactor disassembly code


On 01/18/2017 10:33 AM, Yao Qi wrote:
On 17-01-17 08:14:27, Luis Machado wrote:

/* Like target_read_memory, but slightly different parameters.  */

Should we update these comments to a more useful version? "slightly
different parameters" doesn't explain much, as it is obvious they
are slightly different. Other uses below have the same problem.

How about this?

  /* Wrapper of target_read_code.  */


I think that's better than the previous one.

+int
+gdb_disassembler::print_insn (CORE_ADDR memaddr,
+			      int *branch_delay_insns)
+{
+  int length = gdbarch_print_insn (arch (), memaddr, &m_di);
+
+  if (branch_delay_insns != NULL)
+    {
+      if (m_di.insn_info_valid)
+	*branch_delay_insns = m_di.branch_delay_insns;
+      else
+	*branch_delay_insns = 0;
+    }
+  return length;

length doesn't seem to have a purpose other than being returned. So
just return gdbarch_print_insn (arch (), memaddr, &m_di)?


branch_delay_insns needs update after gdbarch_print_insn call.


Indeed. Missed the m_di.

+
+  /* Prints the instruction INSN into UIOUT and returns the length of
+     the printed instruction in bytes.  */
+  int pretty_print_insn (struct ui_out *uiout,
+			 const struct disasm_insn *insn, int flags);

Can this function return negative? If not, use unsigned?


I don't think pretty_print_insn can return negative, and we can change
it to size_t.  However, this patch is a code refactor.  I want to avoid
change like this in this patch.  I can change 'int' to 'size_t' later.


Sounds good.

+
+  /* Return the gdbarch of gdb_disassembler.  */
+  struct gdbarch *arch ()
+  { return m_gdbarch; }
+
+protected:
+  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file,
+		    di_read_memory_ftype func);
+
+  struct ui_file *stream ()
+  { return (struct ui_file *) m_di.stream; }
+
+private:
+  struct gdbarch *m_gdbarch;
+  struct disassemble_info m_di;

Add comments explaining what m_gdbarch and m_di are used for and/or how?

I don't know what kind of comments I can add for m_gdbarch.  Something
like "gdbarch of gdb_disassembler" looks useless.  We've already had

I can still see value in describing it as a "Pointer to the target's gdbarch". But feel free not to document it if you don't think it's worth it.

The documentation is likely not for us (experienced gdb developers), but for people getting started on gdb's code.

method arch(with comments) which returns m_gdbarch.

I can think of the comments to m_di

  /* Pass it to opcodes disassembler and collect some outputs
     from opcodes disassembler.  */

  struct disassemble_info m_di;

What do you think?


I think something along the lines of:

/* Stores disassembler data.  */

or

/* Stores data required for disassembling instructions.  */

... is good enough, as trivial as it may seem.


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