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

[Simon Marchi <simark@simark.ca>]

On 2020-09-29 4:24 a.m., Shahab Vahedi wrote:
> 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].

target_read_memory isn't the default implementation of
process_stratum_target::read_memory.

target_read_memory calls read_inferior_memory, which calls the target's
read_memory method, and then puts back in place the instructions
shadowed by breakpoints and fast tracepoint jumps inserted by GDBserver
in place.  This is so that the memory returned to the caller (and
possibly to GDB) contains the contents it expects to see.

It also exists because it implements the "target" interface in
gdb/target/target.h interface, which was meant to provide a common
interface between GDB and GDBserver.  Presumably, this allows calling it
from common code.  Although, I'm not sure whether having this common
interface was ever really useful...

`read_memory (...)` is the same as `this->read_memory (...)`, it calls
process_stratum_target::read_memory on the current target object.  The
concrete implementation of this method contains calls to the platform's
API to fetch the requested bytes from the process.  Like I said before I
personally prefer the second version just because it makes it clear
you're not calling a free function (especially that we a `read_memory`
free function in GDB, so it may be confusing).

`the_target->read_memory (...)` is the also the same as
`this->read_memory (...)` when we are already in the method of a target.
When we are already in such a context, I don't really see the point of
using `the_target`, as it just adds more unnecessary references to a
global variable.  It could make the reader wonder "oh, are we calling
read_memory on some other object?" before they realize that the_target
is the same as this.  So, it's not a big deal for me, but I just find it
expresses the intention a bit better.

So the right thing to call between target_read_memory /
read_inferior_memory and process_stratum_target::read_memory depends if
you need the breakpoint shadowing replacement feature or not.  I presume
that in many contexts, it won't make a difference.

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

I don't understand what you mean.  target_read_memory ends up calling
the_target->read_memory (and it does the breakpoint shadowing stuff on
top).  So those calls are _not_ equivalent, and they both end up calling
the concrete implementation of the pure virtual method.  So I don't
understand what you win here.

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

No it isn't.  There are three implementations of
process_stratum_target::read_memory that I and my IDE know of:

    $ ag '::read_memory'
    linux-low.cc
    5522:linux_process_target::read_memory (CORE_ADDR memaddr,

    win32-low.cc
    1675:win32_process_target::read_memory (CORE_ADDR memaddr, unsigned char *myaddr,

    netbsd-low.cc
    554:netbsd_process_target::read_memory (CORE_ADDR memaddr, unsigned char *myaddr,

As I said previously, the implementation of
process_stratum_target::read_memory does calls to the native platform's
API to fetch the bytes from the process' memory.  These three platforms
(linux, win32 and netbsd) all have different ways of doing it, hence the
three implementations.

The linux one offers an implementation that is re-used by all the final
linux targets (aarch64_target, x86_target, etc) because it is the same
on all Linux systems, regardless of the architecture.

Simon