From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id ED3993844046 for ; Tue, 28 Jul 2020 13:26:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org ED3993844046 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9F9A11E794; Tue, 28 Jul 2020 09:26:20 -0400 (EDT) Subject: Re: [PATCH] Implement xfer_partial TARGET_OBJECT_SIGNAL_INFO for NetBSD To: Kamil Rytarowski , gdb-patches@sourceware.org References: <20200727074118.4030-1-n54@gmx.com> From: Simon Marchi Message-ID: <48da991a-84ea-012f-8a6c-e21d90dcf079@simark.ca> Date: Tue, 28 Jul 2020 09:26:14 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200727074118.4030-1-n54@gmx.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Jul 2020 13:26:22 -0000 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