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

[Kamil Rytarowski <kamil@netbsd.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6471 bytes --]

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 <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.
> 

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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]