Re: [PATCH] Implement xfer_partial TARGET_OBJECT_SIGNAL_INFO for NetBSD

[Simon Marchi <simark@simark.ca>]

A few comments, the patch LGTM with these fixed.

On 2020-07-27 3:41 a.m., Kamil Rytarowski wrote:
> NetBSD implementes reading and overwriting siginfo_t received by the

"implementes"

> tracee. With TARGET_OBJECT_SIGNAL_INFO signal information can be
> examined and modified through the special variable $_siginfo.
> 
> Implement the "get_siginfo_type" gdbarch method for NetBSD architectures.
> 
> As with Linux architectures, cache the created type in the gdbarch when it
> is first created.  Currently NetBSD uses an identical siginfo type on
> all architectures, so there is no support for architecture-specific fields.

Watch out tabs vs spaces at a few places in the patch.

> +{
> +  pid_t pid = inferior_ptid.pid ();
> +
> +  switch (object)
> +    {
> +    case TARGET_OBJECT_SIGNAL_INFO:
> +      {
> +	ptrace_siginfo_t psi;
> +
> +	if (offset > sizeof(siginfo_t))
> +	  return TARGET_XFER_E_IO;
> +
> +	if (ptrace (PT_GET_SIGINFO, pid, &psi, sizeof (psi)) == -1)
> +	  return TARGET_XFER_E_IO;
> +
> +	if (offset + len > sizeof(siginfo_t))
> +          len = sizeof(siginfo_t) - offset;

Space after all instances of `sizeof`.

> +    }
> +}
> diff --git a/gdb/nbsd-nat.h b/gdb/nbsd-nat.h
> index 0a7048ecf35..59210ad1d03 100644
> --- a/gdb/nbsd-nat.h
> +++ b/gdb/nbsd-nat.h
> @@ -49,6 +49,12 @@ struct nbsd_nat_target : public inf_ptrace_target
>      override;
> 
>    bool supports_multi_process () override;
> +  enum target_xfer_status xfer_partial (enum target_object object,
> +                                        const char *annex,
> +                                        gdb_byte *readbuf,
> +                                        const gdb_byte *writebuf,
> +                                        ULONGEST offset, ULONGEST len,
> +                                        ULONGEST *xfered_len) override;
>  };
> 
>  #endif /* nbsd-nat.h */
> diff --git a/gdb/nbsd-tdep.c b/gdb/nbsd-tdep.c
> index 4fdfe476b59..faf41313f68 100644
> --- a/gdb/nbsd-tdep.c
> +++ b/gdb/nbsd-tdep.c
> @@ -373,6 +373,169 @@ nbsd_skip_solib_resolver (struct gdbarch *gdbarch, CORE_ADDR pc)
>      return find_solib_trampoline_target (get_current_frame (), pc);
>  }
> 
> +static struct gdbarch_data *nbsd_gdbarch_data_handle;
> +
> +struct nbsd_gdbarch_data
> +  {
> +    struct type *siginfo_type;
> +  };

Remove leading indentation (`{` and `}` on column 0).

> +
> +static void *
> +init_nbsd_gdbarch_data (struct gdbarch *gdbarch)
> +{
> +  return GDBARCH_OBSTACK_ZALLOC (gdbarch, struct nbsd_gdbarch_data);
> +}
> +
> +static struct nbsd_gdbarch_data *
> +get_nbsd_gdbarch_data (struct gdbarch *gdbarch)
> +{
> +  return ((struct nbsd_gdbarch_data *)
> +          gdbarch_data (gdbarch, nbsd_gdbarch_data_handle));
> +}
> +
> +/* Implement the "get_siginfo_type" gdbarch method.  */
> +
> +static struct type *
> +nbsd_get_siginfo_type (struct gdbarch *gdbarch)
> +{
> +  struct nbsd_gdbarch_data *nbsd_gdbarch_data;
> +  struct type *char_type, *int_type, *long_type;
> +  struct type *void_ptr_type;
> +  struct type *int32_type, *uint32_type, *uint64_type;
> +  struct type *pid_type, *lwpid_type, *uid_type, *clock_type;
> +  struct type *sigval_type, *option_type;
> +  struct type *siginfo_type;
> +  struct type *ksiginfo_type;
> +  struct type *reason_type;
> +  struct type *type;
> +  bool lp64;

Feel free to declare the variables when first using them and skip the `struct` keyword.

> +
> +  nbsd_gdbarch_data = get_nbsd_gdbarch_data (gdbarch);
> +  if (nbsd_gdbarch_data->siginfo_type != NULL)
> +    return nbsd_gdbarch_data->siginfo_type;
> +
> +  char_type = builtin_type (gdbarch)->builtin_char;
> +  int_type = builtin_type (gdbarch)->builtin_int;
> +  long_type = builtin_type (gdbarch)->builtin_long;
> +
> +  void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
> +
> +  int32_type = builtin_type (gdbarch)->builtin_int32;
> +  uint32_type = builtin_type (gdbarch)->builtin_uint32;
> +  uint64_type = builtin_type (gdbarch)->builtin_uint64;
> +
> +  lp64 = TYPE_LENGTH (void_ptr_type) == 8;
> +
> +  /* pid_t */
> +  pid_type = arch_type (gdbarch, TYPE_CODE_TYPEDEF,
> +			TYPE_LENGTH (int32_type) * TARGET_CHAR_BIT, "pid_t");

TARGET_CHAR_BIT is bogus, since GDB can have multiple targets with possibly different
target char sizes.  It should really be removed.

Use gdbarch_addressable_memory_unit_size, and multiply that value by 8 to get
the target char size in bits.

> +  TYPE_TARGET_TYPE (pid_type) = int32_type;
> +  TYPE_TARGET_STUB (pid_type) = 1;

Is STUB really needed here?  The TYPE_TARGET_STUB comment says it only applies to
arrays and ranges.

> +
> +  /* uid_t */
> +  uid_type = arch_type (gdbarch, TYPE_CODE_TYPEDEF,
> +			TYPE_LENGTH (uint32_type) * TARGET_CHAR_BIT,
> +			"uid_t");
> +  TYPE_TARGET_TYPE (uid_type) = uint32_type;
> +  TYPE_TARGET_STUB (uid_type) = 1;
> +
> +  /* clock_t */
> +  clock_type = arch_type (gdbarch, TYPE_CODE_TYPEDEF,
> +			  TYPE_LENGTH (int_type) * TARGET_CHAR_BIT,
> +			  "clock_t");
> +  TYPE_TARGET_TYPE (clock_type) = int_type;
> +  TYPE_TARGET_STUB (clock_type) = 1;
> +
> +  /* lwpid_t */
> +  lwpid_type = arch_type (gdbarch, TYPE_CODE_TYPEDEF,
> +			  TYPE_LENGTH (int32_type) * TARGET_CHAR_BIT,
> +			  "lwpid_t");
> +  TYPE_TARGET_TYPE (lwpid_type) = int32_type;
> +  TYPE_TARGET_STUB (lwpid_type) = 1;
> +
> +  /* union sigval */
> +  sigval_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
> +  sigval_type->set_name (xstrdup ("sigval"));

If these types are tied to the gdbarch, maybe allocate it on the gdbarch's
obstack, just like the type is?  You would use gdbarch_obstack_strdup.

You could also pass the name directly to arch_composite_type.  You would
still need to strdup it.  Oddly enough, arch_type strdups its `name` argument,
but arch_composite_type does not.

> +
> +void _initialize_nbsd_tdep ();
> +void
> +_initialize_nbsd_tdep ()
> +{
> +  nbsd_gdbarch_data_handle =
> +    gdbarch_data_register_post_init (init_nbsd_gdbarch_data);

Assignment operator on the next line:

  nbsd_gdbarch_data_handle
    = ...

Simon