From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92751 invoked by alias); 17 Oct 2018 19:13:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 92741 invoked by uid 89); 17 Oct 2018 19:13:40 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=ownership, reader X-HELO: mail.baldwin.cx Received: from bigwig.baldwin.cx (HELO mail.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 Oct 2018 19:13:38 +0000 Received: from John-Baldwins-MacBook-Pro-2.local (ralph.baldwin.cx [66.234.199.215]) by mail.baldwin.cx (Postfix) with ESMTPSA id AC26710AFCD; Wed, 17 Oct 2018 15:13:35 -0400 (EDT) Subject: Re: [PATCH 1/2] Add an optional "alias" attribute to syscall entries. To: Sergio Durigan Junior References: <20181003173005.19581-1-jhb@FreeBSD.org> <20181003173005.19581-2-jhb@FreeBSD.org> <87pnwe62bl.fsf@redhat.com> Cc: gdb-patches@sourceware.org From: John Baldwin Message-ID: Date: Wed, 17 Oct 2018 19:13:00 -0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <87pnwe62bl.fsf@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg00385.txt.bz2 On 10/12/18 6:51 PM, Sergio Durigan Junior wrote: > On Wednesday, October 03 2018, John Baldwin wrote: > >> When setting a syscall catchpoint by name, catch syscalls whose name >> or alias matches the requested string. > > Thanks for the patch, John. > >> When the ABI of a system call is changed in the FreeBSD kernel, this >> is implemented by leaving a compatability system call using the old >> ABI at the existing "slot" and allocating a new system call for the >> version using the new ABI. For example, new fields were added to the >> 'struct kevent' used by the kevent() system call in FreeBSD 12. The >> previous kevent() system call in FreeBSD 12 kernels is now called >> freebsd11_kevent() and is still used by older binaries compiled >> against the older ABI. The freebsd11_kevent() system call can be >> tagged with an "alias" attribute of "kevent" permitting 'catch syscall >> kevent' to catch both system calls and providing the expected user >> behavior for both old and new binaries. It also provides the expected >> behavior if GDB is compiled on an older host (such as a FreeBSD 11 >> host). > > OOC, do you envision the possibility of a syscall having more than 1 > alias? I don't see a use case for more than one alias given the existing group functionality. >> @@ -96,8 +94,8 @@ get_syscall_group_names (struct gdbarch *gdbarch) >> /* Structure which describes a syscall. */ >> struct syscall_desc >> { >> - syscall_desc (int number_, std::string name_) >> - : number (number_), name (name_) >> + syscall_desc (int number_, std::string name_, std::string alias_) >> + : number (number_), name (name_), alias(alias_) > ^ > > Missing whitespace here. Fixed. >> {} >> >> /* The syscall number. */ >> @@ -206,10 +208,10 @@ syscall_group_add_syscall (struct syscalls_info *syscalls_info, >> >> static void >> syscall_create_syscall_desc (struct syscalls_info *syscalls_info, >> - const char *name, int number, >> + const char *name, int number, const char *alias, >> char *groups) >> { >> - syscall_desc *sysdesc = new syscall_desc (number, name); >> + syscall_desc *sysdesc = new syscall_desc (number, name, alias ?: ""); > > As pointed by Kevin, this use of the ternary operator is not common. Yes, I'll expand. >> @@ -389,21 +396,21 @@ syscall_group_get_group_by_name (const struct syscalls_info *syscalls_info, >> return NULL; >> } >> >> -static int >> -xml_get_syscall_number (struct gdbarch *gdbarch, >> - const char *syscall_name) >> +static std::vector >> +xml_get_syscalls_by_name (struct gdbarch *gdbarch, const char *syscall_name) >> { >> struct syscalls_info *syscalls_info = gdbarch_syscalls_info (gdbarch); >> + std::vector syscalls; >> >> if (syscalls_info == NULL >> || syscall_name == NULL) >> - return UNKNOWN_SYSCALL; >> + return syscalls; >> >> for (const syscall_desc_up &sysdesc : syscalls_info->syscalls) >> - if (sysdesc->name == syscall_name) >> - return sysdesc->number; >> + if (sysdesc->name == syscall_name || sysdesc->alias == syscall_name) >> + syscalls.push_back (sysdesc->number); > > You can simplify this code by putting the "for" above inside an "if" > like: > > if (syscalls_info != NULL && syscall_name != NULL) > for (const syscall_desc_up &sysdesc : syscalls_info->syscalls) > if (sysdesc->name == syscall_name || sysdesc->alias == syscall_name) > syscalls.push_back (sysdesc->number); > > And then you can get rid of the first "if". Ok. >> >> - return UNKNOWN_SYSCALL; >> + return syscalls; >> } >> >> static const char * >> @@ -522,14 +529,12 @@ get_syscall_by_number (struct gdbarch *gdbarch, >> s->name = xml_get_syscall_name (gdbarch, syscall_number); >> } >> >> -void >> -get_syscall_by_name (struct gdbarch *gdbarch, >> - const char *syscall_name, struct syscall *s) >> +std::vector >> +get_syscalls_by_name (struct gdbarch *gdbarch, const char *syscall_name) > > I confess I don't feel very happy with this rename. This function > expects the full name of the syscall to be passed via SYSCALL_NAME, and > it doesn't do any fuzzy matching, so it can be confusing to the reader > understanding why SYSCALL_NAME can map to more than 1 syscall number. > Of course one can put an explanation in the comment, but I'd rather see > a more explicit interface. Maybe you can extend "struct syscall" and > include fields for an "alias_name" and "alias_number" there. Alias fields wouldn't work as you can have N aliases. For example in the updated mapping for FreeBSD 12 there are 3 versions of "fhstatfs" so that 'catch syscall fhstatfs' matches 3 syscalls: (gdb) catch syscall fhstatfs Catchpoint 1 (syscalls 'freebsd4_fhstatfs' [297] 'freebsd11_fhstatfs' [398] 'fhstatfs' [558]) The only caller of this function doesn't actually expect to get full 'struct syscall' objects back, but just integers. I could instead return a vector of 'struct syscall' perhaps but only the integers would be used. That said, get_syscalls_by_group returns an array of 'struct syscall' objects (it doesn't use a vector, but uses a plain C array). We could share some code in the consumer if we made get_syscalls_by_group and this function follow the same convention. I would probably want to use a std::vector rather than a plain C array as the memory management/ownership is cleaner. > I confess I didn't run your code. What's the output of "catch syscall" > when an alias is found? I think it might be interesting to explicitly > mark the extra syscall as an alias. It might take a bit more tweaking > to the code, but I like the idea of being explicit here. I posted some output above. The aliases are kind of marked already due to their name in the case of FreeBSD's syscall table. -- John Baldwin                                                                            Â