Re: [PATCH] Implement xfer_partial TARGET_OBJECT_SIGNAL_INFO for NetBSD

[Kamil Rytarowski <kamil@netbsd.org>]

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

On 28.07.2020 15:26, Simon Marchi wrote:
> 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"
> 

Done.

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

Please specify exact lines if there is still anything left.

>> +{
>> +  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`.
> 

Done.

>> +    }
>> +}
>> 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).
> 

Done.

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

Done.

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

Done.

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

Done.

>> +
>> +  /* 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.
> 

This looks like a generic GDB bug wiith this arch_composite_type vs
arch_type. For now, I have used set_name + gdbarch_obstack_strdup.

Feel free to fix the problem in GDB later and pass the name directly in
arch_composite_type.

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

Done.

> Simon
> 



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