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 2E9CE3857004 for ; Mon, 7 Sep 2020 19:58:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2E9CE3857004 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 C24001E509; Mon, 7 Sep 2020 15:58:18 -0400 (EDT) Subject: Re: [PATCH v2 10/10] Add minimal and functional NetBSD/amd64 gdbserver To: Kamil Rytarowski , gdb-patches@sourceware.org Cc: tom@tromey.com References: <20200904002905.13616-1-n54@gmx.com> <20200904002905.13616-11-n54@gmx.com> From: Simon Marchi Message-ID: <071c608e-0709-6fff-cd42-4981b3d0bdb6@simark.ca> Date: Mon, 7 Sep 2020 15:58: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: <20200904002905.13616-11-n54@gmx.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.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: Mon, 07 Sep 2020 19:58:21 -0000 I did a superficial review, that's as much as I can, since I can't easily test this. I also don't know gdbserver by heart, so I can't tell off the top of my head if some method implementation doesn't match what's expected, for example. So I checked for obvious mistakes (didn't find any) and coding standards (looks pretty good). Since this is new code, I am not too worried, it won't break any existing use case. The worst that can happen is that there are bugs and we fix them later. On 2020-09-03 8:29 p.m., Kamil Rytarowski wrote: > Implement the following functionality: create_inferior, > post_create_inferior, attach, kill, detach, mourn, join, thread_alive, > resume, wait, fetch_registers, store_registers, read_memory, write_memory, > request_interrupt, supports_read_auxv, read_auxv, > supports_hardware_single_step, sw_breakpoint_from_kind, > supports_z_point_type, insert_point, remove_point, > stopped_by_sw_breakpoint, supports_qxfer_siginfo, qxfer_siginfo, > supports_stopped_by_sw_breakpoint, supports_non_stop, > supports_multi_process, supports_fork_events, supports_vfork_events, > supports_exec_events, supports_disable_randomization, > supports_qxfer_libraries_svr4, qxfer_libraries_svr4, > supports_pid_to_exec_file, pid_to_exec_file, thread_name, > supports_catch_syscall. > > The only CPU architecture supported: x86_64. > > Implement only support for hardware assisted single step and > software breakpoint. > > Implement support only for regular X86 registers, thus no FPU. > > gdbserver/ChangeLog: > > * netbsd-low.cc: Add. > * netbsd-low.h: Likewise. > * netbsd-x86_64-low.cc: Likewise. Let's name this new file netbsd-amd64-low.cc. We use amd64 everywhere, so it's better to stay consistent. > +/* Implement the fetch_registers target_ops method. */ > + > +void > +netbsd_process_target::fetch_registers (struct regcache *regcache, int regno) > +{ > + struct netbsd_regset_info *regset = netbsd_target_regsets; > + ptid_t inferior_ptid = ptid_of (current_thread); > + > + while (regset->size >= 0) > + { > + char *buf = (char *) xmalloc (regset->size); > + int res = ptrace (regset->get_request, inferior_ptid.pid (), buf, > + inferior_ptid.lwp ()); > + if (res == -1) > + perror_with_name (("ptrace")); > + regset->store_function (regcache, buf); > + free (buf); The perror would leak `buf`. Use some automatic memory management type. > + regset++; > + } > +} > + > +/* Implement the store_registers target_ops method. */ > + > +void > +netbsd_process_target::store_registers (struct regcache *regcache, int regno) > +{ > + struct netbsd_regset_info *regset = netbsd_target_regsets; > + ptid_t inferior_ptid = ptid_of (current_thread); > + > + while (regset->size >= 0) > + { > + char *buf = (char *)xmalloc (regset->size); > + int res = ptrace (regset->get_request, inferior_ptid.pid (), buf, > + inferior_ptid.lwp ()); > + if (res == -1) > + perror_with_name (("ptrace")); > + if (res == 0) > + { > + /* Then overlay our cached registers on that. */ > + regset->fill_function (regcache, buf); > + /* Only now do we write the register set. */ > + res = ptrace (regset->set_request, inferior_ptid.pid (), buf, > + inferior_ptid.lwp ()); > + } > + free (buf); Same here. > +/* Extract &phdr and num_phdr in the inferior. Return 0 on success. */ > + > +template > +int get_phdr_phnum_from_proc_auxv (const pid_t pid, > + CORE_ADDR *phdr_memaddr, int *num_phdr) > +{ > + typedef typename std::conditional + Aux64Info, Aux32Info>::type auxv_type; > + const size_t auxv_size = sizeof (auxv_type); > + const size_t auxv_buf_size = 128 * sizeof (auxv_type); > + > + char *auxv_buf = (char *) xmalloc (auxv_buf_size); > + > + netbsd_read_auxv (pid, nullptr, auxv_buf, auxv_buf_size); > + > + *phdr_memaddr = 0; > + *num_phdr = 0; > + > + for (char *buf = auxv_buf; buf < (auxv_buf + auxv_buf_size); buf += auxv_size) > + { > + auxv_type *const aux = (auxv_type *) buf; > + > + switch (aux->a_type) > + { > + case AT_PHDR: > + *phdr_memaddr = aux->a_v; > + break; > + case AT_PHNUM: > + *num_phdr = aux->a_v; > + break; > + } > + > + if (*phdr_memaddr != 0 && *num_phdr != 0) > + break; > + } > + > + xfree (auxv_buf); Same here. > +struct regcache; > +struct target_desc; > + > +/* Some information relative to a given register set. */ > + > +struct netbsd_regset_info > +{ > + /* The ptrace request needed to get/set registers of this set. */ > + int get_request, set_request; > + /* The size of the register set. */ > + int size; > + /* Fill the buffer BUF from the contents of the given REGCACHE. */ > + void (*fill_function) (struct regcache *regcache, char *buf); > + /* Store the register value in BUF in the given REGCACHE. */ > + void (*store_function) (struct regcache *regcache, const char *buf); I find the distinction between "fill" and "store" unclear, it doesn't really tell which direction it's going (looking at the types helps though). fill the regcache? fill the buffer? store in the regcache? store in the buffer? It's the same with the regcache::supply_regset and regcache::collect_regset methods in GDB... I never know what supply and collect mean. Are they from the point of view of the regset? Or from the point of view of the user of the regset? What about: void (*buf_to_regcache) (struct regcache *regcache, char *buf); void (*regcache_to_buf) (struct regcache *regcache, const char *buf); You could add the `_function` suffix if you prefer, though I don't think it's necessary. Simon