Re: [PATCH] arc: Add support for Linux coredump files

[Shahab Vahedi via Gdb-patches <gdb-patches@sourceware.org>]

Hi Baris, Simon,

On Mon, Sep 28, 2020 at 03:10:23PM -0400, Simon Marchi wrote:
> On 2020-09-28 10:08 a.m., Aktemur, Tankut Baris wrote:
> >
> > Just for the record, there are similar uses in linux-aarch32-low.cc,
> > linux-arm-low.cc, linux-low.cc, linux-sparc-low.cc, and win32-low.cc (I don't
> > see it in linux-riscv-low.cc), and in all of these uses the context is a function
> > (not a method) where the 'this' pointer is not available.

I've looked into those files in master branch [1] which at the time
was at commit 9aed480c3a7. The relevant results are:

--------
==> linux-aarch32-low.relevant_grep_results <==
      the_target->read_memory (where, (unsigned char *) &insn, 2);
	  the_target->read_memory (where + 2, (unsigned char *) &insn, 2);
      the_target->read_memory (where, (unsigned char *) &insn, 4);
      if (target_read_memory (*pcptr, buf, 2) == 0)
	  target_read_memory (*pcptr, (gdb_byte *) &inst1, 2);

==> linux-arm-low.relevant_grep_results <==
  target_read_memory (memaddr, (unsigned char *) &res, len);
  the_target->read_memory (sp, (unsigned char *) &sp_data, 4);
  the_target->read_memory (sp + pc_offset, (unsigned char *) &next_pc, 4);
  the_target->read_memory (sp + pc_offset + 4, (unsigned char *) &cpsr, 4);
      target_read_memory (pc, (unsigned char *) &this_instr, 4);
      if (read_memory (pc - 4, (unsigned char *) &insn, 4))

==> linux-low.relevant_grep_results <==
  return the_target->read_memory (memaddr, myaddr, len);

==> linux-riscv-low.relevant_grep_results <==
  if (target_read_memory (*pcptr, buf.bytes, sizeof (buf.insn)) == 0
  if (target_read_memory (pc, buf.bytes, sizeof (buf.insn)) == 0
	      && target_read_memory (pc + sizeof (buf.insn), buf.bytes,

==> linux-sparc-low.relevant_grep_results <==
      the_target->read_memory (addr, tmp_reg_buf, sizeof (tmp_reg_buf));
  read_memory (where, (unsigned char *) insn, sizeof (insn));

==> win32-low.relevant_grep_results <==
--------

First, I am confused, because apparently there are 3 ways to
skin a target's memory [2]:

the_target->read_memory()             8 occurrences
target_read_memory()                  7 occurrences
read_memory()                         2 occurrences

"the_target->read_memory()" and "read_memory()" should be the
same in terms of code execution (not code writing). Then,
there is "target_read_memory()" which I think is the default
implementation [3].

Maybe it is my style, but I dislike hunting the implementation
of a pure virtual method somewhere along the inheritance line
(lineage? :p). Since ARC is not overriding the "read_memory()",
it is more meaningful to use the "target_read_memory()". I will
be changing "the_target->read_memory()" to that then.

[1] Master branch at commit 9aed480c3a7
https://sourceware.org/git/?p=binutils-gdb.git;hb=HEAD;a=tree

[2]
To be clear, what I mean is "There are 3 flavors". I just couldn't
leave this joke alone.

[3]
"process_stratum_target" class (the base) provides
"read_memory()" as pure virtual method. Therefore, it must be defined
overridden somewhere and I _think_ this is the default implementation.

> FWIW, I'm leaning towards what Baris says, better use "this" than the
> global variable.  And I even prefer to make it explicit,
> "this->read_memory (...", to make it clear that we call a method and not
> a free function.  But I don't want to start an endless discussion about
> coding style :).

Please see my answer above.