[PATCH] Implement xfer_partial TARGET_OBJECT_SIGNAL_INFO for NetBSD
Kamil Rytarowski
kamil@netbsd.org
Tue Jul 28 15:07:32 GMT 2020
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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://sourceware.org/pipermail/gdb-patches/attachments/20200728/b28b9eb4/attachment-0001.sig>
More information about the Gdb-patches
mailing list