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 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. > 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_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. > OK >> +/* 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. > OK >> + 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. > 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 = 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. > 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); > > 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. > 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 >