From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200]) by sourceware.org (Postfix) with ESMTPS id 66F0B3857C46 for ; Tue, 8 Sep 2020 00:06:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 66F0B3857C46 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=netbsd.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kamil@netbsd.org Received: from [IPv6:::1] (localhost [127.0.0.1]) by mail.netbsd.org (Postfix) with ESMTP id E968784CCD; Tue, 8 Sep 2020 00:06:50 +0000 (UTC) To: Simon Marchi , gdb-patches@sourceware.org Cc: tom@tromey.com References: <20200904002905.13616-1-n54@gmx.com> <20200904002905.13616-11-n54@gmx.com> <071c608e-0709-6fff-cd42-4981b3d0bdb6@simark.ca> From: Kamil Rytarowski Autocrypt: addr=kamil@netbsd.org; keydata= mQINBFVwUF8BEADHmOg7PFLIcSDdMx5HNDYr8MY2ExGfUTrKwPndbt3peaa5lHsK+UGoPG48 KiWkhEaMmjaXHFa7XgVpJHhFmNoJXfPgjI/sOKTMCPQ5DEHEHTibC4mta7IBAk+rmnaOF0k8 bxHfP8Qbls66wvicrAfTRXn/1ReeNc3NP4Sq39PoVHkfQTlnQiD4eAqBdq61B7DhzjhbKAZ4 RsNtLfB6eOv9qvmblUzs50ChYewM9hvn+c7MdDH+x2UXoSDhkBDkKcJGkX91evos8s9AuoEd D32X5e+bmdUGe8Cr3cAZJ8IEXR6F9828/kxzPliMsCWVRx1Fr28baCJOUGgFPNr3ips78m9+ Iw8PdQ101jU0dvucDFxw/1SCGYEZzV+O/237oRPuLCiDX5nhQoxf6dn9ukQleLBMNy2BLI4H g342NhF21HLA+KlyLOHaMKQCKzlal+zVNZTRTCh/ikMhsxWQjBfnqTDbMj85DnWwtump27SI qhPjUnS0a6MKoS/A+hbi64k5zztkvloELfCSrX7NyBTT0jgF2IGFIxZMrKCtQ9StcGMCV9MX tjcBy6fj7QMontEaIDRJEMjg8UIGw1B687OhalOv1ISia4xOWvpYAM6ipgqh6tBQmFzasL9P h1RtcVdFpFbhwVlr1Bly8c25gBNQHL5GUjLMn45LlQz50OzrkwARAQABtCNLYW1pbCBSeXRh cm93c2tpIDxrYW1pbEBOZXRCU0Qub3JnPokCOQQTAQgAIwUCVbKF6wIbIwcLCQgHAwIBBhUI AgkKCwQWAgMBAh4BAheAAAoJEEuzCOmwLnZsrgwQAMdXTXDWkxtUciFgBnioE6hvZYOBV7Xa Gh3dwgVvS5rLwwq5ob1R9qdtCGMYxdaCAQCzo2hhUfe9ts11/Q4Pg0aDAb5CfdVVTmyvLMu+ gtK99t/sG4SfCdn8Bb8rCfRRDpkTq1cAGy6pp7rxyMrFBITTbdBWVcWdEdlMhEZtV8Z1BNDI kwEwZkYnM1UxOGW4rJNjNU+hBjNAscCTwBSbpG6NV1oBbgmgJ1PfaPCeAmGTLZyI57VLuFJy kR0Jlj8Ui7dAaJgO1WYdmvL+48s0N2QGEoHnrf50xoO34LlrIBUsCLmhtjWhZiuj0meCxNTr 5YpdBP13b2i64OCruH8/M4IO85GAIWxIMMv510rge9qSe38NHCzSmn9zcjFwVXIh9flZi7PK eqOP3yah6r1ZIBY68If/2FtvwDptUi1NHoSpN+dt0kRg26hDqMFOg+Jc6o7Wtm+3vFNDhU4I 8HkjDr62VlbHBxe6gDgVELcecWgXOydKgdrQhOPwCBJkPJigifsIz4EZQnyI3CchFja3qR9J Vo4iXwqAi6xN4RD0PS775JYDh56qUaaUsEctQ/D6Xm7Bbdv1VPlsYs/9uXxc/jWVhkd1sDn2 KZ3kv7uo04DoejVGWK9B4XEZ1ufRPzmlV0SYohX34ouLBq5Q6wbyw6+hUM+yM9RcvgkOCVgB laejuQINBFVwUF8BEAC61vNvzAAcYvkU89YoStDcGyun1ENNWpHOnuQEw613/Xgys6xZbKKa Xhee8Fiwm6FlaiYWh66Vw5cA+hMna9PDp6tZi106JnKZ9DcYxanHOCQ5V42OwUX0BDfwUIwq YgOz12Cf4pdIheVkDfiSEot3XrdI3lT8od9iWeehx5zfW77utVrWGUXkMFJKmiKzxyzjV+gF gLk2wH+L7KoYiV/MfLukLa7mTJAK4mi0sfjLStPlf5gELvPtyooKG0gs0MbDSG2qmzb1/A4Y ET8Vaa7wJulIePym+Du5TJBwptls0KEF9a04kp2Oc2zlUd/Z5z3lLBiZaXpfProbz3Ydjg4O 2+XTn+SHSq10l3agjiAkGwHH83Xnzn/clg3iTvwYgdOcwvfEnJ1FGz3DAzcBd/+IMaszJjuo dBVckt07mc97sseDjy+vIIyQGdMzDmI0U9UK7nDUFpnIfG5LYe+myBS1CgFrZAQ/WNg0j7aq CiIgbhVAOFi2sPRYlph2L8LZRUPFHLTt23vdJXdFDuKM6JSvPiDf914UpjXr/WSwT43lJzlO O3zgKGM7eclFsetDF3p0I4SVHvR7dHbIC5IHibssmk7bQgH0K1aGUX/QC18v3VY7wYYaotYH RnTiGbBGz+XxPhZYiXKQuyFu6dY3qOw/VjbsV6KVNn49z2Zg4RQV8QARAQABiQIfBBgBCAAJ BQJVcFBfAhsMAAoJEEuzCOmwLnZs9rIP/2MTyZ0252u51LFsMHa9/ylTyvl+UKq8iR852lkZ X9bH9nH4cUcen5vZo0EZI3IVOemHUq71u+DTq8PSj5vtJ0DW+sGBEbjW3Q4IjJ+96PPrlemK fYS0KWVwEzzNQLEejjduU43x83DvQ/URzSWgGnhMBqXUyJdsHyTFFNFwQ9U71gX00+wXHJyh aXRlK+7gRKtCWuNFtW/5bQXL9epxDAS0POIVAdBc1FtPLwg08Pj0KwHsGQpEr5/W8ybDtLF+ zISHIKCj1lZ8dv/7D1PmH5SEXzsv+bbzvPtb6zhoIA8HONshaG2eAYknAiCJZ0gj0npgktwc u9VkvDvHMD9+VyNzRV/M6Ak4nDeEG6QecTPv8IqCcAHDI27nY/49BvFVOJOMwqbTp5Xvfa71 ksP1mARrN+bIYMfOy7OhfCxGeZydvBhgCLKdL698aXmgy0xrmrOw+GXO69GVcebOvxWMXxz1 FOG/JnLIe1ZgCo2YF5wy8zTCGKCMx6gAwnku2nJmDGNsePVedV00FmB8mQ7Oxz+3B9+LtFim FHHR33PlRnViXlG+XTm9NontiGE0LvG4TzIY5CYNSw8PBao795dQMSsmMI4FHlvTIgupE9g1 PQWP+2H2C0RtnLUanRNUGRkze1+MNG7jc+fqJIo5s7+PSs26rUvA38QzEOJ95k7hdJty Subject: Re: [PATCH v2 10/10] Add minimal and functional NetBSD/amd64 gdbserver Message-ID: <219b29d1-2dc9-5843-6893-4e754913576c@netbsd.org> Date: Tue, 8 Sep 2020 02:03:53 +0200 User-Agent: Mozilla/5.0 (X11; NetBSD amd64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <071c608e-0709-6fff-cd42-4981b3d0bdb6@simark.ca> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="tJmPUfQwxUxumfXusH7IcpAf3eWvh94gr" X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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, 08 Sep 2020 00:06:55 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --tJmPUfQwxUxumfXusH7IcpAf3eWvh94gr Content-Type: multipart/mixed; boundary="UdEo4mzRqa2Tz0MtuYdHGm43bfL4mHB9B"; protected-headers="v1" From: Kamil Rytarowski To: Simon Marchi , gdb-patches@sourceware.org Cc: tom@tromey.com Message-ID: <219b29d1-2dc9-5843-6893-4e754913576c@netbsd.org> Subject: Re: [PATCH v2 10/10] Add minimal and functional NetBSD/amd64 gdbserver References: <20200904002905.13616-1-n54@gmx.com> <20200904002905.13616-11-n54@gmx.com> <071c608e-0709-6fff-cd42-4981b3d0bdb6@simark.ca> In-Reply-To: <071c608e-0709-6fff-cd42-4981b3d0bdb6@simark.ca> --UdEo4mzRqa2Tz0MtuYdHGm43bfL4mHB9B Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 07.09.2020 21:58, Simon Marchi wrote: > I did a superficial review, that's as much as I can, since I can't easi= ly 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. >=20 > So I checked for obvious mistakes (didn't find any) and coding standard= s (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. >=20 Thank you for the review! Right, there could be bugs and there is room for improvement and adding new features. Nonetheless, this code already works. > 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_mem= ory, >> 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. >=20 > Let's name this new file netbsd-amd64-low.cc. We use amd64 everywhere,= so it's > better to stay consistent. >=20 OK >> +/* Implement the fetch_registers target_ops method. */ >> + >> +void >> +netbsd_process_target::fetch_registers (struct regcache *regcache, in= t regno) >> +{ >> + struct netbsd_regset_info *regset =3D netbsd_target_regsets; >> + ptid_t inferior_ptid =3D ptid_of (current_thread); >> + >> + while (regset->size >=3D 0) >> + { >> + char *buf =3D (char *) xmalloc (regset->size); >> + int res =3D ptrace (regset->get_request, inferior_ptid.pid (), = buf, >> + inferior_ptid.lwp ()); >> + if (res =3D=3D -1) >> + perror_with_name (("ptrace")); >> + regset->store_function (regcache, buf); >> + free (buf); >=20 > The perror would leak `buf`. Use some automatic memory management type= =2E >=20 OK >> + regset++; >> + } >> +} >> + >> +/* Implement the store_registers target_ops method. */ >> + >> +void >> +netbsd_process_target::store_registers (struct regcache *regcache, in= t regno) >> +{ >> + struct netbsd_regset_info *regset =3D netbsd_target_regsets; >> + ptid_t inferior_ptid =3D ptid_of (current_thread); >> + >> + while (regset->size >=3D 0) >> + { >> + char *buf =3D (char *)xmalloc (regset->size); >> + int res =3D ptrace (regset->get_request, inferior_ptid.pid (), = buf, >> + inferior_ptid.lwp ()); >> + if (res =3D=3D -1) >> + perror_with_name (("ptrace")); >> + if (res =3D=3D 0) >> + { >> + /* Then overlay our cached registers on that. */ >> + regset->fill_function (regcache, buf); >> + /* Only now do we write the register set. */ >> + res =3D ptrace (regset->set_request, inferior_ptid.pid (), buf, >> + inferior_ptid.lwp ()); >> + } >> + free (buf); >=20 > Same here. >=20 OK >> +/* 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 =3D sizeof (auxv_type); >> + const size_t auxv_buf_size =3D 128 * sizeof (auxv_type); >> + >> + char *auxv_buf =3D (char *) xmalloc (auxv_buf_size); >> + >> + netbsd_read_auxv (pid, nullptr, auxv_buf, auxv_buf_size); >> + >> + *phdr_memaddr =3D 0; >> + *num_phdr =3D 0; >> + >> + for (char *buf =3D auxv_buf; buf < (auxv_buf + auxv_buf_size); buf = +=3D auxv_size) >> + { >> + auxv_type *const aux =3D (auxv_type *) buf; >> + >> + switch (aux->a_type) >> + { >> + case AT_PHDR: >> + *phdr_memaddr =3D aux->a_v; >> + break; >> + case AT_PHNUM: >> + *num_phdr =3D aux->a_v; >> + break; >> + } >> + >> + if (*phdr_memaddr !=3D 0 && *num_phdr !=3D 0) >> + break; >> + } >> + >> + xfree (auxv_buf); >=20 > Same here. >=20 OK >> +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)= ; >=20 > I find the distinction between "fill" and "store" unclear, it doesn't r= eally tell > which direction it's going (looking at the types helps though). fill t= he regcache? > fill the buffer? store in the regcache? store in the buffer? >=20 > It's the same with the regcache::supply_regset and regcache::collect_re= gset 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? >=20 > What about: >=20 > void (*buf_to_regcache) (struct regcache *regcache, char *buf); > void (*regcache_to_buf) (struct regcache *regcache, const char *buf);= >=20 > You could add the `_function` suffix if you prefer, though I don't thin= k it's necessary. >=20 I will leave the naming as it is in this patch as it exists in Linux and some generic code. I have got no opinion on renaming, if we go for it, it should happen independently. > Simon >=20 --UdEo4mzRqa2Tz0MtuYdHGm43bfL4mHB9B-- --tJmPUfQwxUxumfXusH7IcpAf3eWvh94gr Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEELaxVpweEzw+lMDwuS7MI6bAudmwFAl9WymkACgkQS7MI6bAu dmyQPBAAsLU3tu4ldeuvvSQ4saOdSS5fHgyv4wubP9gBUOPnq8puBftBBXmslO9V xzK4DLLL4WhP7/epruYNE/p8EPlnXjX/GFokErcmju3iJbnR/yqpljGPgJ8F/K14 TADtbMJRBmsX2jEXtkhFLnChpifVRzJSYBhlquuL+eqBBOvsA60kBhagrv7Warbr /n0Ch9oOIBbVqtei0X8ap3zscQYC2pRQ4GywYETFYN0c1YIGxMX8fcgCr3ZgsdcK DlvBzSy/rPsLVdyzeUtzU0vOngMBCE1kDvuPlDuSzHu/J5vwbNk3WKMP6wF9VlcL BRITPR2WWKY7rkVnORp6Av/qHl40PTLV7FT0CxYkkGYGPESG6xDtRZniHxZZv20V x9MjQWhVh6VOWm+jXNERpinPI9YR1B1ecHZfiZpTMIcEmZlT5Ws4DdA8ejiHJcTk IWgH4kMaVjYaCVt2VDs6Uu9V1eAY0R3voc9eyztczRn1CW57EYoCkO80I1t4kBvn cjp9BGdM7awYWa2LcO5KkvQkeyVmEh6Pcz0pWCFbizfnKHa9a7REIclAGsVyPJDW +4R1mZFvzFM+iwnL2NC3CbpRhJmfEIByJS4HHJPmaFBnkZ17f63c3gGcw8NwLPPE ulSyf6vMcI4iG40teUMYnlZB6SxEjKNcIZ+4UBg2gaq8HTfP8JY= =Qvuf -----END PGP SIGNATURE----- --tJmPUfQwxUxumfXusH7IcpAf3eWvh94gr--