From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 8DA603857C7F for ; Tue, 29 Sep 2020 14:22:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8DA603857C7F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id F0EEA1E58E; Tue, 29 Sep 2020 10:22:18 -0400 (EDT) Subject: Re: [PATCH] arc: Add support for Linux coredump files To: Shahab Vahedi , "Aktemur, Tankut Baris" Cc: "gdb-patches@sourceware.org" , Anton Kolesov , Shahab Vahedi , Francois Bedard References: <20200827112728.4275-1-shahab.vahedi@gmail.com> <18b98a56-e3cf-e05b-49a7-bd6e1f61aefb@simark.ca> <20200928134714.GA4717@gmail.com> <20200929082456.GB4717@gmail.com> From: Simon Marchi Message-ID: <81e2fb09-4d5f-a274-8986-2726a45d4290@simark.ca> Date: Tue, 29 Sep 2020 10:22:18 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20200929082456.GB4717@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Sep 2020 14:22:21 -0000 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