Re: [PATCH v2 10/10] Add minimal and functional NetBSD/amd64 gdbserver

[Simon Marchi <simark@simark.ca>]

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 <typename T>
> +int get_phdr_phnum_from_proc_auxv (const pid_t pid,
> +				   CORE_ADDR *phdr_memaddr, int *num_phdr)
> +{
> +  typedef typename std::conditional<sizeof(T) == sizeof(int64_t),
> +				    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